feat: expose independent batch nearest search in C and C++ - #92
Conversation
e0fd094 to
26608ca
Compare
LuciferYang
left a comment
There was a problem hiding this comment.
Reviewed the batch nearest-neighbor change. No blocking issues; the validation is solid. Mutual exclusion with limit/offset and FTS is enforced symmetrically in both configuration orders, the error path leaves no half-updated state (the only write to s.nearest is the last statement), and the C/C++ signatures match the Rust FFI.
Four inline notes below: one MEDIUM on error-message quality, and three LOW (a latent unreachable!(), the Int8 API asymmetry, and a thin unit test). I also chased down two things that looked off and cleared both. Batch does not pin a default metric the way multi-vector does, but Lance's planner sets the metric before the TopK merge and single-vector search uses the same wrapper path, so batch cannot diverge. And the README's "UInt8 uses the Hamming metric" line is accurate: default_distance_type_for maps UInt8 to Hamming.
| }) | ||
| .ok_or_else(|| { | ||
| invalid(format!( | ||
| "batch query values exceed {MAX_BATCH_QUERY_BYTES} bytes" |
There was a problem hiding this comment.
MEDIUM · robustness
The byte-cap error "batch query values exceed 67108864 bytes" collapses two distinct failures into one message: dimension * num_queries * width overflowing, and the size genuinely exceeding 64 MiB. It also names none of dimension, num_queries, or the computed byte size. A caller rejected with a legitimate 80 MB request just sees "exceeds 64 MiB" with no hint of which parameter blew the budget; on a 32-bit target the overflow path reports the wrong reason entirely.
AGENTS.md asks for variable names and values in error messages. Splitting the two cases and threading dimension/num_queries/the byte size into each message is enough. Correctness is fine; this is purely to make diagnosis easier.
There was a problem hiding this comment.
Fixed in 4ceed34. Checked multiplication now reports usize overflow separately from the 64 MiB limit. Both errors include dimension, num_queries, and element_width; the limit error also includes the computed byte size. Tests cover both multiplication overflow points, the exact byte-limit boundary, and an 80 MB request rejected before reading its buffer while preserving the previously configured query.
| .iter() | ||
| .all(|v| v.is_some_and(f64::is_finite)), | ||
| DataType::UInt8 => true, | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
LOW · maintainability
The finite-check match falls through to unreachable!(). It is genuinely unreachable today only because the element_type match at line 3062 admits 0..=3. But decode_query_values already decodes element_type=4 (Int8), so the two matches are one edit apart: add an Int8 arm at 3062 and decode produces an Int8Array that panics here.
panic = "unwind" turns that into LANCE_ERR_PANIC rather than UB, but it was meant to be a clean invalid_input. Returning Err(invalid(...)) from the fallthrough, or adding an explicit Int8 arm, is safer than leaving unreachable!().
There was a problem hiding this comment.
Fixed in 4ceed34. Int8 now has an explicit finite-check arm alongside UInt8, and the fallback returns invalid_input with the decoded type and element_type instead of unreachable!(). Future decoder extensions therefore cannot turn this validation path into a panic.
| 3 => (DataType::UInt8, 1), | ||
| _ => { | ||
| return Err(invalid(format!( | ||
| "batch element_type ({element_type}) must be float16, float32, float64, or uint8" |
There was a problem hiding this comment.
LOW · API consistency
Batch accepts only element_type 0..=3 and rejects Int8 (4), while the single-vector nearest path decodes Int8 via decode_query_values. So the same Int8 vectors work through single-vector search but not through batch, and a caller migrating from one to the other hits a wall.
The README lists only float16/32/64/uint8 for batch, so this reads as intentional. Worth confirming the direction: if Int8 vector search is meant to be supported, add it to batch; if it is not, the single-vector path should reject it too (or the docs should say so), rather than leaving the two APIs with different type sets. (The asymmetry runs both ways: batch adds a query_data alignment check that nearest/nearest_multivector lack.)
There was a problem hiding this comment.
Fixed in 4ceed34. The pinned Lance dependency supports ordinary Int8 vector queries, so batch now accepts Int8 too. The new L2 regression uses negative Int8 coordinates and checks complete per-query IDs and distances. README and C API documentation list Int8. Alignment validation now lives in the shared decoder used by single-vector, multi-vector, and batch queries and uses each type's actual alignment requirement; a regression covers misaligned floating-point buffers.
| counts[*q as usize] += 1; | ||
| } | ||
| } | ||
| assert_eq!(counts, [2, 2]); |
There was a problem hiding this comment.
LOW · testing
This test asserts only the per-query_index row counts [2, 2], and only on a flat scan (use_index=false); it checks neither distances nor which rows came back. An implementation with correct counts but wrong contents would pass it. The feature as a whole is held up by the full (query_index, id, _distance) comparison against a brute-force expected() in batch_nearest_test.rs, so this is not a coverage gap, just a weak individual test.
Adding a distance/row-membership assertion here would make it carry its weight. Separately, batch combined with index_segments (distributed segment-scoped search) has no test at all, even though build_scanner applies with_index_segments for batch too and the README promises index selection applies; a case for that is worth adding.
There was a problem hiding this comment.
Fixed in 4ceed34. The unit regression now asserts complete (query_index, id, _distance) tuples. A new integration test builds two IVF_FLAT segments and tests selecting the first segment, the second segment, and both, comparing each query's IDs and distances against a brute-force result restricted to the selected rows. All 469 Rust tests and all 3 native C/C++ consumer tests passed, along with formatting, cargo check, and Clippy.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Native Lance execution provides up to k results per query, tagged with query_index. Copied inputs and explicit query modes preserve ownership and separation from multi-vector scoring. The documented request bounds and conditional shared execution fit this contract; global limit/offset remain rejected.
Int8 uses the existing native vector path. Shared alignment validation rejects misaligned buffers before decoding, and batch size failures preserve the previous query. The expanded coverage checks complete IDs and distances across separately selected IVF segments.
Distance ranges remain a single-vector setting: replacing that query with a batch clears its bounds, and attempts to set batch bounds fail without altering the query.
Expose independent batch nearest-neighbor search through
lance_scanner_nearest_batchand the C++Scanner::nearest_batchoverloads. A row-major matrix of N vectors produces up to K neighbors per query, identified by a zero-basedquery_indexcolumn, including a batch of one. This provides the binding needed for consumers such as apache/doris#68394.The existing fixed-size-list query representation also describes multi-vector queries. Track the query mode explicitly so independent batches use Lance's native batch execution instead of the custom multi-vector scoring/window path. Single-vector, multi-vector, and batch queries share input-alignment validation before decoding typed buffers. No dependency changes are needed.
query_indexdataset column.num_queries * k * refine_factor <= 100000(unset refinement counts as 1). These bounds do not cap total index/payload memory.Validation:
cargo test: 469 Rust tests passed.cargo test --test compile_and_run_test -- --ignored: all 3 native consumer tests passed (C, C++, and static OSS transport); batch checks validate per-query ids, distances, counts, and stream ownership.cargo clippy --locked --all-targets -- -D warnings,cargo check --locked --all-targets, formatting, and diff checks passed.