feat: support merge_insert with blob encoding column#7615
Conversation
nyl3532016
force-pushed
the
main
branch
2 times, most recently
from
34389ac to
28641eb
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@jackye1995 @wjones127 Hi, this pull request is similar to #5903. please help take a look. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional blob handling to ChangesBlob handling support
Sequence Diagram(s)sequenceDiagram
participant MergeInsertJob
participant LanceTableProvider
participant DatasetScan
MergeInsertJob->>LanceTableProvider: build ordered target scan
LanceTableProvider->>DatasetScan: set BlobHandling::AllBinary
DatasetScan-->>MergeInsertJob: provide blob-aware target rows for merge planning
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@rust/lance/src/datafusion/dataframe.rs`:
- Around line 80-83: Add a doc comment for the public builder method
DataFrame::with_blob_handling so it documents the semantic effect on blob column
materialization during scans, the valid BlobHandling values, and the impact on
the returned DataFrame; place the documentation directly above
with_blob_handling and make sure it satisfies the public API docs requirement,
including a brief usage example if appropriate.
- Line 180: The removal of read_lance_unordered from SessionContextExt is a
breaking public API change because the trait is exported through the crate root;
keep the method available for now and mark it deprecated instead of deleting it
outright. Update the SessionContextExt trait in dataframe.rs to preserve a
compatibility shim, and if needed route it to the new ordered/alternative path
so downstream callers can migrate before the method is removed in a major
release.
In `@rust/lance/src/dataset/write/merge_insert.rs`:
- Around line 1661-1670: The target scan in merge_insert currently forces
LanceTableProvider::new_with_ordering(...).with_blob_handling(BlobHandling::AllBinary),
which materializes every blob column into memory. Update this path so blob
materialization is not always forced during the join/rewrite, and instead only
enable AllBinary when it is strictly needed for the schema mismatch fix or make
it configurable/lazy via the merge_insert scan setup around
session_ctx.read_table(provider). Keep the ordering/provider behavior intact
while reducing memory pressure for blob-heavy datasets.
- Around line 11525-11528: The comment in the merge insert test path is stale
and should be updated to reflect the current successful behavior. In
merge_insert.rs, remove the outdated “will crash” wording near
execute_reader/new_reader, and inline the execute_reader(new_reader).await
unwrap instead of keeping the temporary result binding that only exists for the
comment. Keep the remaining comment forward-looking and aligned with the actual
semantics of job.execute_reader and the partial update flow.
🪄 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: 86948e8c-be7d-4ee3-a921-2a3d50e10568
📒 Files selected for processing (2)
rust/lance/src/datafusion/dataframe.rsrust/lance/src/dataset/write/merge_insert.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/datafusion/dataframe.rs (1)
45-45: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument
blob_handlingfield semantics.
blob_handling: Option<lance_core::datatypes::BlobHandling>has no doc comment explaining whatNonevsSome(...)means for scan behavior (defaults to dataset's own policy vs override).📝 Suggested doc
+ /// Overrides how blob columns are materialized during `scan`. + /// `None` uses the underlying dataset's default blob-handling policy. blob_handling: Option<lance_core::datatypes::BlobHandling>,As per coding guidelines: "Document the semantic meaning of both present and absent states for
Option<T>fields."🤖 Prompt for 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. In `@rust/lance/src/datafusion/dataframe.rs` at line 45, Add a doc comment for the `blob_handling` field in `DataFrame` explaining the meaning of both states: `None` means scans use the dataset’s configured blob handling policy, while `Some(...)` overrides that policy for the scan. Keep the comment near `blob_handling` so its semantics are clear alongside the `DataFrame` definition.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@rust/lance/src/datafusion/dataframe.rs`:
- Line 45: Add a doc comment for the `blob_handling` field in `DataFrame`
explaining the meaning of both states: `None` means scans use the dataset’s
configured blob handling policy, while `Some(...)` overrides that policy for the
scan. Keep the comment near `blob_handling` so its semantics are clear alongside
the `DataFrame` definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 717e49c7-edf4-4c97-a374-fb3cc7091c68
📒 Files selected for processing (1)
rust/lance/src/datafusion/dataframe.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/datafusion/dataframe.rs (1)
186-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
#[allow(dead_code)]hereread_lance_unorderedis a required trait signature, so the suppression is unnecessary; keep only#[deprecated(note = "use read_lance instead")].🤖 Prompt for 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. In `@rust/lance/src/datafusion/dataframe.rs` around lines 186 - 196, The `read_lance_unordered` trait method in `DataFrame` is a required signature, so the `#[allow(dead_code)]` suppression is unnecessary. Remove that attribute from the `read_lance_unordered` declaration and keep the existing `#[deprecated(note = "use read_lance instead")]` annotation unchanged.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@rust/lance/src/datafusion/dataframe.rs`:
- Around line 186-196: The `read_lance_unordered` trait method in `DataFrame` is
a required signature, so the `#[allow(dead_code)]` suppression is unnecessary.
Remove that attribute from the `read_lance_unordered` declaration and keep the
existing `#[deprecated(note = "use read_lance instead")]` annotation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7b1ceabd-2f9c-4c35-b45f-1166e94a1c5f
📒 Files selected for processing (1)
rust/lance/src/datafusion/dataframe.rs
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
The overall fix looks good. There's two things I'd like to see addressed before we merge.
| #[tokio::test] | ||
| async fn test_merge_insert_with_blob() { |
There was a problem hiding this comment.
suggestion: could we parametrize this by data_storage_version and also test V2_2? That contains the new blob V2, which I think we should try to cover in this test.
| #[allow(dead_code)] | ||
| #[deprecated(note = "use read_lance instead")] |
There was a problem hiding this comment.
issue(blocking): is this dead code? This is a private method, so if it's truly dead we should delete it.
|
1. Does reading the target with In Is that reading correct, or is there something limiting it that I'm missing? 2. Is blob v2 work as expected? Could the test be extended to |
3 participants
merge_insert directly invokes session_ctx.read_lance_unordered without specifying Blob column handling, which defaults to BlobsDescriptions. Consequently, DataFusion retrieves Blob data as a Struct type description when scanning the original dataset, causing a crash with "LargeBinary vs Struct schema mismatch!" because the table's logical schema expects LargeBinary.
Interestingly, this only happens if the input data for merge_insert lacks Blob columns; if Blob columns are present in the input, column pruning is triggered during the original data read, bypassing the Blob columns and masking the bug.
Summary by CodeRabbit
blob_handlingsetting on the table provider.