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)
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.
875a20f to
45eb4ca
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. Co-Authored-By: Claude <[email protected]>
45eb4ca to
31bfa72
Compare
1 participant
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