feat: compress RLE child buffers#7663
Conversation
github-actions
Bot
added
A-encoding
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
BubbleCal
left a comment
BubbleCal
left a comment
There was a problem hiding this comment.
Two concerns from review.
|
|
||
| if rle_bytes < raw_bytes { | ||
| #[cfg(feature = "bitpacking")] | ||
| let use_child_encodings = version.resolve() >= LanceFileVersion::V2_2; |
There was a problem hiding this comment.
This enables new RLE child encodings for V2.2, but released V2.2 readers only accept flat RLE children. Files written this way may be unreadable by older V2.2 readers, so this likely needs a new format-version gate.
📝 WalkthroughWalkthroughAdds support for secondary (child) compression of RLE values and run-lengths buffers in the Lance encoding module. Introduces candidate generation for flat, general, and bitpacked encodings, version-gated selection logic (V2.3+), new decompressor abstractions to reconstruct compressed child buffers, and expanded validation and tests. ChangesRLE child buffer secondary compression
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-encoding/src/compression.rs (1)
1187-1338: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPrefer
Error::corrupt_filefor malformed persisted encodings on the read path.
create_rle_decompressor,create_rle_child_decompressor,validate_rle_child_compression, andvalidate_rle_block_child_innerare reading encodings deserialized from stored files. Missing sub-encodings (e.g. missing values/run-lengths/inner compression) and unsupported nested child combinations are format/integrity problems, not caller-supplied input, soError::corrupt_fileclassifies the root cause more accurately and helps operators distinguish corruption from API misuse. Note the surrounding code already usesinvalid_inputfor similar cases, so consider whether to align both.As per coding guidelines: "Match the
Errorvariant to the root cause: useError::invalid_inputfor caller data issues,Error::corrupt_filefor format or integrity issues".🤖 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/compression.rs` around lines 1187 - 1338, The RLE read-path validation in create_rle_decompressor, create_rle_child_decompressor, validate_rle_child_compression, and validate_rle_block_child_inner is treating malformed persisted encodings as invalid input; switch the missing child encoding/compression and unsupported nested-combination error returns to Error::corrupt_file so these deserialization integrity issues are classified correctly. Keep the existing function structure and update the relevant Error::invalid_input calls in these helpers to use the corruption variant where the data comes from stored file metadata rather than caller-provided arguments.Source: Coding guidelines
🧹 Nitpick comments (2)
rust/lance-encoding/src/encodings/physical/rle.rs (2)
902-932: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid bare
.unwrap()in library code.Inside
required_bits, thevalue.try_into().unwrap()calls are in non-test library code. Althoughchunks_exact(N)guarantees the slice length so the conversion is infallible in practice, the project guideline prohibits bare.unwrap()here.♻️ Use `.expect(...)` (or destructure the array) to document the invariant
16 => buffer .as_ref() .chunks_exact(2) - .map(|value| u16::from_le_bytes(value.try_into().unwrap()) as u64) + .map(|value| { + u16::from_le_bytes( + value.try_into().expect("chunks_exact(2) yields 2-byte slices"), + ) as u64 + }) .max(),As per coding guidelines: "Never use
.unwrap()... in library code for fallible operations" and "If unavoidable, use.expect("reason")."🤖 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/rle.rs` around lines 902 - 932, The `required_bits` helper in `rle.rs` uses bare `unwrap()` on `value.try_into()` in the 16/32/64-bit `chunks_exact` branches, which violates the library code guideline. Replace those `unwrap()` calls with an explicit invariant-documenting approach, such as `expect(...)` with a clear message or destructuring into fixed-size arrays, while keeping the logic inside `required_bits` unchanged.Source: Coding guidelines
994-1020: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid recomputing RLE child candidates on the write path
try_rle_for_mini_blockcallsselected_payload_sizebefore returning the RLE compressor, socompress()repeatsencode_data()+child_candidates()for the same block. With child compression enabled, that doubles the expensive RLE/LZ4/Zstd work. Cache the selected candidates or encoded buffers during the estimate and reuse them incompress()instead of recomputing.🤖 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/rle.rs` around lines 994 - 1020, selected_payload_size currently recomputes encode_data and child_candidates for the same block that try_rle_for_mini_block has already inspected, causing duplicate work on the write path. Update the RLE flow in rle.rs so the result of the sizing/selection step is cached and reused by compress()—for example by threading the encoded buffers or selected child candidates through the compressor state—rather than calling encode_data() and child_candidates() again in selected_payload_size and compress().
🤖 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/compression.rs`:
- Around line 1187-1338: The RLE read-path validation in
create_rle_decompressor, create_rle_child_decompressor,
validate_rle_child_compression, and validate_rle_block_child_inner is treating
malformed persisted encodings as invalid input; switch the missing child
encoding/compression and unsupported nested-combination error returns to
Error::corrupt_file so these deserialization integrity issues are classified
correctly. Keep the existing function structure and update the relevant
Error::invalid_input calls in these helpers to use the corruption variant where
the data comes from stored file metadata rather than caller-provided arguments.
---
Nitpick comments:
In `@rust/lance-encoding/src/encodings/physical/rle.rs`:
- Around line 902-932: The `required_bits` helper in `rle.rs` uses bare
`unwrap()` on `value.try_into()` in the 16/32/64-bit `chunks_exact` branches,
which violates the library code guideline. Replace those `unwrap()` calls with
an explicit invariant-documenting approach, such as `expect(...)` with a clear
message or destructuring into fixed-size arrays, while keeping the logic inside
`required_bits` unchanged.
- Around line 994-1020: selected_payload_size currently recomputes encode_data
and child_candidates for the same block that try_rle_for_mini_block has already
inspected, causing duplicate work on the write path. Update the RLE flow in
rle.rs so the result of the sizing/selection step is cached and reused by
compress()—for example by threading the encoded buffers or selected child
candidates through the compressor state—rather than calling encode_data() and
child_candidates() again in selected_payload_size and compress().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: afccec72-953c-4004-a5c8-6a4880e10bd0
📒 Files selected for processing (2)
rust/lance-encoding/src/compression.rsrust/lance-encoding/src/encodings/physical/rle.rs
2 participants
Fixes #7329.
RLE miniblock compression currently stores run values and run lengths as raw
Flatchild buffers, so repeated or low-entropy child buffers can dominate the encoded page even after RLE chooses a better shape than raw fixed-width data.This change lets RLE child buffers choose the smallest valid representation among
Flat,OutOfLineBitpacking(Flat), andGeneral(Flat)when a general compression scheme is configured. The decoder validates the supported nested child encodings and preserves the existing flat path, while rejecting combinations where both children require the run count.The strategy automatically enables RLE child bitpacking and reuses explicit field compression settings for LZ4/Zstd child candidates, so configured general compression can reduce the values and run-length payload without adding a new public compression knob.
Performance data from a local release microbenchmark is below. Payload bytes exclude metadata;
shapeisvalues/run_lengths.flat/flat)flat/flat)flat/flat)bitpack/flat)general/general)general/general)bitpack/flat)bitpack/general)bitpack/general)flat/bitpack)flat/general)flat/general)flat/flat)flat/flat)flat/flat)The encoder falls back to
Flatwhen a child encoding does not shrink the payload. In the high-benefit cases, child bitpacking keeps encode/decode time in the same order of magnitude as plain RLE, while LZ4/Zstd trade more CPU for substantially smaller payloads. For example, short runs with small values measured about 512/237 us encode/decode for plain RLE, 517/248 us with child bitpacking, 585/265 us with LZ4, and 633/294 us with Zstd.Summary by CodeRabbit
New Features
Bug Fixes