From 5326681323e16e41da1380e6af48e81b689e76ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ha=C5=82as=20Piotr?= Date: Tue, 29 Sep 2026 15:30:09 +0200 Subject: [PATCH] fix(query): stop sending HNSW params for an unhandled index type (#216) setRadius(), setLinear() and setUsingRefiner() all failed on a DiskANN field with query params type does not match the index type of vector field[v], expected DISKANN but got HNSW The message blamed HNSW, but the adapter was the one sending HNSW params. ensure_query_params_for_field() had no IndexType::DISKANN case, so it fell through to a hardcoded HNSW fallback. The fallback ran even when the index type *had* been resolved from the schema, so it silently converted "I do not handle this index type" into "these are HNSW params" -- and upstream rejects exactly that. The workaround was to call setDiskAnnParams() first, which is not discoverable. Radius and linear are supported by the DiskANN core upstream, so they were broken purely by the wrong param type and now work. The refiner is an upstream DiskANN limitation, but it now reports upstream's own error instead of the misleading type mismatch. DiskAnnQueryParams only takes list_size in its constructor, so radius and the flags are set afterwards from the common QueryParams base. The default: branch no longer falls back to HNSW at all. An index type with no case now gets no query params, which lets upstream use its own defaults and skip its params-type check. That fails far less often than guessing, and it also covers index types added upstream after this adapter was written -- the DiskANN and IVF_RABITQ cases were both missing for the same reason. The trade-off is that radius/linear/refiner are not applied in that case, so each new index type needs its own branch; that is recorded in the comment. The group-by switch, which had the same HNSW fallback, got the same treatment. Test: tests/bug_0057.phpt. Verified separately that HNSW, IVF, Flat and Vamana still filter by radius correctly, since the fallback was silently doing work for the unresolvable case. Tests: 213/213, 0 skipped, 0 failed, 2 expected fail. --- CHANGELOG.md | 8 ++++ ffi/zvec_ffi.cc | 31 +++++++++++++-- tests/bug_0057.phpt | 92 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 127 insertions(+), 4 deletions(-) create mode 100644 tests/bug_0057.phpt diff --git a/CHANGELOG.md b/CHANGELOG.md index 26d5111..e43b224 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`setRadius()` / `setLinear()` / `setUsingRefiner()` failed on a DiskANN field** (#216) + - All three threw `query params type does not match the index type of vector field[v], expected DISKANN but got HNSW`. The message blamed HNSW, but the adapter was the one sending HNSW params. + - `ensure_query_params_for_field()` had no `IndexType::DISKANN` case, so it fell through to a hardcoded HNSW fallback. The fallback ran even when the index type *had* been resolved, so it converted "I do not handle this index type" into "these are HNSW params", which upstream then rejects. + - Radius and linear are supported by the DiskANN core upstream, so they were broken purely by the wrong param type. Both work now; the same holds for `setDiskAnnParams(300)->setRadius(0.5)`, which is the workaround this bug forced on users. + - The refiner is an upstream DiskANN limitation, but it now surfaces upstream's own error instead of the misleading type mismatch. + - The `default:` branch no longer falls back to HNSW. An index type with no case gets **no** query params, which makes upstream use its own defaults and skip its params-type check — that fails far less often than guessing, and it also covers index types added upstream after this adapter was written. The trade-off: radius/linear/refiner are not applied in that case, so each new index type needs its own branch. The group-by switch got the same treatment. + - Test: `tests/bug_0057.phpt`. Verified that HNSW, IVF, Flat and Vamana still filter by radius correctly. + - **A failed `destroy()` left the PHP object with a dangling handle** (#222) - `zvec_collection_destroy()` erased the handle from the collections registry even when upstream returned an error, deleting the C++ `Collection` while the PHP object kept the raw pointer. The next method call on it operated on freed memory — reachable by destroying a read-only collection, which upstream rejects with `INVALID_ARGUMENT`, and later by destroying while a document iterator is open. - The registry entry is now erased only on success. On failure upstream leaves the collection untouched, so the handle stays valid and the object remains open and usable. diff --git a/ffi/zvec_ffi.cc b/ffi/zvec_ffi.cc index 4a74897..7a22fbf 100644 --- a/ffi/zvec_ffi.cc +++ b/ffi/zvec_ffi.cc @@ -2627,13 +2627,28 @@ static void ensure_query_params_for_field(Collection* c, SearchQuery& query, con case IndexType::VAMANA: query.target_.query_params_ = std::make_shared(200, holder->radius_, holder->is_linear_, holder->is_using_refiner_); return; + case IndexType::DISKANN: + // DiskAnnQueryParams only takes list_size; radius and the + // flags come from the common QueryParams base. + query.target_.query_params_ = std::make_shared(); + query.target_.query_params_->set_radius(holder->radius_); + query.target_.query_params_->set_is_linear(holder->is_linear_); + query.target_.query_params_->set_is_using_refiner(holder->is_using_refiner_); + return; default: - break; + // An index type we do not handle. Leaving query_params_ null + // lets upstream use its own defaults and skip its params-type + // check, which fails far less often than guessing HNSW: the + // guess was rejected outright for DISKANN before this case + // existed, and for any index type added upstream later. + // Trade-off: radius/linear/refiner are not applied in that + // case. Each new index type needs its own branch here. + return; } } } - // Fallback: use HNSW params if can't determine index type - query.target_.query_params_ = std::make_shared(200, holder->radius_, holder->is_linear_, holder->is_using_refiner_); + // Could not read the schema or the field. Same reasoning as above: no + // params beats the wrong ones. } } @@ -2699,8 +2714,16 @@ zvec_status_t zvec_collection_group_by_query_vector(zvec_collection_t coll, cons case IndexType::VAMANA: holder->query.target_.query_params_ = std::make_shared(200, holder->radius_, holder->is_linear_, holder->is_using_refiner_); break; + case IndexType::DISKANN: + holder->query.target_.query_params_ = std::make_shared(); + holder->query.target_.query_params_->set_radius(holder->radius_); + holder->query.target_.query_params_->set_is_linear(holder->is_linear_); + holder->query.target_.query_params_->set_is_using_refiner(holder->is_using_refiner_); + break; default: - holder->query.target_.query_params_ = std::make_shared(200, holder->radius_, holder->is_linear_, holder->is_using_refiner_); + // See ensure_query_params_for_field(): an unhandled index + // type gets no params rather than wrong ones, so the user + // sees upstream's real "not supported" message. break; } } diff --git a/tests/bug_0057.phpt b/tests/bug_0057.phpt new file mode 100644 index 0000000..651fa01 --- /dev/null +++ b/tests/bug_0057.phpt @@ -0,0 +1,92 @@ +--TEST-- +Bug 0057: queryVector() with radius/linear/refiner on a DiskANN field sent HNSW params +--SKIPIF-- + +--FILE-- +addVectorFp32('v', dimension: 4, metricType: ZVecSchema::METRIC_L2); + $c = ZVec::create($path, $schema); + $c->createIndex('v', ZVecIndexParams::forDiskAnn( + metricType: ZVecSchema::METRIC_L2, + maxDegree: 32, + listSize: 100, + )); + foreach ([['doc1', [1.0, 0.0, 0.0, 0.0]], ['doc2', [0.9, 0.1, 0.0, 0.0]], ['doc3', [0.0, 1.0, 0.0, 0.0]]] as [$pk, $vec]) { + $c->insert((new ZVecDoc($pk))->setVectorFp32('v', $vec)); + } + $c->optimize(); + + $cases = [ + // doc3 is at squared L2 distance 2.0, so a radius of 0.5 excludes it. + 'radius only' => static fn(ZVecVectorQuery $q) => $q->setRadius(0.5), + 'linear only' => static fn(ZVecVectorQuery $q) => $q->setLinear(true), + 'explicit+radius' => static fn(ZVecVectorQuery $q) => $q->setDiskAnnParams(300)->setRadius(0.5), + ]; + foreach ($cases as $name => $apply) { + $q = (new ZVecVectorQuery('v', [1.0, 0.0, 0.0, 0.0]))->setTopk(3); + $apply($q); + try { + $pks = array_map(static fn(ZVecDoc $d): string => $d->getPk(), $c->queryVector($q)); + echo "$name: " . implode(',', $pks) . "\n"; + } catch (ZVecException $e) { + echo "$name: ERROR code={$e->getCode()} {$e->getMessage()}\n"; + } + } + + // The refiner is not supported by the DiskANN core upstream. What matters + // is that the failure is now upstream's own, not "expected DISKANN but + // got HNSW" -- that message blamed the wrong thing entirely. + $refiner = (new ZVecVectorQuery('v', [1.0, 0.0, 0.0, 0.0]))->setTopk(3)->setUsingRefiner(true); + try { + $c->queryVector($refiner); + echo "refiner: accepted\n"; + } catch (ZVecException $e) { + echo 'refiner: ' . (str_contains($e->getMessage(), 'expected DISKANN but got HNSW') ? 'WRONG PARAMS' : 'upstream error') . "\n"; + } + + $c->close(); +} finally { + exec('rm -rf ' . escapeshellarg($path)); +} +?> +--EXPECT-- +radius only: doc1,doc2 +linear only: doc1,doc2,doc3 +explicit+radius: doc1,doc2 +refiner: upstream error