fix(linalg): reject a null Int8 query instead of panicking - #8884
fix(linalg): reject a null Int8 query instead of panicking#8884LuciferYang wants to merge 15 commits into
Conversation
|
My previous revision made the I had qualified it with "With debug assertions on", on the theory that the Worth noting how nearly I convinced myself otherwise: my first attempt at that measurement used an The comment in |
|
A review pass found that the
Measured on the same crate in both profiles. With a dimension-8 target and a length-3 f32 query, debug panics and release returns The line for cosine now says the check is debug-only and names why. That describes this PR base; #8875 adds |
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The author’s correction matches the current paths: cosine’s dimension check is debug-only, whereas L2 and Dot use the always-on layout assertion. The revised docs now describe that pre-existing mismatch hazard accurately; it is unchanged by this null-query fix and is addressed separately by #8875, so no additional change is needed here. The shared Int8 null guard and focused regression still behave as intended.
|
Fifth instance of the same class, and the most interesting one. My cosine qualifier said a mismatch "can read past the shorter vector" in release. Literally hedged, but the reasoning behind it was wrong: cosine having no always-on layout assert does not mean nothing catches a mismatch. Measured on aarch64, both profiles. The line now says the mismatch is not reliably caught without debug assertions, which holds at every dimension, rather than asserting a read-past that only happens at two of them. Naming the dimensions in the doc would be a case table, and #8875 removes the distinction anyway by adding Not claimed, since it was not run: x86_64. There the sub-AVX2 |
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest correction matches the dimension-dependent paths: without debug assertions, specialized cosine branches can bypass a length check while fallback branches may still panic through dot. The revised “not reliably caught” wording now describes that pre-existing behavior accurately. The shared Int8 null guard and regression remain unchanged and valid; #8875 independently adds the always-on layout assertion.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest revision only tightens internal explanations: the helper is now scoped to the three batch entry points that actually accept Int8, and the Dot comments name the duplicate debug_assert_eq! that previously ran before the null guard. These statements match the dispatch and call order. Production behavior, the shared null guard, and its regression remain unchanged; the separate cosine layout fix is still handled independently in #8875.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest revision correctly avoids promising validation precedence while retaining accurate error and panic contracts. The shared Int8 conversion guard still turns null query coordinates into a descriptive error for Cosine, Dot, and L2 without changing supported-query results. The pre-existing cosine release layout gap remains independently handled by #8875.
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The latest cleanup removes edit-history commentary and restores unchanged documentation wrapping without altering executable behavior or public contracts. The shared Int8 conversion guard still rejects null query coordinates with descriptive errors across Cosine, Dot, and L2 while preserving valid and sliced inputs. The separate cosine layout gap remains independently handled by #8875.
What this changes
l2_distance_arrow_batch,dot_distance_arrow_batchandcosine_distance_arrow_batchtake the query as a&dyn Arrayand widen anInt8one element at a time. All three did it the same way:A null element in the query reaches that
unwrapand panics, in every profile. The three functions all returnResultand all document that the null buffer oftois propagated to the output, which says nothing about nulls infrom, so a caller has no reason to expect a panic from the other side.The three sites now share
int8_query_to_f32, which rejects the input withInvalidArgumentErrorand names the count, then widens throughvalues()so noOptionremains. Returning an error rather than panicking is what the unsupported-type arm of the samematchalready does, though l2's is aComputeErrorwhere dot's and cosine's areInvalidArgumentError. The arm is reachable from a real caller:lance-index's flat search passes a user query straight intoDistanceType::arrow_batch_func().Three smaller things travel with it.
dot_distance_arrow_batchcarried adebug_assert_eq!on the dimension in its public entry point, ahead of theInt8arm. Once that arm returns an error instead of panicking, the copy preempts it in a debug build: a query that is both null-bearing and the wrong length would panic on dot and return the error on its two siblings. The copy is gone;do_dot_distance_arrow_batchstill carries the same assert, which is where l2 and cosine have theirs.All three functions gain an
# Errorssection, which none of them had. It names the new null rejection and says the list is not exhaustive, since the unsupported-type and downcast arms return errors of their own.All three already had a
# Panicssection, and only cosine's changes: it now says the length mismatch is caught with debug assertions on, and not reliably without them, because cosine has no always-on layout assert of its own. l2 and dot keep theirs as written, sinceassert_batch_layoutis a plainassert!on every batch path they reach.The float arms are untouched.
Float16,Float32andFloat64hand the array todo_*_arrow_batchdirectly, which reads it throughas_slice()and ignores the validity buffer, so a null there is read as whatever the slot holds rather than panicking. Changing that is a separate decision about what a null query element should mean.Test plan
test_arrow_batch_rejects_null_int8_querygoes throughDistanceType::arrow_batch_func()for L2, Cosine and Dot, so it covers all three sites through the public dispatch rather than each function directly. Three parts:[Some(1), None]returnsInvalidArgumentErrorwhose message contains bothInt8 query vector \from`andfound 1 in 2 values`Int8query[None, Some(3), Some(4), None]at offset 1 goes through, because the slice window holds no nulls even though the full buffer does. Both the null count and the widening have to describe the same window or this either rejects a clean query or widens the wrong two elements. L2's result is asserted literally as[8.0, 0.0], which is what pins the widening; Cosine and Dot are compared against a call on an unsliced[3, 4], which pins the window but would agree with itself if the widening were wrong.test_arrow_batch_null_and_length_mismatch_agreesends an input that is wrong in both ways at once, a three-element null-bearing query against a dimension-2 target, and asserts all three metrics reach the null error. Restoring dot's entry-pointdebug_assert_eq!makes that test fail on dot withleft: 3, right: 2.Measured: restoring the three
unwrapcall sites makestest_arrow_batch_rejects_null_int8_queryfail with a panic at theunwrapinl2.rsrather than an error.cargo test -p lance-linalg: 396 passed, 1 ignoredcargo fmt --all -- --checkandcargo clippy -p lance-linalg --all-targets -- -D warnings: clean