fix: round-trip empty blob values in the 2.1+ structural encoding#7717
Merged
wkalt merged 2 commits intoJul 10, 2026
Merged
Conversation
A valid empty value is stored as {position: 0, size: 0} while nulls pack
their non-zero rep/def levels into position. The page scheduler skips
size-0 rows when scheduling reads, but the load task assigned read
results to every def == 0 blob, so an empty value consumed the next
blob's payload and the last consumer crashed on an exhausted iterator
("Expected option to have value"). Give empty values their zero-length
bytes at scheduling time and only assign read results to blobs that
scheduled a read.
Fixes lance-format#7716
github-actions
Bot
added
A-encoding
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughBlob page scheduling now preserves zero-length values as loaded empty bytes instead of treating them as unread data. A Tokio test covers round-tripping empty, non-empty, and null ChangesEmpty blob value handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-encoding/src/encodings/logical/blob.rs`:
- Around line 561-564: Extend the regression test using
check_round_trip_encoding_of_data to cover LanceFileVersion::V2_2 in addition to
V2_1, either by raising the configured maximum file version or adding an
explicit V2.2 test case.
🪄 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: e06feede-1187-47bb-83d2-1bc7cb938dce
📒 Files selected for processing (2)
rust/lance-encoding/src/encodings/logical/blob.rsrust/lance-encoding/src/encodings/logical/primitive/blob.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
wkalt
force-pushed
the
ticket/oss-1429/blob-empty-value-read
branch
from
July 9, 2026 23:50
3e2f1e9 to
5f33e1b
Compare
The uncapped TestCases default iterates every version iter_non_legacy() yields, so new file versions are covered automatically as they are added.
wkalt
force-pushed
the
ticket/oss-1429/blob-empty-value-read
branch
from
July 9, 2026 23:50
5f33e1b to
17c4b06
Compare
xuanyu-z
approved these changes
Jul 10, 2026
LuQQiu
approved these changes
Jul 10, 2026
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
3 participants
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.
A valid empty value is stored as {position: 0, size: 0} while nulls pack their non-zero rep/def levels into position. The page scheduler skips size-0 rows when scheduling reads, but the load task assigned read results to every def == 0 blob, so an empty value consumed the next blob's payload and the last consumer crashed on an exhausted iterator ("Expected option to have value"). Give empty values their zero-length bytes at scheduling time and only assign read results to blobs that scheduled a read.
Fixes #7716
Summary by CodeRabbit
LargeBinary) values so they’re preserved and remain correctly aligned when mixed with non-empty payloads and nulls.LargeBinary) values interleaved with nulls when blob metadata is enabled.