Map payloads between JSON and Qdrant's protobuf Value - #58
Merged
Conversation
The first piece of the gRPC transport that everything else needs, and the one place where the two wire formats genuinely disagree. JSON has one number type. Qdrant forked google.protobuf.Value specifically to carry integer_value separately from double_value, because a vector database that rounds an id to a double loses it. So the mapping decides integral or fractional from the text rather than from what the text parses to. That distinction is not academic, and the test found it. Checking longOrNull first looks correct and is not: a thirty digit id is too large for Long but parses as a Double perfectly happily, so the check falls through to the double branch, rounds, and the write succeeds having quietly changed the number. A value that fits neither is rejected. Rounding it or turning it into a string both make the write succeed and the data mean something else, which is then found a long way from here. The message says the REST engine has no such limit, because JSON has no int64 ceiling, so there is somewhere to go. In the other direction a Value with no variant set decodes to null rather than failing. The proto calls that an error, but it is one field of a response that otherwise arrived intact, and losing the whole response over it helps nobody.
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.
Second slice of #52, after #54. The payload mapping every other gRPC operation needs, and the one place
the two wire formats genuinely disagree.
JSON has one number type. Qdrant forked
google.protobuf.Valuespecifically to carryinteger_valueseparately from
double_value, because a vector database that rounds an id to a double loses it.The bug the tests found
Checking
longOrNullfirst looks correct and is not:A thirty-digit id is too large for
Longbut parses as aDoubleperfectly happily, so the check fallsthrough to the double branch, rounds, and the write succeeds having quietly changed the number. The
mapping now decides integral or fractional from the text rather than from what the text parses to, and
there is a test asserting that the huge value really does parse as a finite double, so the reason the
naive check fails is written down rather than rediscovered.
A behavioural difference between the engines, on purpose
A value that fits neither
int64nordoubleis rejected. Rounding it or turning it into a stringboth make the write succeed and the data mean something else, found later and far from the cause. The
message names the way out: the REST engine has no such limit, because JSON has no int64 ceiling.
This is the first place the two engines will not accept the same input. Worth knowing now rather than
discovering after someone has switched.
In the other direction a
Valuewith no variant set decodes tonull. The proto comment calls that anerror, but it is one field of a response that otherwise arrived intact, and failing the whole response
over it helps nobody.
Scope
Ten tests, covering both directions, nesting at depth, empty containers, explicit nulls, strings that
look like numbers, and
Long.MAX_VALUEat the boundary. Still noQdrantTransportimplementation —that needs the point, vector and filter mappings on top of this one.