feat: per value support for fixed-length packed structs#7714
feat: per value support for fixed-length packed structs#7714morales-t-netflix wants to merge 3 commits into
Conversation
morales-t-netflix
force-pushed
the
tmorales/compression_fixed_length_structs
branch
from
b6636bf to
8a991b1
Compare
|
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)
📝 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, and rejection tests. ChangesPacked-Struct Compression
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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! |
1 participant
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). 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 😉
Summary by CodeRabbit