Skip to content

Support INSERT OVERWRITE for MemTable - #24969

Queued
osipovartem wants to merge 6 commits into
apache:mainfrom
Embucket:upstream-memtable-insert-overwrite
Queued

Support INSERT OVERWRITE for MemTable#24969
osipovartem wants to merge 6 commits into
apache:mainfrom
Embucket:upstream-memtable-insert-overwrite

Conversation

@osipovartem

@osipovartem osipovartem commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

DataFusion parses and plans INSERT OVERWRITE, but MemTable rejects 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?

  • allow InsertOp::Overwrite in MemTable::insert_into
  • let MemSink replace each target partition after the input stream completes successfully
  • continue rejecting the distinct InsertOp::Replace operation

What 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::Replace remains unsupported.

Validated with:

  • cargo test -p datafusion --lib test_insert_overwrite -- --nocapture
  • cargo clippy -p datafusion -p datafusion-datasource -p datafusion-catalog --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check

Are there any user-facing changes?

Yes. INSERT OVERWRITE now replaces all existing data in a MemTable. There are no breaking public API changes.

@github-actions github-actions Bot added core Core DataFusion crate catalog Related to the catalog crate datasource Changes to the datasource crate labels Sep 6, 2026
@codecov-commenter

codecov-commenter commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.82609% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.74%. Comparing base (46e3158) to head (d3fa44a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/core/src/datasource/memory_test.rs 87.37% 4 Missing and 9 partials ⚠️
datafusion/catalog/src/memory/table.rs 66.66% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@osipovartem
osipovartem force-pushed the upstream-memtable-insert-overwrite branch from 2d7b03b to 2e11c24 Compare September 6, 2026 16:38

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osipovartem,

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<()> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@osipovartem
osipovartem requested a review from kosiew September 8, 2026 12:49

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osipovartem,

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

kosiew commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🚀
@osipovartem
Thank you for your contribution.

@kosiew
kosiew added this pull request to the merge queue Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

catalog Related to the catalog crate core Core DataFusion crate datasource Changes to the datasource crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants