fix(index): reject cosine and hamming in float kmeans instead of panicking - #9027
fix(index): reject cosine and hamming in float kmeans instead of panicking#9027LuciferYang wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The training guards are sound, but assignment still validates unsupported metrics after optional index construction. Reject the data/centroid/metric combination before building SimpleIndex; that keeps invalid requests out of HNSW and makes the existing error arm authoritative.
| )) | ||
| } | ||
| (DataType::Float32, DataType::Float32, _) => { | ||
| (DataType::Float32, DataType::Float32, DistanceType::L2 | DistanceType::Dot) => { |
There was a problem hiding this comment.
This guard runs too late to guarantee the promised rejection: SimpleIndex::may_train_index has already consumed self.distance_type. For float/Hamming, the enabled (and sufficiently large automatic) index path builds FlatFloatStorage, then HNSW calls DistanceType::func::<f32> and panics at its Hamming => todo!() before this match can return InvalidArgumentError. Validate the type/metric combination before may_train_index, and only build the optional index after that validation.
Reproducer
LANCE_USE_HNSW_SPEEDUP_INDEXING=enabled cargo test -p lance-index --lib vector::kmeans::tests::test_compute_membership_rejects_cosine_and_hamming_for_floats -- --exact --nocaptureOn the current head, the test fails with rust/lance-linalg/src/distance.rs:313: not yet implemented; the expected result is for its existing unwrap_err assertions to pass.
…cking The training and assignment dispatches accepted any distance type for a float column, and the membership pass then panicked with "not supported" unless the centroid HNSW happened to be built, which bypasses that match. Narrow both dispatches to L2 and Dot so the existing descriptive error arms handle the rest. lance.util.KMeans documents cosine, so the binding now normalizes its input and clusters with l2, the way the index build path does. That makes cosine behave the same at every k instead of panicking below the HNSW threshold. The module and new_with_params doc comments claimed cosine inputs are normalized each iteration. That was implemented in lance-format#1723 and removed in lance-format#2015 while the comment was carried forward.
5450d01 to
8438ee9
Compare
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The Python cosine path now consistently normalizes and uses L2, but the indexed-assignment finding remains. Validate metric/type compatibility before SimpleIndex::may_train_index so float/Hamming cannot panic during HNSW construction.
Problem
KMeans::new_with_paramsdispatches a float column against any distance type, so cosine and hamming reach the float algorithm, whose membership pass panics withKMeans::find_partitions: cosine is not supported.compute_membership_and_distanceshas the same shape, so a cosine model built with the publicwith_centroidspanics on assignment too. Both matches already end in an arm that returns a descriptive error.There is one exception, and it is why
lance.util.KMeans(k, "cosine")looked like it worked: when the centroid set is large enough formay_train_indexto build an HNSW over it, the assignment goes through the index and never reads the distance type, so training completes. Below that threshold the same call panics.Fixes #9026.
What this changes
Narrow the float arms in both dispatches to
L2 | Dot, so cosine and hamming fall through to the error arm that was already there. Dot stays because the float path implements it.lance.util.KMeansdocuments cosine, so the binding now normalizes its input and clusters with l2, which is what the index build path does before every training call. Cosine then behaves the same at everykrather than panicking below the HNSW threshold, andpredictnormalizes its input so a row and a scaled copy of it land in the same cluster.The module and
new_with_paramsdoc comments claimed cosine inputs are normalized each iteration. That was implemented in #1723 and removed in #2015 while the comment was carried forward, so both now say what callers actually do.One thing this does not cover: calling the lower-level
compute_partitionsorKMeansAlgoFloat::compute_membership_and_distdirectly still panics for cosine, since the check lives in the dispatch rather than in the algorithm. Moving it would mean making the hot membership loop fallible.Test plan
test_kmeans_rejects_cosine_and_hamming_for_floatscovers training through both dispatch blocks: cosine and hamming over an f32 column atk=2, then cosine atk=257for the hierarchical route.test_compute_membership_rejects_cosine_and_hamming_for_floatscovers assignment throughwith_centroids, and asserts l2 still works so the narrowing is not over-wide. Restoring the wildcard arms makes each of them panic atkmeans.rs:511instead.test_kmeans_cosinecovers the Python surface atk=8, which is the size that used to panic: it trains, and it asserts that scaling the input by 7 does not move any row's cluster, which is what fails if eitherfitorpredictskips the normalization.cargo test -p lance-index --lib vector::kmeans17 passed, 1 ignoredcargo test -p lance --lib index::vector285 passed, 1 ignoreduv run pytest python/tests/test_kmeans.py4 passedcargo clippy --all --tests --benches -- -D warningsand the same forpython/Cargo.tomlcleancargo fmt --all --checkclean,uv run make lintclean