feat(collection): full-collection scans via iterDocs() / ZVecDocIterator (#226) - #240
Merged
Merged
Conversation
…tor (#226) There was no way to read every document in a collection. fetch() needs primary keys, queryByFilter() needs both a filter and a topk, and neither can walk a whole collection -- so export, backup and migration were not possible without one. foreach ($collection->iterDocs() as $pk => $doc) { ... } Backed by upstream's Collection::create_iterator() (zvec v0.7.0), exposed as Python does with iter_docs(). Constant memory regardless of collection size, over an isolated snapshot taken at call time. Ownership is the subtle part, and it is why this is split the way it is. The native iterator holds a std::shared_ptr to the collection, copied out of the collections registry at creation. Two consequences: - release_slot in upstream captures a raw Collection*, so freeing the handle underneath a live iterator would make it release a slot in freed memory. The shared_ptr makes that impossible: zvec_collection_free() only drops *our* reference, and the C++ collection stays alive until the last iterator goes. - Member order in DocIteratorHolder matters. Members are destroyed in reverse declaration order, so `collection` is declared first and `iterator` last -- the iterator is released first, giving its slot back, and the collection second. The other order would deadlock: upstream's ~CollectionImpl waits on a condition variable for every iterator to close, which in a single-threaded PHP process means waiting for something that can never happen. tests/test_doc_iterator_shutdown.phpt exists for exactly this -- the failure mode is a hang, not a crash, so nothing else would catch it. On the PHP side, ZVecDocIterator holds a reference to its ZVec object, so the collection cannot be collected out from under a live iterator. The upstream guards are surfaced rather than worked around: while an iterator is open, close(), destroy(), DDL and optimize() return FAILED_PRECONDITION and change nothing. That only works because of #222 -- before it, a rejected destroy() freed the native collection and the next call was a use-after-free. Writes, flush and queries are unaffected, and the test asserts that. Implemented as \Iterator rather than IteratorAggregate returning a Generator: a second getIterator() call on the same object would silently yield nothing from the exhausted handle, where rewind() failing loudly is better. Nullable normalization follows fetch() since #192, but only for the fields actually requested -- otherwise hasField('weight') would become true for outputFields: ['id'], which contradicts upstream's own expectations. Tests: 208/208, 0 skipped, 0 failed, 2 expected fail. - test_doc_iterator.phpt: empty collection, 50-doc scan, vectors on/off, outputFields, primary-key-only, nullable normalization, deleted documents skipped, snapshot isolation from later writes, auto-close on exhaustion, early close, forward-only rewind, and the three validation paths. - test_doc_iterator_lifecycle.phpt: the FAILED_PRECONDITION guards, the collection still usable after a rejected destroy(), writes during iteration, dropping the collection variable mid-iteration, destructor with an open iterator, and the lock being released afterwards. - test_doc_iterator_shutdown.phpt: clean process exit with both a collection and an open iterator alive. - test_memory_doc_iterator.phpt: no handle or char*[N] leak across early close, full scan and output-fields scans. - test_ffi_load.phpt gained the three symbols, test_null_handle_collection the three null cases.
RaBitQ is a lossy quantizer and IVF is approximate, so the exact top hit is not guaranteed. Measured 9/10 for the exact primary key and a 1-off otherwise, which made this test fail roughly 1 run in 10 in CI and forced a rerun on an unrelated PR. Asserts the winner is within 1 of the target instead, which is what the index actually promises. Both query paths are checked the same way. 12 consecutive runs green afterwards.
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 #226. Depends on #222, now merged as #238.
Summary
There was no way to read every document in a collection.
fetch()needs primary keys,queryByFilter()needs both a filter and atopk, and neither can walk a whole collection — so export, backup and migration were not possible at all.Backed by upstream's
Collection::create_iterator()(zvec v0.7.0), exposed the way Python does withiter_docs(). Constant memory regardless of collection size, over an isolated snapshot taken at call time.Ownership is the whole difficulty here
The native iterator holds a
std::shared_ptrto the collection, copied out of the collections registry at creation. Two consequences:Upstream's
release_slotcaptures a rawCollection*. Freeing the handle underneath a live iterator would make it release a slot in freed memory. Theshared_ptrmakes that impossible:zvec_collection_free()only drops our reference, and the C++ collection stays alive until the last iterator goes.Member order in
DocIteratorHolderis load-bearing. Members are destroyed in reverse declaration order, socollectionis declared first anditeratorlast — the iterator releases first (returning its slot), the collection second. The other order deadlocks: upstream's~CollectionImplwaits on a condition variable for every iterator to close, which in a single-threaded PHP process means waiting for something that can never happen.That failure mode is a hang, not a crash, so no ordinary test would catch it. Hence
tests/test_doc_iterator_shutdown.phptas its own file: the script ends with both a collection and an open iterator alive, and any wrong release order shows up as an infinite loop rather than a failing assertion.On the PHP side
ZVecDocIteratorholds a reference to itsZVecobject, so the collection cannot be collected out from under a live iterator.The upstream guards are surfaced, not worked around
While an iterator is open,
close(),destroy(), schema DDL andoptimize()returnFAILED_PRECONDITIONand change nothing. That only works because of #222 — before it, a rejecteddestroy()freed the native collection and the next call was a use-after-free. Writes,flush()and queries are unaffected, and the test asserts that too.Two smaller decisions
\Iterator, notIteratorAggregatereturning aGenerator. A secondgetIterator()call on the same object would silently yield nothing from the exhausted handle.rewind()failing loudly is better, andclose()/isClosed()live on the same object.Nullable normalization is limited to the requested fields.
fetch()has returned unset nullable fields as explicit null since #192, and iteration matches — but only for fields actually asked for. OtherwisehasField('weight')would betrueunderoutputFields: ['id'], which contradicts upstream's own behaviour.Verification
The lifecycle test covers the whole matrix, including the two cases that would be silent failures otherwise:
Full suite:
Four new tests plus extensions to
test_ffi_load.phptandtest_null_handle_collection.phpt.test_dbs/is left with only.gitignore.Note
Two values in the lifecycle test are
11rather than the10of the earlier iterator, because a document written after the first snapshot is visible to later ones. That is commented inline so the asymmetry does not look like an off-by-one.Out of scope, per the issue: filtered iteration (upstream has no filter in
IteratorOptions), resumable iteration, multi-threaded iteration, and the pre-existingchar*[N]leak intoCStringArray()for other callers —iterDocs()frees its own array.🤖 Generated with Claude Code