evalengine, mysql/json: fix JSON and TIME equality in compiled and interpreted paths - #20718
evalengine, mysql/json: fix JSON and TIME equality in compiled and interpreted paths#20718arthurschreiber wants to merge 8 commits into
Conversation
Value.Hash was implemented as the value's weight string, and MySQL defines JSON weight strings to fingerprint arrays and objects by their kind and cardinality alone so that they sort by length. Every consumer of the hash treats a hit as equality without comparing the candidate, so any two arrays of the same length were considered equal, as were any two objects with the same member count. Bit, Blob and Opaque values shared a tag too, so those collided across types whenever their payloads matched. Hash now descends into arrays and objects and leads with the value's type, while WeightString keeps the shape MySQL requires for ordering. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
arthurschreiber
added
Component: Evalengine
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20718 +/- ##
===========================================
+ Coverage 69.67% 79.57% +9.90%
===========================================
Files 1614 151 -1463
Lines 216793 30349 -186444
===========================================
- Hits 151044 24151 -126893
+ Misses 65749 6198 -59551
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Both probe tables treat a 128-bit hash hit as equality without comparing the keys, so they inherited the JSON weight string's habit of fingerprinting arrays and objects by cardinality alone: DISTINCT collapsed same-length documents into one row and hash joins emitted spurious matches. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
|
I like the direction of fixing the shared hash rather than disabling its consumers, but I think the equality encoding may need one more pass. I found two unequal values that still receive the same hash. High-precision numbers are rounded through -- column0 contains JSON 9007199254740992.0
column0 IN (CAST('9007199254740992.1' AS JSON))The interpreter returns Scalar values also aren’t length-framed in the recursive stream, so these unequal arrays hash identically: ["", "X\u0002\u0000\u0004Y"]
["\u0002\u0000\u0004X", "Y"]Would it make sense to hash numbers from the same exact-decimal representation used by comparison, and length-prefix scalar weights—or otherwise make each child encoding self-delimiting? Adding these cases to |
Hashing a container is now proportional to its size, so the cost is worth tracking. The scratch buffer threaded through the recursion is what keeps allocations flat: without it a 1024-element array allocates once per element. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Deriving the hash from the weight string kept leaking sort-key shortcuts into equality. Three collisions came from it: numbers written with a fraction or an exponent were rendered through float64, so any two that agreed to that precision shared a fingerprint; scalar payloads were not length-framed, so an element could spell out the tags framing its neighbours; and TIME '-00:00:00' collapsed onto TIME '00:00:00', which Time.Compare orders apart. Ordering and equality are separate jobs and want separate encodings. Hash now derives everything from the comparison side: each branch fingerprints a value through the representation compareJSONValue compares it by, and never calls WeightString. Numbers go through the exact decimal, with integral spellings canonicalised directly; strings, blobs, bit strings and opaque values through their unencoded bytes; temporals through the datetime package's own hashes. Dropping the weight string also removes every allocation the recursion needed, so the scratch buffer is gone with it. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
97710d0 to
ff2b299
Compare
arthurschreiber
removed
NeedsDescriptionUpdate
arthurschreiber
added
Backport to: release-23.0
Hashing a number went through decimal.NewFromString and Decimal.String, which allocates four times per value: a string to strip the decimal point, a big.Int for the coefficient, a buffer to render it back, and a copy of that buffer. An array of fractional numbers paid that per element. Two decimals are equal exactly when they share a sign, a run of significant digits with no leading or trailing zeros, and the power of ten that run scales by, and all three can be read straight off the text. The digits stay as substrings of the spelling, so nothing is concatenated and nothing is allocated. TestNumberHashMatchesDecimalComparison is the safety net: it walks a decimal point across a set of coefficients, crosses that with exponent forms and signs, and asserts pairwise over the result that hash equality matches Decimal.Cmp equality. Hashing is now allocation-free for every JSON type, and three times faster on arrays of fractional numbers. Arrays of plain integers pay around a third more, because the canonical form writes a sign marker and digit count per number where the previous integral shortcut wrote only the digits. The benchmark set grows fractional arrays, which it was missing. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
| // spelling of one value fingerprints alike while two values that differ only | ||
| // past the precision of a float64 do not. | ||
| func (v *Value) hashNumber(h *vthash.Hasher) { | ||
| if hashDecimalText(h, v.s) { |
There was a problem hiding this comment.
Would it make sense to make the equality-hashing path error-returning here? hashDecimalText can currently recognise values that compareJSONValue rejects. For example, 1e1025 is accepted and fingerprinted here, while Decimal() returns DECIMAL value is out of range during comparison. Since the compiled IN table treats a hash hit as the complete equality result, that gives different results for:
-- column0 contains JSON 1e1025
column0 IN (CAST("1e1025" AS JSON))The interpreter returns the out-of-range error, while the compiled form returns 1.
Perhaps the boolean from hashDecimalText could mean “this is accepted by the comparison representation,” including the decimal exponent/range checks, and failure could be propagated rather than falling back to another fingerprint:
func (v *Value) Hash(h *vthash.Hasher) error {
h.Write16(hashPrefixJSON)
return v.hash(h)
}
func (v *Value) hashNumber(h *vthash.Hasher) error {
if !hashDecimalText(h, v.s) {
return vterrors.NewErrorf(
vtrpcpb.Code_INVALID_ARGUMENT,
vterrors.DataOutOfRange,
"DECIMAL value is out of range",
)
}
return nil
}The recursive container branches could propagate that error, and the hash consumers could stop before probing their tables:
if err := lhs.(hashable).Hash(&env.vm.hash); err != nil {
env.vm.err = err
return 1
}
_, in := table[env.vm.hash.Sum128()]That is a wider interface change, so a JSON-specific error-returning helper might be less disruptive. I think the important part is that a number the comparator rejects cannot produce a successful equality hash.
There was a problem hiding this comment.
You're right that this is an issue, but I don't think we want to change the .Hash signature in this PR, and I'm not sure it's really the correct place to fix this.
From what I understand, MySQL rejects these invalid numbers at parse time, so they can never end up being an issue at execution time. Maybe we should be doing the same?
There was a problem hiding this comment.
Yep, I think parse-time rejection is the cleaner place to address this. I confirmed that MySQL rejects 1e1025 and opened #20720 to track bringing Vitess's parser in line. Since those values should not reach execution once that is fixed, I don't think we need to widen Hash or keep adding scope to this PR.
| // | ||
| // The type leads because comparison orders by type before it looks at any | ||
| // value, which is also why Bit, Blob and Opaque are three tags and not one. | ||
| typ := v.Type() |
There was a problem hiding this comment.
Could we normalize folded JSON strings before this value becomes shared, rather than calling the mutating Type() while building the table? Type() lazily rewrites both v.s and v.t, and unescapeStringBestEffort also rewrites the parser buffer in place.
A cached UntypedExpr can compile different runtime bind-type specializations concurrently. Each specialization has its own sync.Once, but they all share the same folded RHS literal, so an expression along these lines can hash its nested strings from multiple goroutines:
CAST(:v AS JSON) IN (
CAST('["\u0061", "\u0061", ...]' AS JSON)
)On this head I could reproduce races between Value.Type() and Metro128.WriteString; repeated runs also cached a false result for an equal JSON value and occasionally panicked on a torn string header. The identical nested-container probe is clean on the base, where hashing does not visit the children.
Perhaps the folded JSON tree could be recursively normalized during constant folding, while it is still exclusively owned, before evalToIR publishes it. That would leave hashing and comparison read-only once the plan is shared.
| negative, rest = true, rest[1:] | ||
| } | ||
|
|
||
| if rest == "" || len(rest) > 9 || !allDigits(rest) { |
There was a problem hiding this comment.
Could we strip exponent-leading zeroes before applying this length guard? MySQL 8.4 accepts these as ordinary JSON numbers:
1e0000000000 -> 1.0
1e-0000000000 -> 1.0
1e0000000001 -> 10.0
Vitess's decimal comparison likewise considers the first two equal to 1, but parseExponent rejects their ten-digit spelling and sends them through numberHashUnparsed. For example:
-- column0 is VARCHAR containing 1e0000000000
CAST(column0 AS JSON) IN (CAST(1 AS JSON))returns 1 in the interpreter and 0 in the compiled expression on this head. The base returns 1 in both paths.
Would something like this preserve the intended overflow guard while making it depend on the exponent's value rather than its spelling?
if rest == "" || !allDigits(rest) {
return 0, false
}
rest = strings.TrimLeft(rest, "0")
if rest == "" {
return 0, true
}
if len(rest) > 9 {
return 0, false
}This seems separate from the existing out-of-range-number thread: these values are valid and comparison succeeds; they just need equal spellings to reach the same hash.
MySQL fixes the form of a JSON number when the document is built, not when it is compared. An integer that fits stays exact; everything else becomes a double and loses its precision there and then. Comparison is exact over whatever was kept, so 9007199254740992.1 equals 9007199254740992 because both are the same double, while 9007199254740993 does not because it stayed an integer. compareJSONValue read the decimal straight off the text instead, keeping a precision MySQL had already discarded, and reported values unequal that MySQL calls equal. Separately, a zero time carries no sign. Time.Compare tested the sign before the magnitude, so TIME '-00:00:00' sorted below TIME '00:00:00' where MySQL reads them as one value. That reached plain SQL comparisons, not just JSON. Time.Hash tested nothing at all and wrote the sign bit straight out, so it disagreed with its own Compare; both now drop the sign when there is no magnitude for it to apply to. The comparisons are pinned against MySQL by JSONNumberComparison and JSONTimeComparison, which cross a corpus of numbers and times with themselves over both = and <. TestJSONHashMatchesComparison fails on two number pairs at this commit: the JSON hash still fingerprints numbers from their text, and follows in the next one. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The previous commit made comparison read a number's stored form rather than its text. The fingerprint has to read the same thing, or the compiled IN table answers questions the interpreter answers differently: it still separated 9007199254740992.0 from 9007199254740992.1, which are one double and so one value. Both now go through Value.NumericValue, which is the single statement of what form a number is kept in. Hashing works from the same form without building a decimal: an integer is already its own value, a double canonicalises through the shortest text that round-trips it, and a decimal carried over from a SQL value stands for itself. Doubles pay for the round trip through their text, so hashing an array of fractional numbers is roughly two and a half times the cost of hashing one of integers. It stays allocation-free. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
arthurschreiber
changed the title
hashDecimalText is covered directly, but nothing exercised the path a parsed document actually takes through hashNumber, which is where a spelling can survive as far as the fingerprint. A zero-padded exponent used to reach the canonicaliser verbatim and be turned away for being long, so 1e0000000000 and 1 fingerprinted apart while comparison called them equal. Routing numbers through their stored form fixed that on the way past; this pins it. The test groups spellings into equality classes and checks both directions, so it also records that precision beyond the form a number is kept in is not part of its value: 9007199254740993.0 belongs with 9007199254740992 because it is that double, while the integer 9007199254740993 stands alone. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
| return decimal.NewFromUint(u), ok | ||
| case NumberTypeFloat: | ||
| f, ok := v.Float64() | ||
| return decimal.NewFromFloat(f), ok |
There was a problem hiding this comment.
Could we keep non-finite SQL floats from becoming JSON numbers before this call, and make this method defensive too? A FLOAT64 bind containing NaN, +Inf, or -Inf passes bind validation, and CAST(:v AS JSON) = CAST(0 AS JSON) currently panics in decimal.NewFromFloat.
The local guard seems worth having:
f, ok := v.Float64()
if !ok || math.IsNaN(f) || math.IsInf(f, 0) {
return decimal.Zero, false
}
return decimal.NewFromFloat(f), trueI don't think that is sufficient by itself, though. With :v = NaN, this still gives an interpreter error but compiled IN = 1 after applying only the guard:
CAST(:v AS JSON) IN (
CAST(CAST('NaN' AS DOUBLE) AS JSON)
)Both hashes fall back to the same unparsed spelling. Would it make sense to reject non-finite values in the SQL-float-to-JSON conversion, including the VM path, before calling NewNumber, while keeping this guard as a backstop?
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
github-actions
Bot
added
the
Stale
Description
Two things decide whether two JSON values are the same:
compareJSONValue, and the 128-bit fingerprint that the compiledINtable, theDISTINCTprobe table and the hash join probe table use in place of comparing. Both were wrong, in different directions, and they were wrong about different values — so the compiled and interpreted paths disagreed.The fingerprint was derived from the weight string. Weight strings exist to order JSON values, and ordering takes shortcuts that equality cannot: arrays and objects are fingerprinted by their kind and cardinality alone. Any two arrays of the same length were treated as equal, as were any two objects with the same member count. With a JSON column holding
[1], compiledjson_col IN (JSON_ARRAY(2))returned 1 where the interpreter and MySQL return 0. Scalar payloads were also unframed, so an element could spell out the tags separating it from its neighbours:["", "X\x02\x00\x04Y"]and["\x02\x00\x04X", "Y"]produced the same byte stream.The comparison read numbers from their text. MySQL fixes the form of a number when the document is built, not when it is read: an integer that fits stays exact, and everything else becomes a double, losing precision there and then. Comparison is exact over whatever was kept. So
9007199254740992.1and9007199254740992are one value — the same double — while9007199254740993and9007199254740993.0are two, because the first stayed an integer. Reading the decimal off the original text keeps a precision that has already been discarded, and reported values unequal that MySQL calls equal.And a zero time carried a sign.
Time.Comparetested the sign before the magnitude, soTIME '-00:00:00'sorted belowTIME '00:00:00'where MySQL reads them as one value. That reached plain SQL comparisons, not only JSON.Time.Hashtested nothing and wrote the sign bit straight out, so it disagreed with its ownCompare—DISTINCTover a TIME column split the two apart.After this, ordering and equality have separate encodings, and equality has one definition that both paths read:
Value.Hashno longer touchesWeightString, which keeps the shape ordering needs. It descends into containers, leads with the type, and length-frames variable-width payloads so a payload cannot be confused with the tags around it.Value.NumericValueis the single statement of what form a number is kept in.compareJSONValuecompares it;Value.Hashfingerprints it.datetime.Timedrops the sign, in bothCompareandHash, when there is no magnitude for it to apply to.Three consumers treat a hash hit as equality with no verifying compare, and all three are fixed: the compiled
INtable (evalengine/compiler_asm.go), theDISTINCTprobe table (engine/distinct.go), and the hash join probe table (engine/hash_join.go).These fingerprints never leave the process — the probe tables and the compiled
INtable are all per-execution — so there are no wire or upgrade/downgrade implications.This overlaps with #20691, which disables the static
INtable for JSON operands instead. That fixes the compiledINcase only, gives up the fast path, and in its current position makes JSONINover folded literals fail to compile until #20682 lands. Fixing the fingerprint keeps the fast path and coversDISTINCTand hash joins as well.Reading the commits
The history is additive and the middle of it argues for two things the end reverses:
46ce4e698eandf3bab9da85fingerprint numbers from their exact text, andff2b299e78splitsTIME '-00:00:00'fromTIME '00:00:00'. Both looked right against Vitess's own comparison and turned out to be wrong against MySQL, which is whata1ba5dbe8aanda58e7628afcorrect. The end state is what matters; reviewing the diff whole will be less confusing than reading commit by commit.Backport justification
Labelled for release-23.0 and release-24.0. Every change here is a wrong-answer fix against MySQL, not a change of intent:
DISTINCTover a JSON column drops rows. Any two arrays of the same length, or objects with the same member count, collapse into one group, soSELECT DISTINCT json_colsilently returns fewer rows than it should.INover a JSON column returns 1 for a value that is not in the list.DISTINCTover aTIMEcolumn splits-00:00:00from00:00:00, and plainTIME '-00:00:00' = TIME '00:00:00'returns 0 where MySQL returns 1.None of these raise an error or a warning. They are silently incorrect results, and in the
INand hash join cases the compiled and interpreted paths disagree with each other, so the same query can answer differently depending on whether it was compiled — which makes the wrong answer intermittent rather than reproducible.The behaviour changes are all in the direction of MySQL, and are checked against a live MySQL by the differential cases in the integration suite rather than against our own expectations. The risk of backporting is that a query relying on the current answers changes: JSON numbers differing only past their stored precision become equal, and zero times stop sorting apart. In both cases the current answer is the divergent one.
The widest-reaching piece is the
datetime.Timechange, since it touches plain SQLTIMEcomparison and not only JSON. If that is too broad for a release branch on its own terms, it is cleanly separable from the rest and can be backported or held independently.Related Issue(s)
Alternative to #20691.
Related to #20720. That issue asks that no comparison-invalid JSON number reach either execution path. #20722 handles it at the parser, rejecting the documents MySQL rejects, but a document can be valid and still carry a number
decimal.NewFromStringrefuses —1e-1025, which MySQL reads as zero, or0.followed by 800 zeros and1e1100, whose double is an ordinary1e299. Both error in the interpreter while the compiled path reads a fingerprint instead. Comparing the stored double rather than the written text is what settles those, so #20720 needs this PR as well as #20722. The two are independent and can merge in either order.Tests
JSONNumberComparisonandJSONTimeComparisoncross a corpus of numbers and times with themselves over=and<, checked against a live MySQL by the integration suite. The time cases cover plain SQLTIMEcomparison as well as JSON.TestJSONHashMatchesComparisoncross-checks the fingerprint againstcompareJSONValueover every pair of a document matrix: same-length differing arrays, member-order-independent objects, nested containers, payloads that spell out their neighbours' tags, numbers that are one double against numbers that stayed integers, dates and datetimes and times including negative zero, and Bit against Blob against Opaque with identical payloads.TestNumberHashMatchesDecimalComparisonwalks a decimal point across a set of coefficients, crosses that with exponent forms and signs, and asserts pairwise that the canonicalisation agrees with decimal comparison.TestCompareNegativeZeropins that a zero time compares and hashes without a sign, and that a time with a magnitude still keeps one.TestCompiledJSONInListcompares compiledINandNOT INover JSON literals against the interpreter.TestDistinct/json arrays and objects that share a shape but not a valueandTestHashJoinJSONKeyscover the two probe tables, on both the Execute and StreamExecute paths.TestJSONHashIgnoresLazyUnescapingpasses onmaintoo, so it is not evidence of a fix. It guards a detail the recursion depends on: parsing leaves a string in a raw kind thatType()rewrites in place on first use, and a draft of this change read the type field directly and silently broke JSON string equality.go/mysql/datetime,go/mysql/json,go/vt/vtgate/evalengine,go/vt/vtgate/evalengine/integrationandgo/vt/vtgate/enginepass locally.Benchmarks
BenchmarkValueHash, arm64,GOMAXPROCS=4,-count=6, via benchstat. Fingerprinting a container used to be O(1) because it only looked at the kind and the cardinality, which is precisely the bug, so the before column is the cost of being wrong rather than a baseline worth preserving:Scalars get cheaper: they no longer build a weight string first. Containers now cost in proportion to the document, which is unavoidable — nothing can fingerprint a document correctly without reading it. Doubles additionally pay for a round trip through their text, which is what reproduces the precision MySQL stored them with, so an array of fractional numbers costs about three times one of integers. For scale, parsing these same documents costs 12µs (array/1024), 1.9µs (object/64) and 141ns (nested).
Working from the comparison primitives rather than through a weight-string buffer makes the whole recursion allocation-free:
Checklist
Deployment Notes
Three classes of query change their answers, all of them toward MySQL:
IN,DISTINCTand hash joins over JSON no longer report false matches for documents that share a shape but not a value.9007199254740992.1and9007199254740992, or0.1and0.10000000000000000000001— are now equal, where Vitess previously reported them different.TIME '-00:00:00'now equalsTIME '00:00:00', in plain SQL comparisons as well as in JSON, and the two no longer land in separateDISTINCTgroups.No migrations or configuration changes.
AI Disclosure
Claude Code wrote this one, including the tests and benchmarks — I reviewed it and provided direction. It came out of reviewing #20691.