fix(udf): compare row against row when both arguments are columns - #35
Merged
Merged
Conversation
A distance function given a column as its second argument resolved that column to a single query vector by taking row 0, then compared every row against it. `cosine_distance(v, v)` answered 0 for the first row and the distance to the first row for all the others — no error, no warning, and adding a filter moved which row came out as 0, because it changed which row led the scan. Compare row i against row i when the second argument has the same number of rows as the first, which is how a scalar function over two columns behaves everywhere else and what DuckDB's array_cosine_distance does. A scalar, a literal, or a single-row array still broadcasts to every row, so the documented query-vector form is unchanged. A second column of any other length is now an error instead of silently using row 0. Expected values in the tests are DuckDB's, over the same fixture.
Three gaps in the previous commit, from review. Only a list-typed array is a column of vectors. A flat Float32Array or Float64Array is one query vector however many elements it holds, and routing it by row count sent it to the pairwise path where it failed — a shape that worked before. Decide on the array type instead. The kernels are symmetric and neither the docs nor the signature fix which side holds the query, so a single vector given as the first argument now broadcasts over a column in the second, rather than being refused for having one row. Cover the two behaviours that had no test: a null on either side yields a null distance, and two columns of different lengths error. The second goes through invoke_with_args, since DataFusion gives every argument the same row count and SQL cannot reach that arm. Document both shapes in the module header and the README.
There was a problem hiding this comment.
All four prior threads are addressed in the code, not only in replies.
The type-based rule in is_vector_column (src/udf.rs:265) keeps the flat query-vector branches of extract_query_vec reachable, and the (1, _) arm handles a query vector on the left. try_extract_distance (src/rule.rs:492) rejects a column as the second argument, so the row-wise case cannot be rewritten into an ANN search.
Two non-blocking comments on README.md. CI Test and Clippy had not reported when this review started.
The symmetry paragraph read as unconditional. try_extract_distance requires args[0] to be a column and args[1] a literal, so the reversed form returns the same distances but is not rewritten and falls back to an exact scan — correct answers, no index, no signal. Also move the paragraph above the lead-in sentence it had been inserted into, which ends in a colon pointing at the kernel table.
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
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.
The bug
A distance function given a column as its second argument silently compared every row against row 0 of that column.
Each row's own vector against itself must be
0for every row. Instead:No error and no warning. And it is not stable: adding
WHERE id >= 3moves the0onto row 3, because the filter changes which row leads the scan. The same query over the same data can give different answers depending on batching, filtering, or parallelism.extract_query_vecresolves argument 2 to a single vector, taking.value(0)when handed an array. That is correct for the intended shape — one query vector — but silently wrong for a column.What it should do
Row-wise: row i of argument 1 against row i of argument 2. That is how every scalar function over two columns behaves, and it is what DuckDB does:
Only the column-against-column case diverged; the literal query-vector form was already correct and is unchanged.
The change
Nulls on either side yield a null distance, matching the existing single-vector path. Per-row dimensionality mismatches error, as they already did.
Verification
Expected values are DuckDB's, taken from
array_cosine_distance/array_distanceover the same three orthogonal unit vectors.Confirmed the tests fail without the fix and pass with it:
Full suite: 78 passed, 0 failed (3 pre-existing ignored). clippy and fmt clean.
Note on
l2_distanceWhile writing the oracle comparison: this crate's
l2_kernelreturns L2 squared, deliberately, to match USearch'sMetricKind::L2sq. Sol2_distancegives2.0for an orthogonal unit pair where DuckDB'sarray_distancegives1.414. Ranking is identical since squaring is monotonic, so k-NN results agree — but absolute values do not, which matters for a threshold ported from DuckDB. Not changed here; pinned in a test so the convention is asserted rather than assumed.