perf(encoding): avoid inline bitpacking input copy - #7696
Conversation
|
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:
📝 WalkthroughWalkthrough
ChangesInline bitpacking unchunk
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark
participant InlineBitpacking
participant BitPacking
Benchmark->>Benchmark: build typed inline chunk
Benchmark->>InlineBitpacking: decode typed-view chunk
InlineBitpacking->>BitPacking: unchecked_unpack payload
BitPacking-->>InlineBitpacking: fixed-width values
Benchmark->>Benchmark: compare with legacy decode
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/benches/decoder.rs`:
- Around line 66-67: The benchmark currently hardcodes a local bitpacking
chunk-size constant that duplicates InlineBitpacking’s chunk size. Update
decoder.rs to use the shared chunk-size value exported from lance_encoding
instead of BITPACKING_ELEMS_PER_CHUNK, and adjust the benchmark setup to
reference the existing InlineBitpacking/bitpacking constant so buffer generation
always matches the encoder’s expected chunking.
In `@rust/lance-encoding/src/encodings/physical/bitpacking.rs`:
- Around line 195-202: The inline header validation in
BitPacking::unchecked_unpack is currently using assert! on persisted input, so
replace it with an Error::corrupt_file path when the chunk size doesn’t match
the expected bit_width_value-derived length. In the bitpacking.rs decompression
flow, validate bit_width_value with checked_mul before computing the expected
byte count, then return a corrupt_file error on mismatch or overflow instead of
panicking, and only call BitPacking::unchecked_unpack after the header is safely
validated.
🪄 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: 03b8707a-c394-46b1-a718-1a0b56c5c126
📒 Files selected for processing (2)
rust/lance-encoding/benches/decoder.rsrust/lance-encoding/src/encodings/physical/bitpacking.rs
github-actions
Bot
added
A-python
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/encodings/physical/bitpacking.rs (1)
611-649: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the error message and parametrize the corruption cases.
assert_corrupt_unchunkonly matches on theCorruptFilevariant, so any of the fourunchunk_rejects_*tests would still pass even if a regression made it trip a different corruption branch than intended (e.g. the payload-mismatch case hitting the header/alignment guard). Assert on the message content too. The four cases also differ only by input, so they should berstestcases with#[case::{name}(...)].♻️ Suggested restructure
fn assert_corrupt_unchunk<T>(data: LanceBuffer, expected_msg: &str) where T: ArrowNativeType + BitPacking + Pod, { let err = InlineBitpacking::unchunk::<T>(data, 1).unwrap_err(); match err { lance_core::Error::CorruptFile { message, .. } => { assert!(message.contains(expected_msg), "unexpected message: {message}"); } other => panic!("expected CorruptFile, got {other:?}"), } } #[rstest] #[case::too_small_header(LanceBuffer::from(vec![1, 2, 3]), "too small")] #[case::misaligned_chunk_size(LanceBuffer::from(vec![0, 0, 0, 0, 0]), "multiple of")] #[case::payload_size_mismatch(LanceBuffer::reinterpret_vec(vec![12_u32]), "payload")] #[case::invalid_bit_width(LanceBuffer::reinterpret_vec(vec![33_u32]), "exceeds")] fn unchunk_rejects(#[case] data: LanceBuffer, #[case] expected_msg: &str) { assert_corrupt_unchunk::<u32>(data, expected_msg); }As per coding guidelines: "Assert on both the error variant and the message content in tests; do not check only
is_err()" and "Userstestfor Rust tests ... when cases differ only by inputs; use#[case::{name}(...)]for readable Rust case names".🤖 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/bitpacking.rs` around lines 611 - 649, The `assert_corrupt_unchunk` helper and the four `unchunk_rejects_*` tests only verify the `CorruptFile` variant, so regressions that hit the wrong corruption branch could still pass. Update `assert_corrupt_unchunk` to also validate the error message content, and refactor the four nearly identical `unchunk_rejects_*` tests in `InlineBitpacking::unchunk` into a single `rstest`-driven `unchunk_rejects` case table with readable `#[case::...]` names and an expected message per input.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/bitpacking.rs`:
- Around line 611-649: The `assert_corrupt_unchunk` helper and the four
`unchunk_rejects_*` tests only verify the `CorruptFile` variant, so regressions
that hit the wrong corruption branch could still pass. Update
`assert_corrupt_unchunk` to also validate the error message content, and
refactor the four nearly identical `unchunk_rejects_*` tests in
`InlineBitpacking::unchunk` into a single `rstest`-driven `unchunk_rejects` case
table with readable `#[case::...]` names and an expected message per input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 20218ced-df94-4c8a-8144-d770dd731da5
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
rust/lance-encoding/Cargo.tomlrust/lance-encoding/src/encodings/physical/bitpacking.rs
|
Addressed the outside-diff test feedback in The corrupt-input tests now use an Validated with:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Re Codecov remaining The remaining uncovered branch is the defensive overflow arm around: bit_width_value.checked_mul(ELEMS_PER_CHUNK as usize)That branch is not reachable for the current So the remaining Codecov warning is from defensive overflow handling / branch accounting, not an untested reachable corrupt-input path. Covering it would require constructing an impossible state or weakening the preceding validation. |
23027dd to
07e73c2
Compare
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/physical/bitpacking.rs`:
- Line 238: In the decompression initialization within the relevant bitpacking
function, replace T::from_usize(0).unwrap() with T::default() when creating the
decompressed vector, since ArrowNativeType guarantees Default.
🪄 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: eb276f80-b1f2-4371-8b9f-f3b2e58bab21
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rust/lance-encoding/Cargo.tomlrust/lance-encoding/benches/decoder.rsrust/lance-encoding/src/encodings/physical/bitpacking.rs
f96ca37 to
8a69471
Compare
The helper takes a logical/section name, not a filesystem path: the decoder only has an in-memory buffer and does not know its origin. Rename and document accordingly so the API surface stops implying a real path is being passed. Callers that have the real file path should continue to use Error::corrupt_file. Updates the inline-bitpacking call sites introduced in lance-format#7696.
…press The mini-block decompressor path already short-circuits num_values == 0 to avoid decoding a missing inline bit-width header, but the block-level decompressor path did not. After the validations added in lance-format#7696 (which convert asserts into CorruptFile errors), decoding an empty block through BlockDecompressor::decompress surfaced as a spurious "too small for N-byte header" corrupt-file error instead of returning an empty DataBlock. Add the same num_values == 0 short-circuit to the block-level path and share an empty_block() helper between the two entry points to avoid duplication. Regression test covers u8/u16/u32/u64 widths. Refs lance-format#7794.
|
Two follow-up commits pushed to address review feedback:
Verified: |
The helper takes a logical/section name, not a filesystem path: the decoder only has an in-memory buffer and does not know its origin. Rename and document accordingly so the API surface stops implying a real path is being passed. Callers that have the real file path should continue to use Error::corrupt_file. Updates the inline-bitpacking call sites introduced in lance-format#7696.
…press The mini-block decompressor path already short-circuits num_values == 0 to avoid decoding a missing inline bit-width header, but the block-level decompressor path did not. After the validations added in lance-format#7696 (which convert asserts into CorruptFile errors), decoding an empty block through BlockDecompressor::decompress surfaced as a spurious "too small for N-byte header" corrupt-file error instead of returning an empty DataBlock. Add the same num_values == 0 short-circuit to the block-level path and share an empty_block() helper between the two entry points to avoid duplication. Regression test covers u8/u16/u32/u64 widths. Refs lance-format#7794.
8499699 to
40b725b
Compare
The helper takes a logical/section name, not a filesystem path: the decoder only has an in-memory buffer and does not know its origin. Rename and document accordingly so the API surface stops implying a real path is being passed. Callers that have the real file path should continue to use Error::corrupt_file. Updates the inline-bitpacking call sites introduced in lance-format#7696.
…press The mini-block decompressor path already short-circuits num_values == 0 to avoid decoding a missing inline bit-width header, but the block-level decompressor path did not. After the validations added in lance-format#7696 (which convert asserts into CorruptFile errors), decoding an empty block through BlockDecompressor::decompress surfaced as a spurious "too small for N-byte header" corrupt-file error instead of returning an empty DataBlock. Add the same num_values == 0 short-circuit to the block-level path and share an empty_block() helper between the two entry points to avoid duplication. Regression test covers u8/u16/u32/u64 widths. Refs lance-format#7794.
The helper takes a logical/section name, not a filesystem path: the decoder only has an in-memory buffer and does not know its origin. Rename and document accordingly so the API surface stops implying a real path is being passed. Callers that have the real file path should continue to use Error::corrupt_file. Updates the inline-bitpacking call sites introduced in lance-format#7696.
…press The mini-block decompressor path already short-circuits num_values == 0 to avoid decoding a missing inline bit-width header, but the block-level decompressor path did not. After the validations added in lance-format#7696 (which convert asserts into CorruptFile errors), decoding an empty block through BlockDecompressor::decompress surfaced as a spurious "too small for N-byte header" corrupt-file error instead of returning an empty DataBlock. Add the same num_values == 0 short-circuit to the block-level path and share an empty_block() helper between the two entry points to avoid duplication. Regression test covers u8/u16/u32/u64 widths. Refs lance-format#7794.
31bfa72 to
31e242a
Compare
|
Hi @hamersaw , Thanks a lot! |
dde3636 to
93f9250
Compare
|
Hi, can you measure the end to end perf performance for this PR? |
93f9250 to
08a6dd9
Compare
|
Thanks for the suggestion. I measured this at two end-to-end scan layers:
Workload and procedure
Results
All confidence intervals cross zero, so I could not establish a statistically distinguishable end-to-end improvement or regression on this machine. Run-to-run CV was roughly 1%–4%, and individual paired results changed direction. I also temporarily instrumented a representative read to verify whether the new borrowed-buffer path was reached. The i32 case borrowed all 16,384 chunks. For u64, 5,463 chunks were borrowed and 10,921 fell back to an owned copy because of alignment. This helps explain why the improvement in the aligned These measurements are warm local-cache scans on WSL2 (Intel i7-10700). They verify that the optimized path is exercised, but do not demonstrate a broader cold-cache or remote-object-store improvement. |
94babb0 to
6729a5f
Compare
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This change seems fine to me. Just one question around why we are getting these empty chunks.
|
|
||
| impl BlockDecompressor for InlineBitpacking { | ||
| fn decompress(&self, data: LanceBuffer, num_values: u64) -> Result<DataBlock> { | ||
| if num_values == 0 { |
There was a problem hiding this comment.
Why are we getting empty chunks?
There was a problem hiding this comment.
We aren't, as far as I can tell — I don't know of any caller that reaches the block-level decompressor with num_values == 0. I checked by replacing the short-circuit with a panic! and running the lance-encoding test suite: the only failures were the synthetic unit tests added in this PR. Sparse round-trips, empty-batch reads, and all-null pages never hit it.
The guard is there to match the mini-block entry point, which does have a real trigger: #7752 added it because sparse pages that project to zero leaf values still decode an empty value buffer. When I converted the assert!s in unchunk into CorruptFile errors, the same empty input on the block side would have failed with a misleading "too small for N-byte header" corrupt-file error (it panicked before), so I filed #7794 and added the matching short-circuit. Note the issue text says this PR added the mini-block guard — that's wrong, it came from #7752.
It's one branch on a cold path and also covers 0-row pages and corrupt/legacy files, so I'd lean towards keeping it, but I can move it out of this PR if you prefer.
6729a5f to
0b75ff0
Compare
- Replace the unconditional input copy in InlineBitpacking::unchunk with a typed view borrowed over the inline bitpacking chunk words. Misaligned buffers still fall back to a copy, so the new path is never slower than the old one. - Validate chunk headers up front (header size, word alignment, value count, bit width, payload size) and reject malformed inline bitpacking chunks with Error::CorruptFile instead of asserting. - Add Error::corrupt_file_named so decoders that only have a logical section name (not a filesystem path) can report corrupt data without lance-encoding depending on object_store. - Short-circuit num_values == 0 in BlockDecompressor::decompress, sharing an empty_block() helper with the MiniBlockDecompressor path. Refs lance-format#7794. - Add unchunk roundtrip/corruption tests and a Criterion benchmark comparing the old copy path with the new typed-view path.
0b75ff0 to
deda929
Compare
3 participants
Summary
InlineBitpacking::unchunkby borrowing a typed view over the inline bitpacking chunk words.unchunkroundtrip coverage and a Criterion benchmark comparing the old copy path with the new typed-view path.Benchmark
Local machine: WSL2 on Intel(R) Core(TM) i7-10700 CPU @ 2.90GHz, 12 logical CPUs.
Command:
Results:
u32_bw12_1024legacy_copyu32_bw12_1024typed_viewu64_bw23_1024legacy_copyu64_bw23_1024typed_viewPath-to-path deltas:
u32_bw12_1024:typed_viewwas 13.4% faster thanlegacy_copyu64_bw23_1024:typed_viewwas 21.6% faster thanlegacy_copyThe benchmark uses aligned chunks built with
LanceBuffer::reinterpret_vec, so it measures the alignedborrow_to_typed_viewfast path. Misaligned buffers can still fall back to a copy.Test Plan
cargo fmt --allcargo test -p lance-encoding --features bitpackingcargo clippy -p lance-encoding --all-features --tests --benches -- -D warningscargo clippy --all --tests --benches -- -D warningscargo bench -p lance-encoding --bench decoder decode_inline_bitpacking_unchunk --features bitpacking -- --noplotSummary by CodeRabbit