Support INSERT OVERWRITE for MemTable - #24969
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24969 +/- ##
========================================
Coverage 81.74% 81.74%
========================================
Files 1128 1128
Lines 416644 416755 +111
Branches 416644 416755 +111
========================================
+ Hits 340587 340683 +96
- Misses 55998 56005 +7
- Partials 20059 20067 +8 ☔ 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.
|
🚀 |
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.