feat(query): fromId() works in every query entry point (#219) - #242
Merged
Merged
Conversation
ZVecVectorQuery::fromId() existed and was documented, but no entry point could run the query it built: - query(), queryWithReranker(), queryMulti() and groupByQuery() all threw "query() with docId not yet implemented" - queryVector() passed the handle straight through and upstream rejected it with "Invalid query: missing query clause", because the native handle carried no vector Only queryById() worked. All of them now fetch the document, read its vector and run an ordinary query -- the same client-side approach the Python SDK takes, since upstream has no native query-by-document-id and no id-based C API either. Doing it in C++ would be the same two steps with more plumbing, and it would need new FFI functions on every platform. The fetched vector is deliberately not written back into the query object, so it stays reusable: if the document changes, the next call must see the new vector. A test runs one object twice to pin that down, and asserts $q->vector is still []. queryVector() needed the vector written onto the native handle, which means an fp32/fp64 choice; int8 travels the fp32 path as it already does in queryById(). groupByQuery() only builds a float[] buffer, so it accepts fp32 and int8 and names the actual type for anything else rather than failing obscurely. fp16 has no setter on the native query handle at all, so query()/queryVector()/groupByQuery() point at queryById(), which does have a half-precision path. queryById() is unchanged. The fetch-and-resolve logic moved into a private fetchQueryVector() shared by both, so the four error messages stay in one place and both existing queryById tests pass untouched. Two behaviour changes worth calling out: - fromId() now rejects an empty $docId at construction instead of failing later with a "document not found" that named an empty string. - Setting both docId and a vector throws "Cannot provide both docId and vector" instead of "not yet implemented". The two existing test_query_decomposition tests asserted the old message and were updated. Tests: 212/212, 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 #219.
Summary
ZVecVectorQuery::fromId()existed and was documented, but no entry point could run the query it built:query(),queryWithReranker(),queryMulti(),groupByQuery()ZVecException: query() with docId not yet implementedqueryVector()Invalid query: missing query clause for field[embedding]Only
queryById()worked. All of them now work:Why client-side, not in C++
Upstream has no native query-by-document-id: the C++
SearchQueryhas no id field,QueryTargetholds only a vector or FTS clause, and the C API has no such function. The Python SDK resolves it client-side too —QueryExecutor.set_query_vector()fetches the document and puts its vector into the native query. Doing it in C++ would be the same two steps behind more plumbing, plus new FFI functions on every platform.The query object stays reusable
The fetched vector is not written back into the query object. If the document changes, the next call must see the new vector. The test runs one object twice and asserts
$q->vectoris still[]— a write-back would have passed the first two calls and been wrong afterwards.Precision, honestly bounded
queryVector()writes the vector onto the native handle, so it needs an fp32/fp64 choice. INT8 travels the fp32 path, as it already does inqueryById().groupByQuery()only builds afloat[]buffer, so it accepts FP32 and INT8 and names the actual type for anything else instead of failing obscurely.query()/queryVector()/groupByQuery()point atqueryById(), which does have a half-precision path. A missing document is not removed from the results, matching the Python SDK andqueryById().queryById()unchangedThe fetch-and-resolve logic moved into a private
fetchQueryVector()shared by both, so the four error messages live in one place. Both existingqueryByIdtests pass untouched — that is the refactor guarantee.Two behaviour changes
fromId()now rejects an empty$docIdat construction, rather than failing later with a "document not found" naming an empty string.docIdand a vector throwsCannot provide both docId and vector. The twotest_query_decompositiontests asserted the old "not yet implemented" message and were updated — they are the only existing tests this changes.Verification
Full suite:
No C++ change and therefore no rebuild needed.
test_dbs/is left with only.gitignore.Note
One bug I introduced and caught before committing: both
resolveQueryParams()andgroupByQuery()overwrite$fieldNamefrom the query object to a plain string early on, so aninstanceof ZVecVectorQuerycheck placed after that never fires. ThedocIdis now captured before the reassignment. Worth knowing if more code is added to those two methods.Out of scope per the issue: removing the source document from results, FP16 in
query()/queryVector()/groupByQuery(), sparse fields as the source,fromId()onZVecGroupByVectorQuery, and an end-to-end FP64 test — the v0.7.0 schema rejects denseVECTOR_FP64outright, which is why the FP64 test scripts are marked XFAIL.🤖 Generated with Claude Code