feat: per value support for fixed-length packed structs - #7714
Conversation
github-actions
Bot
added
A-encoding
b6636bf to
8a991b1
Compare
|
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:
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)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds fixed-per-value packed-struct encoding and decoding, refactors shared packed-field handling, wires the new compression path into strategy selection, and adds direct, full-zip, version-gating, and rejection tests. ChangesPacked-Struct Compression
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CompressionStrategy
participant PackedStructFixedPerValueEncoder
participant FixedWidthDataBlock
participant DecompressionStrategy
participant PackedStructFixedPerValueDecompressor
CompressionStrategy->>PackedStructFixedPerValueEncoder: select fixed packed-struct encoding
PackedStructFixedPerValueEncoder->>FixedWidthDataBlock: write row-major packed bytes and metadata
DecompressionStrategy->>PackedStructFixedPerValueDecompressor: create PackedStruct decoder
PackedStructFixedPerValueDecompressor->>PackedStructFixedPerValueDecompressor: unzip rows into child buffers
PackedStructFixedPerValueDecompressor-->>DecompressionStrategy: return reconstructed struct
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
morales-t-netflix
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/physical/packed.rs`:
- Around line 190-197: Move the byte-alignment check and byte-width calculation
out of `append_row_bytes` and into `FixedPackedFieldData` construction so they
are validated once instead of per row. Update the
`FixedPackedFieldData`/`Packed` setup to reject non-byte-aligned
`bits_per_value` up front, store `bytes_per_value` on the struct, and have
`append_row_bytes` use the cached value without repeating the invariant check.
- Around line 440-462: The packed-width arithmetic in the fixed struct path can
overflow or wrap when accumulating child widths or sizing buffers. In the
`packed.rs` flow around `compress`/row allocation, replace `bits_per_row +=` and
any metadata-derived size math with `checked_add` and `checked_mul`, and reject
invalid totals as corrupted data rather than continuing. Store the validated
row-width in the decompressor/decoder state, and make the related paths at the
referenced `Packed*` helpers use the same checked arithmetic and
`Error::corrupt_file` reporting with the relevant sizes/values included.
- Around line 869-875: The decompressor type is exposing a public API surface
unnecessarily; make PackedStructFixedPerValueDecompressor crate-private to match
its pub(crate) constructor and internal-only usage. Update the struct
declaration in packed.rs so the type itself is no longer public, while keeping
PackedStructFixedPerValueDecompressor::new(crate) available for internal
callers.
- Around line 1554-1559: The test for PackedStructFixedPerValueEncoder::compress
only checks the error message text, so it can pass even if the wrong error
category is returned. Update the assertion to verify both the specific error
variant produced by compress and that its message still contains the expected
“fixed-width” text, using the existing encoder/compress/unwarp_err setup in this
test.
- Around line 198-208: The packed struct child slice in packed.rs computes the
end index with unchecked addition, which can wrap and bypass the bounds check.
Update the logic around the `Packed` row extraction path to use checked addition
for `end` (mirroring the existing `checked_mul` on `row_idx`) and return
`Error::invalid_input` on overflow before slicing `self.block.data`; keep the
existing `start`, `data`, and `output.extend_from_slice` flow in the same
method.
- Around line 414-422: The PackedStructFixedPerValueEncoder type is exposed as
public API even though it is only used internally by the compression strategy.
Change both the struct and its new constructor to crate-visible access in
PackedStructFixedPerValueEncoder so they remain internal to the crate, and keep
the encoder construction limited to the strategy that creates it.
🪄 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: f958ff30-04b5-409c-9387-116180210930
📒 Files selected for processing (3)
rust/lance-encoding/src/compression.rsrust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/encodings/physical/packed.rs
💤 Files with no reviewable changes (1)
- rust/lance-encoding/src/encodings/logical/primitive.rs
| fn append_row_bytes(&self, row_idx: usize, output: &mut Vec<u8>) -> Result<()> { | ||
| let bits_per_value = self.block.bits_per_value; | ||
| if !bits_per_value.is_multiple_of(8) { | ||
| return Err(Error::invalid_input( | ||
| "Packed struct encoding requires byte-aligned fixed-width children", | ||
| )); | ||
| } | ||
| let bytes_per_value = (bits_per_value / 8) as usize; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Hoist byte-alignment validation out of the row loop.
This method runs once per field per row, but alignment and byte width never change. Validate them when constructing FixedPackedFieldData and store bytes_per_value.
As per coding guidelines, “In performance-critical lance-encoding encoding/decoding paths, hoist loop-invariant conditionals out of hot loops.”
🤖 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-encoding/src/encodings/physical/packed.rs` around lines 190 - 197,
Move the byte-alignment check and byte-width calculation out of
`append_row_bytes` and into `FixedPackedFieldData` construction so they are
validated once instead of per row. Update the `FixedPackedFieldData`/`Packed`
setup to reject non-byte-aligned `bits_per_value` up front, store
`bytes_per_value` on the struct, and have `append_row_bytes` use the cached
value without repeating the invariant check.
Source: Coding guidelines
There was a problem hiding this comment.
Added all of AI's suggestions except this one. I think philosophically, the above sounds right to me, but this isn't the behavior of the code previously - so would be interested in input from a maintainer
There was a problem hiding this comment.
@morales-t-netflix, that's a fair point to flag for maintainer input. A couple of things that might help frame the discussion:
FixedPackedFieldDatais new in this PR, so there isn't really "previous behavior" tied to this struct specifically — the per-call check pattern was likely copied for symmetry withVariablePackedFieldData::append_row_bytes, where the equivalent checks (bits_per_lengthalignment, 32/64-bit prefix) genuinely can't be hoisted because they gate amatchonblock.bits_per_offset, which is inspected fresh each call.- For the fixed case, though,
bits_per_valueis fixed at construction (self.block.bits_per_valuenever changes across rows), so hoisting is behavior-preserving in the sense that the same error is still returned for invalid input — it would just surface once duringFixedPackedFieldDataconstruction instead of on the firstappend_row_bytescall. No valid input path changes.
That said, I'll defer to the maintainer's call here — if there's a reason to keep validation lazy/per-call (e.g., to match error-reporting expectations elsewhere in the encoder), that's a legitimate tradeoff to weigh against the minor per-row overhead.
(๐•ᴗ•๐) still thinking this one over alongside you
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
The fixed full-zip path makes sense for wide packed structs. My main concern is the file-version contract: create_per_value enables this fixed PackedStruct full-zip representation for V2.1 and V2.2, while existing readers for those stable versions only recognize PackedStruct in the mini-block dispatcher. For example, the released v9.0.0-beta.19 reader falls through to the fixed-per-value todo! for this metadata, so a file written as V2.2 by this PR cannot be read after rolling back to that released reader.
Could we only enable writing this combination for V2.3+ and keep V2.1/V2.2 returning NotSupported? It would also be worth covering V2.1/V2.2 rejection and V2.3 round-trip in the versioned tests. The reader side can remain permissive.
Fair point! Went ahead and switched this to be V2.3 only - and also added a test to verify failure pre V2.3 (and updated the new tests to point to V2.3 for round trip conversion and all that jazz) |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rust/lance-encoding/src/encodings/physical/packed.rs (4)
294-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject or preserve struct validity before packing.
check_struct_validityacceptsStructDataBlockvalues with a validity bitmap, but the packed representation and decompressor returnvalidity: None. Nullable structs can therefore round-trip with their null mask silently discarded.🤖 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-encoding/src/encodings/physical/packed.rs` around lines 294 - 320, Update check_struct_validity to reject StructDataBlock inputs with a non-None validity bitmap before packing, since packed structs cannot preserve it. Return an appropriate invalid-input error while retaining the existing child-count and value-count validation for structs without validity.Source: Coding guidelines
246-282: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject descending offsets before subtraction and slicing.
Both branches need to validate
start <= end(and the next offset index) before computingend - start; malformed offsets can otherwise panic instead of returning a validation error.As per coding guidelines, validate API-boundary inputs and include row, start, end, and buffer-size context in the error.
🤖 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-encoding/src/encodings/physical/packed.rs` around lines 246 - 282, Update both 32-bit and 64-bit branches in the packed variable-child decoding logic to validate the next offset index and require start <= end before subtracting or slicing. Return an invalid-input error for malformed offsets, including row, start, end, and buffer length context, while preserving the existing bounds validation for end.Source: Coding guidelines
692-774: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake packed-row cursor arithmetic fallible.
cursor + ...andcursor + lencan overflow before the bounds checks, andtry_into().expect(...)can panic while decoding malformed data. Usechecked_addand fallible byte conversion; report persisted-format failures as corruption errors.As per coding guidelines, library decoding must not use
expect()for fallible operations and must use checked arithmetic with contextual errors.🤖 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-encoding/src/encodings/physical/packed.rs` around lines 692 - 774, Update the packed-row decoding logic around the FieldAccumulator fixed and variable branches to use checked_add for every cursor/end and cursor/length calculation, returning contextual corruption errors on overflow before slicing or bounds checks. Replace the u32/u64 byte-slice try_into().expect(...) conversions with fallible handling that reports persisted-format corruption errors, and apply the same safeguards to both Variable32 and Variable64 paths.Source: Coding guidelines
467-474: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate child alignment before the row loop.
For an empty struct block,
append_row_bytesis never called, so non-byte-aligned children bypass validation whilebits_per_row / 8truncates the width. This can emit metadata that the decoder rejects.🤖 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-encoding/src/encodings/physical/packed.rs` around lines 467 - 474, Validate every child’s byte alignment before entering the row loop in the surrounding encoding function, including when num_values is zero. Reuse the same alignment condition enforced by append_row_bytes, return the existing validation error for any non-byte-aligned child, and only then compute bytes_per_row and append row data.Source: Coding guidelines
🟡 Other comments (1)
rust/lance-encoding/src/encodings/physical/packed.rs-1574-1607 (1)
1574-1607: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the version-gating error message too.
The test should verify both
Error::NotSupportedand that the message identifies the Lance V2.3 requirement.As per coding guidelines, tests must 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-encoding/src/encodings/physical/packed.rs` around lines 1574 - 1607, Update fixed_per_value_packed_struct_requires_v23 to inspect the returned Error::NotSupported value and assert its message states that Lance V2.3 is required, while preserving the existing variant assertion.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-encoding/src/encodings/physical/packed.rs`:
- Around line 294-320: Update check_struct_validity to reject StructDataBlock
inputs with a non-None validity bitmap before packing, since packed structs
cannot preserve it. Return an appropriate invalid-input error while retaining
the existing child-count and value-count validation for structs without
validity.
- Around line 246-282: Update both 32-bit and 64-bit branches in the packed
variable-child decoding logic to validate the next offset index and require
start <= end before subtracting or slicing. Return an invalid-input error for
malformed offsets, including row, start, end, and buffer length context, while
preserving the existing bounds validation for end.
- Around line 692-774: Update the packed-row decoding logic around the
FieldAccumulator fixed and variable branches to use checked_add for every
cursor/end and cursor/length calculation, returning contextual corruption errors
on overflow before slicing or bounds checks. Replace the u32/u64 byte-slice
try_into().expect(...) conversions with fallible handling that reports
persisted-format corruption errors, and apply the same safeguards to both
Variable32 and Variable64 paths.
- Around line 467-474: Validate every child’s byte alignment before entering the
row loop in the surrounding encoding function, including when num_values is
zero. Reuse the same alignment condition enforced by append_row_bytes, return
the existing validation error for any non-byte-aligned child, and only then
compute bytes_per_row and append row data.
---
Other comments:
In `@rust/lance-encoding/src/encodings/physical/packed.rs`:
- Around line 1574-1607: Update fixed_per_value_packed_struct_requires_v23 to
inspect the returned Error::NotSupported value and assert its message states
that Lance V2.3 is required, while preserving the existing variant assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 4d133d86-f09a-43a8-b4df-c895487d70b9
📒 Files selected for processing (2)
rust/lance-encoding/src/compression.rsrust/lance-encoding/src/encodings/physical/packed.rs
|
Updated! Seeing tests fail - but think this is because main has this issue - seeing this across many other PRs :sweat-smile: |
|
Howdy @Xuanwo! Know there was some innovation in the enocder space through this stack of PRs. Wanted to check your thoughts on me adding onto that, allowing this encoding in V2.3? (Pending feedback - will integrate those changes, and fix the build) |
|
Hi, @morales-t-netflix, Sorry for the late. Now this PR has been merged, feel free to just rebase. Thank you! |
5129a81 to
8b18afc
Compare
|
Thanks @Xuanwo! Just rebased and tests are 🟢 |
|
Howdy! Wanted to give this one a friendly bump, as this would be very helpful for some work in my world 😄. Thank you again for all your help! @Xuanwo |
|
Another friendly bump on this one 😄 |
# Conflicts: # rust/lance-encoding/src/compression.rs # rust/lance-encoding/src/encodings/physical/packed.rs
8b18afc to
022e013
Compare
3 participants
Adds #5021
In line with the suggestion from this issue, this added support for full-zip encoding for fixed-length structs.
Please note - this is a change from current behavior (per-value Fixed-length packed structs would error out prior to this). Please let me know if that warrants marking this feat as a breaking change.
Will note - I'm fairly new to rust, so any and all feedback is appreciated 😉