fix(query): stop sending HNSW params for an unhandled index type (#216) - #243
Merged
Merged
Conversation
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.
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.
Closes #216.
Summary
setRadius(),setLinear()andsetUsingRefiner()all failed on a DiskANN field:The message blames HNSW. The adapter was the one sending HNSW params.
Root cause
ensure_query_params_for_field()had noIndexType::DISKANNcase and 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" — which upstream then rejects.The only workaround was
setDiskAnnParams()first, which is not discoverable from the error.Radius and linear are supported by the DiskANN core upstream — they were broken purely by the wrong param type. The refiner is a genuine upstream limitation, but it now reports upstream's own error instead of blaming the wrong thing.
DiskAnnQueryParamsonly takeslist_sizein its constructor, so radius and the flags are set afterwards from the commonQueryParamsbase.The more valuable half
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: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 rather than left for the next person to rediscover. The group-by switch had the same fallback and got the same treatment.
Verification
tests/bug_0057.phptcovers the three cases plus the refiner. Since the removed fallback was silently doing work for the unresolvable case, I also checked that the handled types still filter correctly:Full suite:
One test-hygiene note: the refiner case is expected to fail and upstream logs it at ERROR on stderr, so the test initialises with
LOG_FATAL— same reason the FTS tokenizer tests do.🤖 Generated with Claude Code