fix: backport encoding and FTS fixes to release/v10.0 - #8146
Merged
Conversation
…er::unzip (#8138) `parse_length` reads the length prefix out of the page buffer with `get_unchecked`, and the only thing between it and the end of the buffer is a `debug_assert!`: ```rust // Safety: Data should have at least bytes_per_length bytes remaining debug_assert!(databuf.len() >= bytes_per_length); let length = unsafe { Self::parse_length(databuf, in_bits_per_length) }; ``` There is no `[profile.release]` override in the workspace `Cargo.toml`, so `debug-assertions` defaults to false in release and that assertion is not present in the published wheels. The loop it sits in continues on `while !databuf.is_empty()`, so it enters the body with as little as one byte remaining. `parse_length` then reads up to eight. A page whose item walk ends with a partial trailing item therefore reads past the end of the buffer. Reproduced on x86-64 with `-Zsanitizer=address` on a release build, driving the real `VariableFullZipDecoder::new`: ``` ERROR: AddressSanitizer: heap-buffer-overflow READ of size 8 at 0x7b9989be1017 #0 <lance_encoding::...::VariableFullZipDecoder>::new 0x7b9989be1017 is located 3 bytes after 4-byte region [0x...1010,0x...1014) ``` A well-formed control buffer is clean in the same run. ## The change The payload read one line below the call site is already bounds checked and panics on malformed input: ```rust unzipped_data.extend_from_slice(&databuf[..length as usize]); ``` So a truncated item already fails cleanly on the payload path. Only the length read was inconsistent. This makes the two match by using safe indexing in `parse_length`, which lets the `unsafe` block and the `debug_assert!` both go away. On valid input the behaviour is unchanged. On a truncated trailing item the result is the same clean panic the payload path already produces, rather than an out-of-bounds read. ## Tests Two, per the contributing guide: - `variable_full_zip_wellformed_length_prefix` decodes a well-formed prefix - `variable_full_zip_truncated_length_prefix_is_rejected` is `#[should_panic]` and covers the case above Both pass, and the crate's existing suite is unaffected (520 passing before and after). ## Scope, stated honestly I have not established that a `.lance` file produced by the writer can reach this state. Truncating a data file is rejected earlier by the I/O range check, and a sweep of in-place single-byte edits either read cleanly, were rejected by that same check, or panicked in safe code further along in decode. So I am not claiming this is reachable from a crafted dataset, and I am filing it as hardening rather than as a security report. The case for the change does not depend on that: an `unsafe` read whose only guard is compiled out of release builds is worth removing on its own, particularly when the adjacent read of the same buffer is already checked. ## One unrelated observation Not part of this change, and not something I have shown to be a bug, but it looked odd while reading. The length is read using `in_bits_per_length` and the cursor is then advanced by `bytes_per_offset`, which comes from `out_bits_per_offset`: ```rust let length = ... parse_length(databuf, in_bits_per_length); databuf = &databuf[bytes_per_offset..]; ``` Those are equal in the common case, so this may well be deliberate. Flagging it only in case the asymmetry is unintentional. --------- Co-authored-by: Xuanwo <github@xuanwo.io> (cherry picked from commit a778c59)
…ath (#8119) New-format indexes persist total_tokens in schema metadata, so aggregate_corpus_stats() resolves O(1) without loading doc lengths as a side effect. The scoring phase then had to load lengths sequentially per partition, adding one extra disk round-trip on the cold query path. Fix: after aggregate_corpus_stats(), pre-load lengths in parallel for partitions that contain at least one query token. Partitions with no matching terms are skipped to preserve the existing no-load optimization for no-hit queries. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> (cherry picked from commit 499045f)
github-actions
Bot
added
bug
Xuanwo
marked this pull request as ready for review
August 2, 2026 20:54
![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: request changes. The two encoding backports correctly close validated corruption paths, but the FTS preload is not justified by this release branch: candidate preparation already overlaps partition loads and the new phase bypasses eligibility pruning. Please retain the encoding fixes and drop the FTS cherry-pick, or move bounded length loading behind posting and visibility pruning with no-hit regression coverage.
| return None; | ||
| } | ||
| let has_match = (0..request.tokens.len()) | ||
| .any(|i| part.tokens.get(request.tokens.get_token(i)).is_some()); |
Contributor
There was a problem hiding this comment.
This any predicate eagerly reads and retains the full document-length column for a partition that cannot satisfy an AND/phrase query, and it runs before an empty visibility mask can discard that partition. A zero-result search therefore gains O(rows) I/O and memory and can fail on a length read it never needed. Keep lengths() in the existing bounded partition-preparation pipeline after posting-list and visibility pruning; that preserves concurrent loads without sacrificing the no-hit contract.
Reproducer
I appended this case to test_no_hit_partition_does_not_load_document_columns on this head:
let tokens = Arc::new(Tokens::new(
vec!["t0".to_owned(), "missing-token".to_owned()],
DocType::Text,
));
let params = Arc::new(FtsSearchParams::new().with_limit(Some(10)));
let (row_ids, scores) = index
.bm25_search(
tokens,
params,
Operator::And,
Arc::new(NoFilter),
Arc::new(NoOpMetricsCollector),
None,
)
.await
.unwrap();
assert!(row_ids.is_empty());
assert!(scores.is_empty());
assert!(
!documents.lengths_loaded(),
"an AND query missing a required term should not load document lengths",
);Run: CARGO_TARGET_DIR=/home/agent/tmp/pr8146-target cargo test -p lance-index test_no_hit_partition_does_not_load_document_columns -- --nocapture
The query returned no rows as expected, but the final assertion failed because document lengths had been loaded.
jackye1995
approved these changes
Aug 2, 2026
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
4 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.
Summary
Backport the selected fixes from #8138, #8144, and #8119 onto
release/v10.0.Changes
VariableFullZipDecoder::unzip.release/v10.0writer test API without changing their coverage.Validation
cargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningscargo test -p lance-file(136 unit tests and 5 doctests passed)