fix(encoding): honor LANCE_MINIBLOCK_MAX_VALUES in byte-stream-split - #9299
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
`ByteStreamSplitEncoder::max_chunk_size` returned a hardcoded 1024 for f32 and 512 for f64 and never consulted `MAX_MINIBLOCK_VALUES`, which `value.rs`, `binary.rs` and `rle.rs` all respect. The encoding docs describe `LANCE_MINIBLOCK_MAX_VALUES` as an upper bound on the values in a single mini-block chunk, with no exemption, and prescribe lowering it when mini-block read amplification saturates a constrained link. A user who profiled and set it below 1024 still got 1024-value chunks on exactly the float and timestamp columns BSS is chosen for. Files written that way read back correctly, so this is lost tuning rather than corruption; the default of 4096 is above the byte budget, so default behavior does not change. Take the smaller of the byte budget and the configured cap, rounded down to a power of two. That rounding is not cosmetic: `compress` records a non-final chunk's length as `chunk_size.ilog2()`, so a cap of, say, 300 would declare 256 values for a chunk holding 300. The floor is two, because a non-final chunk carrying `log_num_values == 0` is rejected on read. ## Testing `cargo test --release -p lance-encoding --all-features` (1832 passed), plus `cargo fmt` and clippy with `-D warnings`. The cap arithmetic is split into a pure helper so it can be exercised without the process-wide lazy static; ignoring the cap fails the three cases that set it below the byte budget. A second test covers multi-chunk output, which had none: the existing round trips fit in a single chunk, so nothing checked a non-final chunk's declared length.
github-actions
Bot
added
A-encoding
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The encoder now applies the configured cap while preserving power-of-two non-final chunk metadata, unchanged default chunk sizes, and the existing file-format contract. The focused coverage exercises both cap selection and multi-chunk framing.
lance-gatekeeper
Bot
added
the
K-approved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ByteStreamSplitEncoder::max_chunk_sizehardcoded 1024 for f32 and 512 for f64 and never consultedMAX_MINIBLOCK_VALUES, which the other mini-block encoders respect. The docs describeLANCE_MINIBLOCK_MAX_VALUESas an upper bound on the values in a single mini-block chunk, so a user who lowered it below 1024 still got 1024-value chunks on BSS-encoded columns. Such files read back correctly -- lost tuning, not corruption -- and the default 4096 exceeds the byte budget, so default behavior is unchanged.Take the smaller of the byte budget and the configured cap, rounded down to a power of two. The rounding matters:
compressrecords a non-final chunk's length aschunk_size.ilog2(), so a cap of 300 would declare 256 for a chunk of 300. The floor is two, sincelog_num_values == 0there is rejected on read.Testing
cargo test --release -p lance-encoding --all-features(1832 passed), plus fmt and clippy with-D warnings. The cap arithmetic moved into a pure helper, testable without the lazy static; ignoring the cap fails the three cases below the byte budget. A second test covers multi-chunk output.