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