Repository navigation
Support INSERT OVERWRITE for MemTable - #24969
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24969 +/- ##
========================================
Coverage 81.92% 81.92%
========================================
Files 1134 1134
Lines 426000 426111 +111
Branches 426000 426111 +111
========================================
+ Hits 349000 349091 +91
- Misses 56304 56313 +9
- Partials 20696 20707 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2d7b03b to
2e11c24
Compare
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The change looks good overall, and I like that INSERT OVERWRITE is enabled for MemTable while Replace remains explicitly unsupported. The buffering before modifying the target also makes the overwrite behavior safer if the input stream fails.
I have one non-blocking test coverage suggestion below.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_insert_overwrite_replaces_existing_data() -> Result<()> { |
There was a problem hiding this comment.
Could we add an overwrite test with multiple target partitions and multiple input batches? The current tests cover a single target partition, while MemSink::write_all distributes batches round-robin and then replaces each target partition independently. A test that asserts every partition contains only its replacement batches would give us coverage for the full overwrite loop.
There was a problem hiding this comment.
Added in e8c94de. The new test uses two target partitions with distinct existing rows and four replacement batches, then verifies the round-robin batch distribution and that both partitions contain only replacement data. The targeted tests and the PR clippy command pass locally.
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I reviewed the latest changes and the additional overwrite coverage looks good.
The earlier request from @kosiew is addressed by e8c94de. The new test starts with distinct data across two target partitions, feeds four replacement batches, and confirms that each output partition contains only its round-robin assigned replacement values: [10, 30] and [20, 40].
I also verified the author's follow-up against the current HEAD, and the Codecov report does not point to any new actionable issue.
I did not find any additional problems in the follow-up changes. Looks good to me.
|
🚀 |
kosiew
left a comment
There was a problem hiding this comment.
@osipovartem, thanks for the update. I took another look and the previous comment has been addressed. Commit e8c94dea6 now covers two distinct target partitions with four replacement batches, and verifies the expected round-robin outputs [10, 30] and [20, 40] while confirming the previous rows are gone.
I didn't find any new issues in the follow-up changes. The latest HEAD update is a merge from main and doesn't introduce any additional branch-side changes related to this feature.
Looks good to me. Thanks!
|
🚀 |
Which issue does this PR close?
Rationale for this change
DataFusion parses and plans
INSERT OVERWRITE, butMemTablerejects every insert operation except append. Users therefore cannot replace the contents of an in-memory table with the standard overwrite operation.What changes are included in this PR?
InsertOp::OverwriteinMemTable::insert_intoMemSinkreplace each target partition after the input stream completes successfullyInsertOp::ReplaceoperationWhat is the testing strategy for this PR?
Three unit tests verify that overwrite replaces existing rows, distributes multiple replacement batches across multiple target partitions, and clears the table for empty input. A separate test confirms that
InsertOp::Replaceremains unsupported.Validated with:
cargo test -p datafusion --lib test_insert_overwrite -- --nocapturecargo clippy -p datafusion -p datafusion-datasource -p datafusion-catalog --all-targets --all-features -- -D warningscargo fmt --all -- --checkAre there any user-facing changes?
Yes.
INSERT OVERWRITEnow replaces all existing data in aMemTable. There are no breaking public API changes.