feat: Vamana two_pass_build and query prefetch (#220) - #234
Merged
Merged
Conversation
Two upstream Vamana options that zvec-php did not expose. HNSW prefetch already existed from #181, and this follows the same shape. Part A -- two_pass_build, an index param added in v0.7.0 that runs a second full-graph Vamana construction pass: better graph quality, slower build. forVamana() takes it as a trailing optional argument, so existing positional calls are unaffected. It is a separate FFI function rather than an extra argument to zvec_index_params_set_vamana(). That function's signature is part of the ABI other code may have been compiled against, and extending it in place would break those callers; a new function breaks nothing. The Vamana constructor is called with its full argument list, passing an empty QuantizerParam -- safe, because the rotate branch in build() still overwrites it when setQuantizerEnableRotate() was used. Part B -- prefetch_offset / prefetch_lines, query params from v0.5.1 that tune software prefetch during the Vamana graph search. Both prefetch setters now share the single stored prefetch_offset_ / prefetch_lines_ pair already on VectorQueryHolder, rather than adding Vamana-specific fields, and merge_stored_query_settings() applies them to VamanaQueryParams as well as HnswQueryParams. That makes the call order irrelevant -- setVamanaPrefetch() then setVamanaParams() works, and so does the reverse -- which is the same guarantee #197 and #181 gave HNSW. When prefetch was never set, the stored values are the upstream defaults, so re-applying them does nothing. zvec_vector_query_set_vamana_prefetch() creates VamanaQueryParams when no params exist yet, not HnswQueryParams: upstream rejects params whose type does not match the field's index type, and the prefetch-only case in the test is what catches that. The PHP setter validates its input and throws ZVecException; the C setter returns void, so there is no status to check, and it clamps negatives to 0 as a second safety net. queryVector() and createIndex() errors keep flowing through self::checkStatus(). Known limitation, documented in the README rather than fixed here: prefetch is honoured only by queryVector(). The legacy query() path sends just queryParamType, ef/nprobe, radius, isLinear and isUsingRefiner through zvec_collection_query_ex, so prefetch is silently dropped. HNSW prefetch has the same limitation today; fixing it needs new arguments on that function and belongs in its own issue. Tests: 193/193, 0 skipped, 0 failed, 2 expected fail. Index params cannot be read back from an existing index, so test_vamana_two_pass_build.phpt asserts on upstream's own CollectionSchema::to_string() output, which embeds ",two_pass_build:true" per field. test_vamana_prefetch.phpt covers both call orders, the 0/0 disable combination, and negative rejection. test_hnsw_prefetch.phpt, test_vamana_index.phpt, test_vector_query_vamana_params.phpt and test_docs_consistency.phpt pass unchanged.
…ass-prefetch # Conflicts: # CHANGELOG.md # tests/test_ffi_load.phpt
…ass-prefetch # Conflicts: # CHANGELOG.md # tests/test_ffi_load.phpt
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 #220.
Summary
Two upstream Vamana options that were not exposed. HNSW prefetch already existed from #181, so this follows the same shape.
two_pass_buildruns a second full-graph Vamana construction pass — better graph quality, slower build.prefetch_offset/prefetch_linestune software prefetch during the graph search; offset0disables it, lines0means "derive from vector size".Part A —
two_pass_buildTrailing and optional on
forVamana(), so existing positional calls are unaffected. Upstream and Python both default tofalse.It is a separate FFI function rather than an extra argument to
zvec_index_params_set_vamana(). That function's signature is part of the ABI other code may have been compiled against; extending it in place would break those callers, a new function breaks nothing. The Vamana constructor is now called with its full argument list, passing an emptyQuantizerParam— safe, because the rotate branch inbuild()still overwrites it whensetQuantizerEnableRotate()was used.Part B — Vamana prefetch
Both prefetch setters share one stored field pair.
VectorQueryHolderalready keepsprefetch_offset_/prefetch_lines_, so I reused those rather than adding Vamana-specific fields, and extendedmerge_stored_query_settings()to apply them toVamanaQueryParamsas well asHnswQueryParams. That makes call order irrelevant —setVamanaPrefetch()thensetVamanaParams()works, and so does the reverse — which is the same guarantee #197/#181 gave HNSW. When prefetch was never set, the stored values are the upstream defaults, so re-applying them is a no-op.zvec_vector_query_set_vamana_prefetch()createsVamanaQueryParamswhen no params exist yet, notHnswQueryParams: upstream rejects params whose type does not match the field's index type, and the prefetch-only case in the test is what catches that.The PHP setter validates and throws
ZVecException; the C setter returns void so there is no status to check, and clamps negatives to0as a second safety net.Known limitation, documented not fixed
Prefetch is honoured only by
queryVector(). The legacyquery()path sends justqueryParamType,ef/nprobe,radius,isLinearandisUsingRefinerthroughzvec_collection_query_ex, so prefetch is silently dropped there. HNSW prefetch has the same limitation today. Fixing it needs new arguments on that function and applies to both index types, so it belongs in its own issue. Noted in the README.Verification
Index params cannot be read back from an existing index, so
test_vamana_two_pass_build.phptasserts on upstream's ownCollectionSchema::to_string(), which embeds,two_pass_build:trueper field:test_vamana_prefetch.phptcovers both call orders, prefetch with nosetVamanaParams(),0/0, and negative rejection.Full suite:
test_hnsw_prefetch.phpt,test_vamana_index.phpt,test_vector_query_vamana_params.phptandtest_docs_consistency.phptpass unchanged.test_dbs/is left with only.gitignore.🤖 Generated with Claude Code