fix(blob)!: preserve null selections across blob APIs#7903
Conversation
BREAKING CHANGE: Blob selection APIs now return one result per selector and represent null blob values explicitly. Rust take_blobs APIs return Vec<Option<BlobFile>>, read result payloads are Option<Bytes>, Python returns Optional values, and Java lists may contain null elements.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBlob selection and range-read APIs now preserve null entries in their original selection positions, distinguish null blobs from valid empty blobs, and expose optional results consistently across Rust, Python, and Java bindings with updated tests and documentation. ChangesNullable blob result pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Dataset
participant BlobSelection
participant BlobStorage
Caller->>Dataset: select blob rows or ranges
Dataset->>BlobSelection: collect selected rows and null positions
BlobSelection->>BlobStorage: read non-null payloads
BlobStorage-->>BlobSelection: payload results
BlobSelection-->>Dataset: merge payloads with None entries
Dataset-->>Caller: one result per row or request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
A-python
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
java/src/main/java/org/lance/Dataset.java-1628-1628 (1)
1628-1628: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a linked null-safe usage example.
Document
{@linkBlobFile}and show callers checking for null elements before reading or closing them.As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
Also applies to: 1646-1646
🤖 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 `@java/src/main/java/org/lance/Dataset.java` at line 1628, Update the public API documentation around the row-id blob retrieval method and its return description to link to BlobFile and include a usage example using the actual method signature; demonstrate checking each returned element for null before reading from or closing it.Source: Coding guidelines
docs/src/guide/blob.md-226-230 (1)
226-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSynchronize the table and examples with nullable results.
The new text says results can be
None, but the table still declaresList[Tuple[int, bytes]]andList[BlobFile]; thetake_blobsexample also dereferencesblobs[0]unconditionally. Use optional types and show a null check.As per coding guidelines, “Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.”
🤖 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 `@docs/src/guide/blob.md` around lines 226 - 230, Update the blob selection API table and examples to represent nullable results: change the relevant return types to optional element types, and revise the take_blobs example to check whether blobs[0] is None before dereferencing it. Keep the documented cardinality and distinction between null and empty blobs unchanged.Source: Coding guidelines
🧹 Nitpick comments (1)
rust/lance/src/dataset/blob.rs (1)
2190-2274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the selection bounds/duplicate-index validation.
collect_null_selection_valuesandinto_optional_blob_filesboth re-implement the same "check selection_index in bounds, error if slot already filled" pattern (andtotalize_blob_selection_streamrepeats a third variant of the bounds check). This is copy-pasted invariant-checking logic across three call sites with duplicated error message formatting. A shared helper (e.g.fn checked_slot_mut<T>(slots: &mut [T], selection_index: usize, selection_count: usize) -> Result<&mut T>) would remove the duplication and guarantee consistent error text.Separately, this new selection/null-tracking and stream-totalization logic (~150 lines:
collect_null_selection_values,totalize_blob_selection_stream, related helpers) adds a non-trivial scheduling/merging component inline into an already very large file (7500+ lines). As per coding guidelines, "Extract substantial new logic such as bin packing or scheduling into dedicated submodules instead of inlining it into large files." Moving this to a dedicatedblob/selection.rs(or similar) submodule would improve discoverability and keepblob.rsfocused.Also applies to: 2670-2690
🤖 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/dataset/blob.rs` around lines 2190 - 2274, Deduplicate selection-index validation across collect_null_selection_values, into_optional_blob_files, and totalize_blob_selection_stream by introducing and reusing a shared checked-slot helper with consistent bounds and duplicate errors. Move the selection/null-tracking and stream-totalization helpers into a dedicated blob selection submodule, updating blob.rs call sites and visibility/imports while preserving existing ordering and error behavior.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.
Other comments:
In `@docs/src/guide/blob.md`:
- Around line 226-230: Update the blob selection API table and examples to
represent nullable results: change the relevant return types to optional element
types, and revise the take_blobs example to check whether blobs[0] is None
before dereferencing it. Keep the documented cardinality and distinction between
null and empty blobs unchanged.
In `@java/src/main/java/org/lance/Dataset.java`:
- Line 1628: Update the public API documentation around the row-id blob
retrieval method and its return description to link to BlobFile and include a
usage example using the actual method signature; demonstrate checking each
returned element for null before reading from or closing it.
---
Nitpick comments:
In `@rust/lance/src/dataset/blob.rs`:
- Around line 2190-2274: Deduplicate selection-index validation across
collect_null_selection_values, into_optional_blob_files, and
totalize_blob_selection_stream by introducing and reusing a shared checked-slot
helper with consistent bounds and duplicate errors. Move the
selection/null-tracking and stream-totalization helpers into a dedicated blob
selection submodule, updating blob.rs call sites and visibility/imports while
preserving existing ordering and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: fe3f05fe-5449-4e15-9490-2502a14a9899
📒 Files selected for processing (12)
docs/src/guide/blob.mdjava/lance-jni/src/blocking_blob.rsjava/src/main/java/org/lance/Dataset.javajava/src/test/java/org/lance/DatasetTest.javajava/src/test/java/org/lance/TestUtils.javapython/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/tests/test_blob.pypython/src/dataset.rsrust/lance/src/dataset.rsrust/lance/src/dataset/blob.rsrust/lance/src/dataset/optimize.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 (2)
rust/lance/src/dataset.rs (2)
1737-1773: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd compiling examples for the changed
take_blobs*APIs.These breaking nullable-return semantics are documented, but
take_blobs,take_blobs_by_addresses, andtake_blobs_by_indicesstill lack usage examples demonstratingNoneversus a present zero-size blob.As per coding guidelines,
**/*.{rs,md,rst}: Document all public APIs with examples and links to relevant structs and methods; keep examples synchronized with actual signatures.🤖 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/dataset.rs` around lines 1737 - 1773, Add compiling documentation examples to the public methods take_blobs, take_blobs_by_addresses, and take_blobs_by_indices, using their current signatures and return types. Each example must demonstrate handling None for a null blob versus Some(BlobFile) with size zero for an empty blob, and include relevant links to BlobFile and the corresponding row-ID, row-address, or row-index method.Source: Coding guidelines
2929-2946: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression tests for positional filtering.
This fix changes the alignment contract by filtering
idsandaddressestogether and rejecting mismatched lengths. Cover live/deleted IDs, the no-index fallback, reordered or duplicate inputs, and the mismatch error—including both the error variant and count-bearing message.As per coding guidelines, every bugfix and feature must have corresponding tests, and tests should assert both the error variant and message content.
🤖 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/dataset.rs` around lines 2929 - 2946, The positional filtering change in the surrounding dataset method lacks regression coverage. Add tests covering live and deleted IDs, the no-index fallback, reordered and duplicate inputs, and mismatched ID/address lengths; for the mismatch case, assert both the expected error variant and a message containing the relevant counts, while verifying successful cases preserve positional alignment.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/dataset.rs`:
- Around line 1737-1773: Add compiling documentation examples to the public
methods take_blobs, take_blobs_by_addresses, and take_blobs_by_indices, using
their current signatures and return types. Each example must demonstrate
handling None for a null blob versus Some(BlobFile) with size zero for an empty
blob, and include relevant links to BlobFile and the corresponding row-ID,
row-address, or row-index method.
- Around line 2929-2946: The positional filtering change in the surrounding
dataset method lacks regression coverage. Add tests covering live and deleted
IDs, the no-index fallback, reordered and duplicate inputs, and mismatched
ID/address lengths; for the mismatch case, assert both the expected error
variant and a message containing the relevant counts, while verifying successful
cases preserve positional alignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8d26e396-aeb9-43d9-b955-a603e453126e
📒 Files selected for processing (1)
rust/lance/src/dataset.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
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/dataset.rs (1)
1756-1760: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPreserve the existing public Rust API or add a deprecation path.
Changing
take_blobs*return types toVec<Option<BlobFile>>is source-breaking for callers that treat the result as a non-option vector. Add nullable replacement APIs while retaining and deprecating the old methods, or document the sanctioned major-version exception and migration tests, per the public API rule.Also applies to: 1789-1793, 1816-1820
🤖 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/dataset.rs` around lines 1756 - 1760, Preserve the existing public return types of the `take_blobs` methods and add separate nullable replacement APIs returning `Vec<Option<BlobFile>>`; retain the original methods as deprecated wrappers that preserve their current non-optional behavior. Apply this consistently to all `take_blobs*` variants identified in the diff, and route shared logic through the new APIs without breaking existing callers.Source: Coding guidelines
🟡 Other comments (1)
java/src/main/java/org/lance/Dataset.java-1627-1635 (1)
1627-1635: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the
takeBlobsexample self-contained.
rowIdis never declared, so the copied example does not compile.Proposed fix
<pre>{`@code` +long rowId = 42L; List<BlobFile> blobs = dataset.takeBlobs(List.of(rowId), "images");🤖 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 `@java/src/main/java/org/lance/Dataset.java` around lines 1627 - 1635, Update the Javadoc example for Dataset.takeBlobs to declare and initialize rowId before passing it to takeBlobs, using a type and value consistent with the method’s expected row identifier so the example compiles as a standalone snippet.
🤖 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 `@java/src/main/java/org/lance/Dataset.java`:
- Around line 1638-1640: Update the takeBlobs documentation to identify rowIds
strictly as stable logical _rowid values, not physical _rowaddr addresses, and
clarify that callers needing addresses must use takeBlobsByAddresses. Add
JNI-side validation for rejected _rowaddr inputs if this binding currently
accepts them.
---
Outside diff comments:
In `@rust/lance/src/dataset.rs`:
- Around line 1756-1760: Preserve the existing public return types of the
`take_blobs` methods and add separate nullable replacement APIs returning
`Vec<Option<BlobFile>>`; retain the original methods as deprecated wrappers that
preserve their current non-optional behavior. Apply this consistently to all
`take_blobs*` variants identified in the diff, and route shared logic through
the new APIs without breaking existing callers.
---
Other comments:
In `@java/src/main/java/org/lance/Dataset.java`:
- Around line 1627-1635: Update the Javadoc example for Dataset.takeBlobs to
declare and initialize rowId before passing it to takeBlobs, using a type and
value consistent with the method’s expected row identifier so the example
compiles as a standalone snippet.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 74b9fc55-46cf-490f-930c-bbab28cc82b9
📒 Files selected for processing (3)
docs/src/guide/blob.mdjava/src/main/java/org/lance/Dataset.javarust/lance/src/dataset.rs
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
I like this behavior a lot better
2 participants
Why
Blob selection APIs currently expose an implementation detail of the physical read planner: null descriptors are omitted because they schedule no payload I/O. This makes result cardinality depend on descriptor contents, prevents callers from distinguishing null values from omitted rows, and leaves
read_blobs,read_blob_ranges, andtake_blobswith inconsistent contracts.This is a follow-up to #7864 and resolves the remaining null behavior described in #7899.
Contract
All blob selection APIs now return one logical result per selector or range request. Null blobs are represented explicitly, while valid empty blobs and empty ranges remain non-null empty values. The physical planner still avoids payload I/O for nulls; the logical result layer restores their selection positions.
Breaking change
This intentionally changes public return types and observable result cardinality:
take_blobs*APIs returnVec<Option<BlobFile>>, andReadBlob::data/ReadBlobRange::dataareOption<Bytes>.Optionalvalues for null blobs.takeBlobs*lists retain every requested position and may contain null elements.Callers that relied on null selections being omitted must now handle explicit null results.
BREAKING CHANGE: Blob selection APIs now preserve every selector or request and represent null blob values explicitly instead of omitting them.