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. |
u70b3
force-pushed
the
perf/inline-bitpack-unchunk-zero-copy
branch
from
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
u70b3
force-pushed
the
perf/inline-bitpack-unchunk-zero-copy
branch
4 times, most recently
from
ffa6392 to
5c03730
Compare
- Replace the unconditional input copy in InlineBitpacking::unchunk with a typed view borrowed over the inline bitpacking chunk words. - Validate chunk headers up front and reject malformed inline bitpacking chunks with Error::CorruptFile. - Use lance_core::Error::corrupt_file_at_path so lance-encoding does not need a direct object_store dependency. - Share ELEMS_PER_CHUNK with the benchmark instead of a hard-coded constant. - Keep output zero-initialization unchanged.
u70b3
force-pushed
the
perf/inline-bitpack-unchunk-zero-copy
branch
from
5c03730 to
f96ca37
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