fix(encoding): tolerate legacy RLE chunks that overflow the declared value count#7708
Open
jackye1995 wants to merge 3 commits into
Open
fix(encoding): tolerate legacy RLE chunks that overflow the declared value count#7708jackye1995 wants to merge 3 commits into
jackye1995 wants to merge 3 commits into
Conversation
…value count The legacy miniblock RLE encoder rolled back to a power-of-2 checkpoint after a run had already crossed it, producing chunks whose run lengths sum past the declared value count; the legacy decoder truncated the excess. Restore that truncating behavior so those files stay readable, while still rejecting underflow and zero run lengths.
github-actions
Bot
added
A-encoding
📝 WalkthroughWalkthroughThe RLE decoder now clamps miniblock overflow to the expected value count, while block decoding still errors on overflow. The miniblock tests were updated for successful clamped decoding, and a regression test was added for legacy chunk-boundary overflow. ChangesRLE decoder clamping
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
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/rle.rs (1)
1459-1478: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject zero-length runs before ignoring trailing legacy overflow.
Line 1460 exits before reading remaining run lengths, so
[5, 0]withexpected_value_count = 5now succeeds and silently ignores the zero run. That conflicts with the PR contract that zero run lengths are still rejected.Proposed fix
let mut decoded: Vec<T> = Vec::with_capacity(expected_value_count); for (value, length_bytes) in values.iter().zip(lengths.chunks_exact(length_size)) { - if decoded.len() == expected_value_count { - break; - } let length = self.run_length_width.read_length(length_bytes); if length == 0 { return Err(Error::invalid_input_source( "RLE decoding encountered a zero run length".into(), )); } + if decoded.len() == expected_value_count { + continue; + } let length = usize::try_from(length).map_err(|_| { Error::invalid_input_source( format!("RLE run length does not fit in usize: {length}").into(), ) })?;🤖 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 1459 - 1478, The RLE decoder in the `decode_runs` logic is returning early once `decoded.len()` reaches `expected_value_count`, which allows a trailing zero run length to be skipped instead of rejected. Update the loop to keep validating remaining run lengths even after the expected output has been filled, while still ignoring only legacy overflow beyond the expected count; ensure the zero-length check in `read_length` handling still triggers for any run encountered.
🤖 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/rle.rs`:
- Around line 1459-1478: The RLE decoder in the `decode_runs` logic is returning
early once `decoded.len()` reaches `expected_value_count`, which allows a
trailing zero run length to be skipped instead of rejected. Update the loop to
keep validating remaining run lengths even after the expected output has been
filled, while still ignoring only legacy overflow beyond the expected count;
ensure the zero-length check in `read_length` handling still triggers for any
run encountered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 76833c0c-7928-4807-af17-4390cefceabe
📒 Files selected for processing (1)
rust/lance-encoding/src/encodings/physical/rle.rs
Only miniblock chunks can legitimately overflow their declared value count (legacy chunk-boundary crossing); block payloads have no next chunk, so an overflow there is corruption and stays a hard error.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/rle.rs`:
- Around line 2184-2190: The test in rle.rs only checks the error message from
BlockDecompressor::decompress and should also verify the specific overflow error
variant. Update the assertion to match on the strict error type returned by
decompress, then separately assert that its message contains the expected
“overflowed expected value count” text, using the BlockDecompressor::decompress
call site as the reference point.
- Around line 1510-1511: The trailing-run handling in the RLE decode path is
stopping too early by breaking after clamping, which lets zero-length runs slip
through. Update the logic in the decode loop around the `decoded.resize(...)`
branch in `rle.rs` so it keeps validating remaining run lengths after
`expected_value_count` is reached, rejecting any zero-length runs while only
suppressing extra output beyond the expected count. Use the existing
decode/run-length scanning logic in that function to continue iterating instead
of exiting immediately.
- Line 1485: The output buffer reservation in RLE decoding is unguarded and can
abort on malformed metadata. In the decoder path that builds the `decoded` Vec
in rle.rs, replace the direct capacity allocation with a fallible reservation
using try_reserve_exact(expected_value_count), and convert any allocation
failure into Error::invalid_input_source before proceeding. Keep the change
scoped to the decode logic that uses expected_value_count so oversized counts
are handled safely.
🪄 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: 3defb8bb-91be-4f7d-9b83-5b92ede74d87
📒 Files selected for processing (1)
rust/lance-encoding/src/encodings/physical/rle.rs
westonpace
approved these changes
Jul 9, 2026
Guard the decoded-vector allocation with try_reserve_exact so corrupt metadata surfaces as InvalidInput instead of aborting, and assert the error variant in the block overflow test.
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/rle.rs (1)
1510-1512: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse checked addition in the overflow error path
decoded.len() + lengthcan overflow for large RLE counts and turn malformed input into a panic while buildinginvalid_input_source. Usechecked_addhere and report the safe total.🤖 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 1510 - 1512, In the RLE overflow error path inside the decoding logic in rle.rs, replace the direct decoded.len() + length calculation with a checked addition so malformed large counts cannot panic while constructing invalid_input_source. Update the overflow reporting in the RLE decode handling to use the safe total from checked_add, and keep the existing error context around the expected_value_count message.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/rle.rs`:
- Around line 1510-1512: In the RLE overflow error path inside the decoding
logic in rle.rs, replace the direct decoded.len() + length calculation with a
checked addition so malformed large counts cannot panic while constructing
invalid_input_source. Update the overflow reporting in the RLE decode handling
to use the safe total from checked_add, and keep the existing error context
around the expected_value_count message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a9d961f1-27a8-498e-9b12-f4a59d9a5ad6
📒 Files selected for processing (1)
rust/lance-encoding/src/encodings/physical/rle.rs
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
2 participants
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.
#7376 made the RLE miniblock decoder reject chunks whose run lengths sum past the declared value count. The legacy (pre run-length-width) miniblock encoder rolled back to a power-of-2 checkpoint after a run had already crossed it, so existing files contain chunks that declare 2048 values but hold runs summing slightly more; the excess values are re-encoded at the start of the next chunk. The legacy decoder truncated the excess, so those files were valid when written — after #7376 they fail to read with:
This restores the truncating behavior for miniblock decoding: clamp the final crossing run at the declared count and ignore trailing legacy runs, while still rejecting underflow and zero run lengths. Block payloads have no chunk boundaries, so an overflow there can only be corruption and remains a hard error. Current encoders emit exact chunks, so this only changes how legacy files are read. Includes regression tests for the legacy chunk-boundary overflow and the strict block path.
Summary by CodeRabbit