Add merge insert external blob option#1
Conversation
Co-authored-by: Ragnor Comerford <ragnorc@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdae836d4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| batches, | ||
| new_fragments.clone(), | ||
| memory_size, | ||
| write_params.clone(), | ||
| ); |
There was a problem hiding this comment.
When allow_external_blob_outside_bases is enabled, the option is only forwarded on the insert/new-fragment branch, but matched-row updates in the update_fragments path still write via open_writer / Updater defaults. In merge-insert runs with a source subschema (the path that enters update_fragments), updating blob columns with external URIs still uses default blob validation and can reject URIs outside registered bases even though the caller explicitly enabled this option. This makes the new flag ineffective for a real merge-insert update scenario.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="python/python/tests/test_blob.py">
<violation number="1" location="python/python/tests/test_blob.py:553">
P2: This merge_insert call is missing a required action (for example `when_not_matched_insert_all()`), so `execute(source)` will raise `ValueError` instead of inserting the new row.</violation>
</file>
<file name="rust/lance/src/dataset/write/merge_insert.rs">
<violation number="1" location="rust/lance/src/dataset/write/merge_insert.rs:919">
P2: `allow_external_blob_outside_bases` is not applied on the matched-row update path, only on new-fragment inserts. Merge updates can still reject external blob URIs even when this flag is enabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
| source = pa.table({"id": [2], "blob": lance.blob_array([uri])}) | ||
| stats = ( | ||
| ds.merge_insert("id") | ||
| .allow_external_blob_outside_bases(True) |
There was a problem hiding this comment.
P2: This merge_insert call is missing a required action (for example when_not_matched_insert_all()), so execute(source) will raise ValueError instead of inserting the new row.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At python/python/tests/test_blob.py, line 553:
<comment>This merge_insert call is missing a required action (for example `when_not_matched_insert_all()`), so `execute(source)` will raise `ValueError` instead of inserting the new row.</comment>
<file context>
@@ -535,6 +535,33 @@ def test_blob_extension_write_external(tmp_path):
+ source = pa.table({"id": [2], "blob": lance.blob_array([uri])})
+ stats = (
+ ds.merge_insert("id")
+ .allow_external_blob_outside_bases(True)
+ .execute(source)
+ )
</file context>
| .allow_external_blob_outside_bases(True) | |
| .allow_external_blob_outside_bases(True) | |
| .when_not_matched_insert_all() |
Co-authored-by: Ragnor Comerford <ragnorc@users.noreply.github.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| stats = ( | ||
| ds.merge_insert("id") | ||
| .allow_external_blob_outside_bases(True) | ||
| .execute(source) |
There was a problem hiding this comment.
🔴 Python test fails because merge insert builder is not configured to do any work
The test test_blob_extension_merge_insert_external_outside_bases calls ds.merge_insert("id").allow_external_blob_outside_bases(True).execute(source) without ever calling .when_not_matched_insert_all(). The Python MergeInsertBuilder initializes both when_matched and when_not_matched to DoNothing (python/src/dataset.rs:214-216), which means insert_not_matched=false. When execute calls try_build() (rust/lance/src/dataset/write/merge_insert.rs:562-569), all three guard conditions (!insert_not_matched, when_matched == DoNothing, delete_not_matched_by_source == Keep) are true, so it returns an error: "The merge insert job is not configured to change the data in any way". The test will fail, leaving the feature without Python test coverage. Note: the Rust test test_merge_insert_allows_external_blobs_outside_bases does not have this problem because the Rust MergeInsertBuilder::try_new defaults insert_not_matched to true.
| stats = ( | |
| ds.merge_insert("id") | |
| .allow_external_blob_outside_bases(True) | |
| .execute(source) | |
| stats = ( | |
| ds.merge_insert("id") | |
| .when_not_matched_insert_all() | |
| .allow_external_blob_outside_bases(True) | |
| .execute(source) | |
| ) |
Was this helpful? React with 👍 or 👎 to provide feedback.
Co-authored-by: Ragnor Comerford <ragnorc@users.noreply.github.com>
…es (lance-format#6767) ## Summary After a writer flushed a memtable to L0 and an external compactor merged that generation into the base table — legitimately draining `flushed_generations` to empty — a subsequent restart re-replayed the original WAL entries into the new active memtable, duplicating rows on read. Two bugs were interacting: 1. **Disambiguation:** `replay_memtable_from_wal` distinguished "fresh shard" from "flushed and compacted" via `flushed_generations.is_empty()`. That works in a closed-world deployment but breaks the moment an external compactor enters the picture — and the compactor is the *intended* consumer that drains that vector, so the signal is structurally broken under OSS-WAL. 2. **Cursor never advanced:** `MemTableFlusher::flush` read `covered_wal_entry_position` from `memtable.last_flushed_wal_entry_position()`, but that field is only set by the `mark_wal_flushed` test helper. In production it stayed at 0, so `replay_after_wal_entry_position` never advanced past 0. Under 0-based WAL positions this masked bug #1 — both "fresh" and "post-flush-of-0" produced cursor=0. ## Fix - **WAL positions are now 1-based** (`FIRST_WAL_ENTRY_POSITION = 1`). A cursor of `0` unambiguously means "no flush has stamped this shard," so replay collapses to `cursor.saturating_add(1)` without consulting `flushed_generations`. - **`WalFlushHandler::handle`** writes the just-appended position back into `state.last_flushed_wal_entry_position` under the state lock before signalling the completion cell. - **`MemTableFlusher::flush` / `flush_with_indexes`** now take an explicit `covered_wal_entry_position` arg. The production caller derives it per-memtable from the `WalFlushResult` carried in the completion cell — authoritative under concurrent flushes — falling back to `memtable.frozen_at_wal_entry_position()` when freeze did not trigger a flush. - **State seed at open** uses the post-replay WAL tip, not `manifest.wal_entry_position_last_seen` (the latter is bumped on every tailer read and can sit above any flushed generation). - Proto field docs on `ShardManifest.replay_after_wal_entry_position` / `wal_entry_position_last_seen` updated to spell out the 1-based convention and what default-0 means. ## Test plan - [x] Added `test_memtable_replay_skips_entries_after_external_compaction` in `rust/lance/src/dataset/mem_wal/write.rs`: open writer, put rows, close (flush), simulate the compactor by directly committing a manifest with empty `flushed_generations`, reopen, assert the memtable is empty. Fails on the pre-fix code; passes now. - [x] `cargo test -p lance --lib dataset::mem_wal` — 236/236 pass - [x] `cargo test -p lance --lib` — 1600/1600 pass - [x] `cargo test -p lance-index --lib` — 302/302 pass - [x] `cargo clippy --all --tests --benches -- -D warnings` — clean - [x] `cargo fmt --all -- --check` — clean ## Compatibility WAL position numbering changes from 0-based to 1-based. Existing on-disk manifests / WAL files written by the prior `oss-wal-multiplex` code are not migrated — coordinated with downstream consumers (sophon) to start fresh. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="rust/lance/src/dataset/updater.rs">
<violation number="1">
P1: Updater writes now ignore `WriteParams` by using `open_writer` directly, so options like `allow_external_blob_outside_bases` are not applied during fragment rewrite paths.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…rvation (lance-format#7675) Raises the per-partition memory pool from 100MB to 150MB and sets the sort spill reservation to 40MB (up from the DataFusion default of 10MB) to give sort operations more headroom while spilling to disk (we should still spill at roughly the same rate). The previous defaults were 100MB / 10MB. This _usually_ worked but certain patterns would lead to false memory exhaustion errors: ``` OSError: LanceError(IO): Resources exhausted: Additional allocation failed for ExternalSorterMerge[0] with top memory consumers (across reservations) as: ExternalSorterMerge[0]#1(can spill: false) consumed 58.5 MB, peak 58.5 MB, ExternalSorter[0]#0(can spill: true) consumed 41.4 MB, peak 89.9 MB. Error: Failed to allocate additional 345.9 KB for ExternalSorterMerge[0] with 27.6 MB already allocated for this reservation - 92.2 KB remain available for the total pool, /home/pace/lance/rust/lance-datafusion/src/chunker.rs:49:46 ``` The problem happens as follows: 1. The sort node accumulates batches of data without modifying them until it determines a spill is needed. During this phase each batch counts double against the pool reservation. This is meant to provide overhead for the later steps. In our above example we can see spilling was triggered at 41.4MB which is about half of the 90MB pool (half, because each batch is counted double) 2. The sort node determines that spilling is needed. First, it must sort the data that has accumulated in memory. Each batch is sorted by itself. This batch sort is in-place and doesn't affect reservations much. 3. A cursor is created for each in-memory batch. The in-memory batches are then fed into a merge sort. 4. The merge sort accumulates batches of data to send to the spill. Once a batch is accumulated it is written to the spill file. Both the cursors and the accumulation require additional space. This is the `ExternalSorterMerge[0]` mentioned above. It is given the overcounting described in step 1. In other words, once this starts, we have half the reservation in `ExternalSorter[0]` and half the reservation in `ExternalSorterMerge[0]`. This "additional space" _should_ be about the same size as the input. This is why we count each batch twice. In practice, the `ExternalSorterMerge[0]` reservation ends up being slightly higher (for various reasons). This is what the `sort_spill_reservation_bytes` is supposed to account for. Datafusion defaults this to 10MB. There is no guidance (and I can't get Claude to come up with any good guidance) as to what this value should be set to. However, 10MB seems like too little. This PR updates it to about 1/3 of the memory pool size. In theory it shouldn't grow proportionally to the memory pool but in practice it seems to. I also really don't want to expose it as yet another knob that users have to tune so I'm hoping 1/3 is slightly conservative but good enough. --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
write_fragments_internaland direct writer creationTesting
cargo test -p lance external_blobscargo check --manifest-path python/Cargo.tomlcargo check --manifest-path java/lance-jni/Cargo.toml./mvnw spotless:check(fromjava/)Notes
protobuf-compilerlocally because the Rust build requiredprotoc.uvis not installed in this cloud image.Updaterhave a separate existing metadata limitation and intentionally remain on the existing writer path in this PR.Summary by cubic
Adds an opt-in flag for merge-insert to allow external blob URIs that don’t map to registered bases. Applies to new-fragment writes and full-fragment rewrites; row-level partial rewrites remain unchanged. Default behavior still rejects unmatched URIs.
MergeInsertBuilder.with_allow_external_blob_outside_bases(bool); plumbed viaWriteParamsand used by DataFusion exec and full-fragment rewrite writers throughopen_writer_with_write_params.MergeInsertBuilder.allow_external_blob_outside_bases(True|False)for opt-in.MergeInsertParams.withAllowExternalBlobOutsideBases(boolean)andallowExternalBlobOutsideBases().Written for commit 2e9e44b. Summary will update on new commits. Review in cubic