fix: relax test_query_delta_indices assertion for approximate index#7622
Conversation
The test_query_delta_indices test asserts that the top-2 nearest neighbors of a query vector are exactly [0, 1000], but this only holds for exact indexes (e.g. IvfPq / IvfFlat). For approximate indexes such as IvfHnswSq, scalar quantization can cause the returned ids to differ from the ground truth (observed on Windows CI: got [1000, 1486]). The core intent of the test is to verify that both delta indices are queried (i.e. one result comes from each delta segment). Relax the assertion accordingly for approximate indexes while keeping the exact assertion for exact indexes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| // For exact indexes (e.g. IvfPq) the top-2 nearest neighbors of the | ||
| // query vector are deterministic (id 0 in the first delta and id 1000 | ||
| // in the second delta, since the same vector is duplicated across | ||
| // the two fragments). For approximate indexes (e.g. IvfHnswSq) the | ||
| // returned ids are not guaranteed to be exactly [0, 1000]; the key | ||
| // property this test verifies is that both delta indices are queried | ||
| // (i.e. one result comes from each delta), so we only assert that. |
| let is_approximate = !matches!( | ||
| index_params.index_type(), | ||
| IndexType::IvfFlat | IndexType::IvfPq | ||
| ); |
📝 WalkthroughWalkthroughThe test ChangesDelta Query Test Update
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 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.
🧹 Nitpick comments (1)
rust/lance/src/index/append.rs (1)
1216-1233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApproximate-index classification is inverted rather than positive-listed, and the "exact" branch is untested here.
is_approximateis derived as!matches!(index_params.index_type(), IndexType::IvfFlat), i.e. anything that isn'tIvfFlatis treated as approximate. This correctly resolves the earlier review feedback thatIvfPqshould not be classified as exact (per the past comments and follow-up commit noted in the PR description). However, since the#[values(...)]list for this test only suppliesivf_pqandwith_ivf_hnsw_sq_params, theelsebranch (Line 1231, exact[0, 1000]assertion) is currently dead code within this test — it's never exercised. Consider positive-listing the exact type(s) explicitly (e.g.matches!(index_params.index_type(), IndexType::Vector)or whatever type the flat/brute-force build actually reports) instead of the negation, so that adding a new approximate index type to the values list doesn't silently default to the "exact" branch.🤖 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/src/index/append.rs` around lines 1216 - 1233, The test in append.rs is classifying approximate indexes by negating a single exact type check, which makes the exact branch effectively unreachable with the current parameter values. Update the `is_approximate` logic in the test near `IndexType::IvfFlat` to use a positive allowlist of the exact index type(s) actually produced by the flat/brute-force path, and keep the approximate assertion for the remaining types. This should be located via the `is_approximate` variable and the `index_params.index_type()` match in the test that asserts on `id_arr`.
🤖 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.
Nitpick comments:
In `@rust/lance/src/index/append.rs`:
- Around line 1216-1233: The test in append.rs is classifying approximate
indexes by negating a single exact type check, which makes the exact branch
effectively unreachable with the current parameter values. Update the
`is_approximate` logic in the test near `IndexType::IvfFlat` to use a positive
allowlist of the exact index type(s) actually produced by the flat/brute-force
path, and keep the approximate assertion for the remaining types. This should be
located via the `is_approximate` variable and the `index_params.index_type()`
match in the test that asserts on `id_arr`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 46ef85e2-da52-4608-a69e-4b21ae8ffe49
📒 Files selected for processing (1)
rust/lance/src/index/append.rs
2 participants
Summary
Fix a flaky assertion in
test_query_delta_indicesthat fails on Windows CI when the underlying index type is approximate (e.g.IvfHnswSq).Background
test_query_delta_indicesasserts that the top-2 nearest neighbors of the query vector are exactly[0, 1000]. This assumption only holds for exact indexes such asIvfPq/IvfFlat.For approximate indexes such as
IvfHnswSq, scalar quantization (SQ) introduces small numerical differences that can cause the returned row ids to diverge from the exact ground truth. This has been observed on Windows CI, where the test returned[1000, 1486]instead of[0, 1000], producing a flaky failure unrelated to the delta-index logic being tested.Reference failure log:
test index::append::tests::test_query_delta_indices<IvfHnswSq>on Windows CI[0, 1000][1000, 1486]What this PR does
The core intent of the test is to verify that both delta index segments are queried — i.e. one result comes from the first delta segment (row ids
< 1000) and the other from the second delta segment (row ids>= 1000).This PR relaxes the assertion so that:
IvfPq,IvfFlat, ...): keep the strict assertionrow_ids == [0, 1000].IvfHnswSq, ...): assert only the structural invariant — one result comes from each delta segment — instead of asserting the exact row ids.This preserves the original intent of the test while eliminating the false positive caused by SQ quantization error.
Changes
rust/lance/src/index/append.rs: relax the assertion intest_query_delta_indicesfor approximate index types.Testing
cargo fmt --check✅cargo clippy -p lance --lib --tests --all-features -- -D warnings✅cargo test -p lance index::append::tests::test_query_delta_indices✅ (all variants pass)Risk
Minimal — this is a test-only change that does not alter any production code path. It relaxes an over-strict assertion to match the actual guarantees provided by approximate indexes.
Summary by CodeRabbit