Repository navigation
Conversation
Allow MergeInsertBuilder callers to configure the WriteParams used when merge operations create fragments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wjones127
left a comment
There was a problem hiding this comment.
Sorry for the late review. I agree we should add this feature, but I'm not sure if this API is the right direction as it doesn't align with the InsertBuilder API.
| /// Configure write parameters used when merge insert creates data fragments. | ||
| /// | ||
| /// This is useful for configuring Blob V2 writes, such as ingesting | ||
| /// external blob URIs into Lance-managed storage. | ||
| pub fn with_write_params(mut self, configure: impl FnOnce(WriteParams) -> WriteParams) -> Self { |
There was a problem hiding this comment.
question(blocking): why pass a closure instead of just a pointer to some parameters? That's what we do for InsertBuilder.
https://docs.rs/lance/latest/lance/dataset/write/struct.InsertBuilder.html#method.with_params
| #[derive(Debug, Clone)] | ||
| struct MergeWriteParams(Arc<WriteParams>); |
There was a problem hiding this comment.
question(blocking): what's the purpose of this wrapper? What would we lose if we dropped it?
WriteParams to MergeInsertBuilder
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The linked problem is valid, but builder-level write configuration must reach every supported merge-insert fragment writer. A viable revision should carry it through the indexed partial-schema fallback as well, or route that case to a writer that honors it, with a regression covering that path.
| /// | ||
| /// This is useful for configuring Blob V2 writes, such as ingesting | ||
| /// external blob URIs into Lance-managed storage. | ||
| pub fn with_write_params(mut self, configure: impl FnOnce(WriteParams) -> WriteParams) -> Self { |
There was a problem hiding this comment.
with_write_params is still ignored for indexed partial-schema inserts. Because use_index defaults to true, a scalar index on the join key makes can_use_create_plan fall back to update_fragments; its handle_new_fragments still passes Default::default() to write_fragments_internal. Callers therefore receive the same outside-base Blob error even after setting this flag. Carry the configured fragment-write options through this fallback, including its update writers, or ensure this supported case uses the configured full-row writer.
Reproducer
I extended the submitted test with a nullable extra target field, created a scalar index on id, omitted extra from the source schema, and inserted unmatched id = 2 with the same external Blob URI and allow_external_blob_outside_bases = true.
cargo test -p lance test_merge_insert_forwards_write_params_for_indexed_partial_schema_blobs -- --nocapture
Expected: the merge succeeds and the inserted blob reads inserted.
Observed on this head: the merge returns InvalidInput: External blob URI ... is outside registered external bases ... Set allow_external_blob_outside_bases=true.
|
Carried forward in #9888, rebased onto |
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
Per the continuation update, #9888 carries this work forward on main, including forwarding write parameters through the indexed partial-schema fallback. It is a separate, unmerged PR; this revision remains unchanged. If #7969 is still intended to land on release/v8.0, its write configuration must reach that fallback too, with regression coverage. Otherwise, the work can continue in #9888.
Fixes #6426.
Merge-insert currently creates data fragments with
WriteParams::default(), so callers cannot configure blob handling or any other write behavior.Add
MergeInsertBuilder::with_write_params, which accepts a callback over the current parameters and forwards the resulting configuration through both full-schema merge fragment-write paths.The regression test verifies that merge-insert forwards
allow_external_blob_outside_basesfor an external Blob V2 URI.Do note: I'm not entirely sure about the target branch here, but copilot figured this was the correct target to get these changes in a minor release :)