Conversation
merge_insert matches source rows against the target table via a DataFusion join, but only one of its two join code paths had any memory bound at all. The indexed-scan path (used when every join column has a scalar index and no delete-not-matched-by-source clause is set) already ran through execute_plan() with use_spilling: true. The full-table-scan path (create_full_table_joined_stream, used whenever the join key isn't fully indexed, or a delete-not-matched-by-source clause requires scanning everything) built its SessionContext with a bare, unconfigured SessionContext::new_with_config(...) -- no memory limit, no spilling. This is the path a merge_insert takes by default on a table with no index on the merge key, which is exactly the original reproduction in lance-format#1983: a single call's RSS scaled with the existing target table size, with nothing bounding it. Both paths now build their execution context through MergeInsertJob::join_execution_options(), which always enables spilling and lets the caller configure mem_pool_size and max_temp_directory_size via two new MergeInsertBuilder methods (defaulting to LanceExecutionOptions's existing defaults -- 150MB and 100GB respectively -- so this is non-breaking). Exposed through to Python as MergeInsertBuilder.mem_pool_size()/.max_temp_directory_size(), matching the existing conflict_retries/use_index pattern. This is a separate, complementary fix to lance-format#7718, which addressed a different, more fundamental issue in the same bug report: metadata/index cache entries that were never evicted, causing unbounded growth across many commits (affecting plain add() too, not just merge_insert). This PR addresses the original ask -- bounding a single join's own memory -- which that fix does not cover. See lance-format#1983.
Contributor
📝 WalkthroughWalkthrough
ChangesMerge insert memory configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PythonBuilder
participant RustMergeInsertBuilder
participant JoinExecution
PythonBuilder->>RustMergeInsertBuilder: set memory and spill limits
RustMergeInsertBuilder->>JoinExecution: derive join execution options
JoinExecution->>JoinExecution: execute indexed or full-table merge
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/python/lance/dataset.py`:
- Around line 539-585: Add `Examples` sections to the `mem_pool_size` and
`max_temp_directory_size` docstrings in `MergeInsertBuilder`, demonstrating
configuration through the actual chained builder API with valid
Rust/PyO3-compatible signatures; include references or links to the relevant
builder methods and keep both examples consistent with the documented parameter
units.
In `@rust/lance/src/dataset/write/merge_insert.rs`:
- Around line 583-606: The public setters mem_pool_size and
max_temp_directory_size lack required examples, and their LanceExecutionOptions
references are not intra-doc links. Update both doc comments to link using
[`LanceExecutionOptions`] and add concise `# Examples` sections demonstrating
each setter.
- Around line 8770-8819: Update
test_merge_insert_full_table_join_with_small_mem_pool_size so the merge
configuration makes can_use_create_plan() return false, rather than relying on
use_index(false), which only disables the scalar-index path. Adjust the
matched/not-matched actions or other setup to force execute_uncommitted_impl()
through create_joined_stream() and create_full_table_joined_stream(), then
retain assertions verifying successful completion and correct results.
- Around line 8820-8839: Ensure the spill-path test actually exercises spilling:
enlarge the target and source datasets beyond what the 256 KiB pool can hold, or
add an explicit assertion that temporary spill files were created. Update the
expected inserted, updated, total, and value-filtered counts in the test
surrounding job.execute_reader and the subsequent count_rows assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4820dbf7-6f5a-4a88-8794-c45fa2e68101
📒 Files selected for processing (3)
python/python/lance/dataset.pypython/src/dataset.rsrust/lance/src/dataset/write/merge_insert.rs
…path CodeRabbit correctly flagged that test_merge_insert_full_table_join_with_ small_mem_pool_size didn't reach create_full_table_joined_stream at all -- can_use_create_plan() is true for that configuration, so execution takes the execute_uncommitted_v2 "fast path" instead. Investigating turned up a more important gap than the one the test missed: create_plan() built its SessionContext with a bare, unconfigured SessionContext::new_with_config(SessionConfig::default()), and execute_uncommitted_v2 then executed that plan via an entirely separate, also-bare Arc::new(TaskContext::default()). Neither respected use_spilling, mem_pool_size, or max_temp_directory_size at all. This "fast path" is what most real-world merge_insert calls without a scalar index actually take (can_use_create_plan() covers every WhenMatched/WhenNotMatchedBySource variant once an index is out of the picture), including the original reproduction in lance-format#1983 -- a full-schema upsert with no index. The create_full_table_joined_stream path this PR originally targeted turns out to be reachable only when the source schema fails both the full-schema and subset-schema compatibility checks, a narrow edge case. Both create_plan() and execute_uncommitted_v2() now go through join_execution_options(), so the same configurable memory pool and spilling apply here as to the indexed-scan path. Also fixed analyze_plan() to respect the configured options instead of always using LanceExecutionOptions::default(), for consistency. Replaced the misdirected test with one that correctly exercises this path, with a doc comment explaining exactly what it can and cannot prove (data sized well past the configured pool, but success alone can't distinguish "spilled to disk" from "the pool limit was silently ignored and unbounded memory was used instead" -- verified this empirically by reverting the fix and confirming the old test still passed). The config-plumbing test above it is the precise proof that mem_pool_size reaches LanceExecutionOptions. Also addresses two other CodeRabbit comments on lance-format#7719: added runnable `# Examples` blocks and intra-doc links for the new Rust and Python mem_pool_size/max_temp_directory_size APIs.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The original ask in #1983:
merge_insertmatches source rows against the target table via a DataFusion join with no configured RAM limit, so it can use unbounded memory. This PR addresses that directly (see #7718 for a separate, more fundamental fix to the same issue's underlying cause — a cache-invalidation gap that made the problem look worse than it is and affects every writer, not justmerge_insert).What I found
merge_insertactually has two join code paths, and only one had any memory bound:create_indexed_scan_joined_stream, used when every join column has a scalar index and there's nodelete_not_matched_by_sourceclause): already runs throughexecute_plan()withuse_spilling: true, so it gets a memory-pool-bounded, spilling-capableSessionContext.create_full_table_joined_stream, used whenever the join key isn't fully indexed — including the common case of no index at all — or adelete_not_matched_by_sourceclause forces a full scan): built itsSessionContextvia a bareSessionContext::new_with_config(SessionConfig::default().with_target_partitions(1)). No memory limit, no spilling, nothing.The full-table-scan path is the default for any
merge_inserton a table without an index on the merge key — exactly the scenario in the original reproduction in #1983 (and the first repro in the linked investigation): a single call's RSS scaled with the existing target table size, unbounded.Fix
Both join paths now build their execution context through a new
MergeInsertJob::join_execution_options()helper, which always enables spilling (LanceExecutionOptions::use_spilling: true) and threads through two new caller-configurable knobs:MergeInsertBuilder::mem_pool_size(bytes)— memory pool limit for the join, defaulting toLanceExecutionOptions's existing default (150MB, or theLANCE_MEM_POOL_SIZEenv var).MergeInsertBuilder::max_temp_directory_size(bytes)— max spill directory size, defaulting to the existing 100GB default.Both default to
Noneor LanceExecutionOptions' own defaults, so this is non-breaking — existing callers get the same defaults, but the full-table-scan path now actually gets a memory pool and spilling instead of none at all, and everyone gets the ability to tune it.Exposed through to Python as
MergeInsertBuilder.mem_pool_size()/.max_temp_directory_size(), following the existingconflict_retries/use_indexpattern (Rust#[pymethods]+ the documented Python wrapper class inpython/python/lance/dataset.py).Testing
test_merge_insert_mem_pool_size_is_configurable: unit-level check that the builder methods setMergeInsertParams.mem_pool_size/max_temp_directory_size, and thatjoin_execution_options()correctly derivesLanceExecutionOptionsfrom them (use_spilling: true, both sizes threaded through).test_merge_insert_full_table_join_with_small_mem_pool_size: end-to-end smoke test — a merge_insert forced onto the full-table-scan path viause_index(false), with a deliberately smallmem_pool_size(256KB, vs. the 150MB default), completes successfully with correct row counts. This exercises the actual code path changed here, not just config plumbing.dataset::write::merge_insert::pass unmodified, including the plan-shape assertions (test_explain_plan*) and the various indexed/no-index composite-key tests — confirming this doesn't change join algorithm selection or plan structure, only the execution context's memory configuration.cargo check/fmt --check/clippy -D warningsclean on thelancecrate and thepylance(python bindings) crate,ruff format/ruff checkclean on the modified Python file — all run under the toolchain/tool versions pinned by this repo'srust-toolchain.tomland.pre-commit-config.yaml.Summary by CodeRabbit
New Features
Bug Fixes