Fix the five gaps that were filed instead of fixed - #157
Merged
Conversation
Filing these was the wrong call on at least two of them, and the reasoning does not survive being written down. #152 and #154 were loose ends of work that had just landed: the contract test's missing operations only became visible because the guard started naming what it covers, and the classifier copies were hand-fixed on both sides rather than unified. That is not backlog, it is half-finished. #154, the cause rather than the symptom. Each engine carried its own copy of the matchers that read a downed shard or a read-only node out of an error message, and the copies diverged: one learned "timed out" and not "timeout", so a transient cluster state was reported as terminal. They are in kdrant-core now, beside the exception types whose retryable flag they decide, behind the same opt-in annotation KdrantJson uses. The table of phrasings is asserted once, and each engine keeps only the test for what it does with the answer, because a REST status code and a gRPC Status.Code are different decisions. #152. searchMatrixPairs, searchMatrixOffsets and updateCollection send request bodies and were absent from a test whose own name says otherwise. Forty bodies now validate against Qdrant's published schema. #149. updateCollection carries params, so replication, write consistency, read fan-out and payload placement can change after a collection exists. The last is why it mattered: memory tiers arrived in 1.19 and placement could only be chosen at creation, so moving a collection from cold to cached meant recreating it. A field left out of the diff stays as it was, and the contract asserts that rather than trusting it, because a client that sends a full params object silently resets what the caller did not mention. #151. enableHnsw on all eight payload index types, which the issue had as two: it decides whether Qdrant builds the extra links that make a filtered search read the matching points rather than walk the collection, and it needs payloadM above zero, so a caller could ask for half of a multi-tenant layout. The text index takes a Snowball stemmer for nineteen languages, the explicit Disabled that 1.19 added in place of the old "none" hack, and asciiFolding. #150. Product and TurboQuant quantization, beside the scalar and binary that were there. And the read side, which is the half that decides recall: a collection quantized to a quarter of its size answers from the approximation unless a search asks for the originals, so params { rescore(oversampling = 2.0) } exists and a caller who never set it had traded accuracy they did not choose to trade. One thing found and not fixed. getCollection exposes a collection's vector and sharding parameters and nothing else, so quantization, the HNSW config and strict mode cannot be read back at all. The contract case for the quantization families therefore asserts acceptance rather than a round trip, and says so. Making them readable means a deserializer for a sealed type that is currently write-only, and it is the read model rather than this. Every builder's build() went to named arguments on the way through, which is the thing that would have caught the one compile error this change produced: a new field landing in the slot of the one beside it.
|
This pull request adds no capability the code did not already have. What I could not follow47 calls resolve to a name whose type is written nowhere, so what they reach is not established here, and 49 more are in tests and benchmarks, which this report does not judge. Across everything it read that is 96 of 3539, 3%. That is too many to list, and where they are is more useful than which they are:
If one of those reaches the network or starts a process, this report does not show it. 23 files read · 19117 ms · against |
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 #149, closes #150, closes #151, closes #152, closes #154.
Five gaps that were filed as issues a few hours ago and should have been fixed then. On two of them the
reasoning does not survive being written down: #152 and #154 are loose ends of work that had just landed,
not backlog.
The cause, not the symptom (#154)
Each engine carried its own copy of the matchers that read a downed shard or a read-only node out of an
error message. The copies diverged: one learned
timed outand nottimeout, so1 of 1 read operations failed: Timeout error: Deadline Exceededwas reported as terminal when it clearsin seconds. Both copies were fixed by hand, which left the cause in place.
They are in
kdrant-corenow, beside theKdrantExceptiontypes whoseretryableflag they decide,behind the same
@InternalKdrantApithatKdrantJsonuses. The table of phrasings is asserted once, andeach engine keeps only the test for what it does with the answer, because a REST status code and a gRPC
Status.Codeare different decisions.The test that promised more than it covered (#152)
searchMatrixPairs,searchMatrixOffsetsandupdateCollectionsend request bodies and were absent froma test called the operations covered here are the ones the engine can send a body for. It only became
visible because the guard started naming its operations instead of counting them. Forty bodies now
validate against Qdrant's published schema.
What could be created and never changed (#149)
updateCollectioncarriesparams, so replication factor, write consistency, read fan-out and payloadplacement can change after a collection exists. The last is why it mattered: memory tiers arrived in 1.19
and
payloadMemoryset placement at creation, so moving a collection fromcoldtocached, which is thewhole point of a tier, meant recreating it.
A field left out of the diff stays as it was, and the contract asserts that rather than trusting it,
because a client that sends a full params object silently resets what the caller did not mention.
The index options that decide whether a filter is fast (#151)
enableHnswon all eight payload index types, which the issue had as two. It decides whether Qdrant buildsthe extra links that make a filtered search read the matching points rather than walk the collection, and
it needs
payloadMabove zero to do anything, so a caller could previously ask for half of a multi-tenantlayout and get no warning.
The text index takes a Snowball stemmer for nineteen languages, the explicit
Disabledthat 1.19 added inplace of the old
"none"hack, andasciiFolding. Stemming is asserted by behaviour against a realserver, searching
runand matchingrunning, becausegetCollectionreports an index's data type and notthe parameters it was built with.
The quantization families, and the half that decides recall (#150)
QuantizationConfig.Producttakes a compression ratio andQuantizationConfig.Turbotakes one ofTurboQuant's four bit sizes, beside the scalar and binary that were there.
And the read side:
params { rescore(oversampling = 2.0) }preselects from the quantized index and ranksthat larger set against the originals. A collection quantized to a quarter of its size answers from the
approximation unless a search asks otherwise, so a caller who never set this had traded accuracy they did
not choose to trade.
One thing found and deliberately not fixed
getCollectionexposes a collection's vector and sharding parameters and nothing else. Quantization, theHNSW config, the optimizer config and strict mode cannot be read back at all, so a caller can set them and
never verify them. The contract case for the quantization families therefore asserts acceptance rather
than a round trip, and says so in the test.
Closing that means a deserializer for
QuantizationConfig, which is write-only today, and it is the readmodel rather than this change. Raised rather than filed, because the last thing this branch should do is
open another issue.
Verification
./gradlew buildgreen,koverVerifyincluded. Forty request bodies validate against Qdrant's own v1.19.1OpenAPI document. The three new shared-contract cases were run against a real Qdrant 1.19.1 on loopback
before this was committed, and they run over both engines in CI.
Every builder's
build()went to named arguments on the way through, which is what would have caught theone compile error this change produced: a new field landing in the slot of the one beside it.