mysql/json: settle values when parsing, so reading one cannot change it - #20723
Draft
arthurschreiber wants to merge 9 commits into
Draft
mysql/json: settle values when parsing, so reading one cannot change it#20723arthurschreiber wants to merge 9 commits into
arthurschreiber wants to merge 9 commits into
Conversation
MySQL decides a JSON number's storage when it parses the document: an
integer that fits stays exact and everything else becomes a double. A number
too large for a double therefore has nowhere to live, and MySQL calls the
whole document invalid — JSON_VALID('1e1025') is 0, and inserting it into a
JSON column is refused. Vitess parsed it happily and left every consumer to
cope with a value that should not exist, which is how the same expression
could error in the interpreter and answer 1 in the compiled form.
Underflow is not rejected, matching MySQL: 1e-1000 is a valid document that
reads as zero.
The check is kept off the common path. readFloat already walks the exponent,
so it reports whether one was written, and a number without one has to be
longer than the largest double before it can overflow. Parsing arrays of
integers and of fractional numbers both measure unchanged.
Also sets the number type on the NaN branch, which left whatever the value
cache last held in that field.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
A written sign, a zero-padded exponent and an underflowing one are all spellings rather than magnitudes, and all stay valid. They go through the conversion because they carry an exponent, so they are the cases most likely to be caught by mistake. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
arthurschreiber
added
Component: Evalengine
vitess-bot
Bot
added
NeedsWebsiteDocsUpdate
Contributor
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 #20723 +/- ##
===========================================
+ Coverage 69.67% 72.88% +3.21%
===========================================
Files 1614 6 -1608
Lines 216793 1704 -215089
===========================================
- Hits 151044 1242 -149802
+ Misses 65749 462 -65287
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:
|
arthurschreiber
added
Backport to: release-23.0
A JSON number carries two ways of being too big, and only one of them shows up after conversion. MySQL bounds the exponent a number is written with before it converts anything: the decimal point can travel 308 places, plus one for every digit the number already has after it. 0e309 is refused on those grounds even though it is zero, while 0.00e310 is fine because its two fraction digits buy the places back. Converting caught the second way and missed the first, so every exponent that lands on zero came through: 0e309, 0.0e310, 0.00e311. Converting is still needed for the other direction — 10e308 is written within the bound and overflows anyway. readFloat already walks both the fraction digits and the exponent, so it reports them rather than reporting that an exponent was there at all. A written exponent stops at a ceiling that clears the largest double plus every digit the number could hold after its point, so a long one neither overflows nor changes the answer. Negative exponents are left alone, matching MySQL: underflow is a valid document that reads as zero. Parsing arrays of integers, of fractional numbers and of numbers with exponents all measure unchanged. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The number reader was a general-purpose one: it took a written plus, an
integer part opening with a zero, and a decimal point with nothing on one
side of it. JSON allows none of those and MySQL rejects all of them, so
Vitess answered questions MySQL refuses to answer — JSON_EXTRACT('{"a": .2}',
'$.a') is 0.2 here and an error there. Which one a query gets depends on
whether it is evaluated in vtgate or pushed down, so the same query fails in
one place and succeeds in the other. A number written 007 also kept its
leading zeros all the way back out again.
Reading the grammar directly turns out to be less work than the loop that
read around it: the leading-zero bookkeeping and the labelled switch go away,
and parsing an array of integers or of fractional numbers is around a fifth
to a quarter faster.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The parser carried a branch for nan, from the JSON reader it grew out of. It
is an extension to JSON rather than part of it, and MySQL does not take it:
JSON_VALID('nan') is 0. A document Vitess accepted here was one no MySQL
column could hold, and 'nan' as a JSON bind value reached the evalengine as a
number no consumer could read — the interpreter reports a decimal range error
where the compiled form compares two of them equal.
Deleting the branch also removes the number type it had to set, which was
whatever the value cache last left there.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
5 tasks
Whether a JSON number is too big is not a question about the number, it turns out — it is a question about how the number was written. MySQL parses JSON with RapidJSON, which splits the digits between a significand it accumulates into a double and a power of ten to scale that by, and the split decides which way the last place rounds. 1.7976931348623158e308 does not fit. The same value written 1.79769313486231580e308 does, because the extra digit moves the split. Comparing a correctly converted value against the largest double cannot tell those two apart, so it took the first one as well. So the conversion is now RapidJSON's rather than a correct one, deliberately landing an ULP or two off the true value so the boundary lands where MySQL's does. The written-exponent bound moves into it as well, since that is where RapidJSON applies it, leaving one place that decides whether a document holds a number instead of two that had to agree. Checked against MySQL 8.0.45, 8.4.11 and 9.4.0 over 1279 documents — both sides of the boundary at every spelling above, the exponent bound stepped one place at a time, and 900 random numbers, a fifth of them crowded up against the largest double. All three versions agree with each other and with this, and 8.4 still parses with the same flags and the same code. The check runs only for numbers whose digits could reach that far, as before, and reaching the answer in one pass rather than converting the whole number makes those about a third cheaper. Numbers that cannot reach it are untouched. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The conversion accumulates the significand into a double one digit at a time, multiplying and adding. MySQL's builds round the two operations separately, but written as d*10 + digit the Go compiler is free to fuse them into one FMA on arm64, which rounds once and can land the accumulation an ULP from where MySQL puts it — enough to flip which side of the largest double a number falls on. MySQL 8.0.45, 8.4.11 and 9.4.0 all accept 17976931348623154547712857878e280; on arm64 the fused loop rejected it. The explicit float64 conversion forces the intermediate rounding that the spec otherwise lets the compiler discard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Type() and NumberType() cached their work into the value they were reading: Type() unescaped a string in place and rewrote its kind, NumberType() wrote back the kind it had just worked out. A parsed document is shared by every goroutine running a cached plan, so a read that rewrites what it read is a data race. Hashing a shared literal from several goroutines reports races on both, and produced torn reads and a wrong cached answer. Parsing now settles both. Strings are unescaped as they are read, the way object keys already were: parseRawValueString reports whether escapes were seen, from the scan it was doing anyway. A number's kind is decided from the shape readFloat already saw, so a fraction or an exponent answers the question outright and a short run of digits answers it without converting anything; only integers long enough to straddle the limits are converted, and those once rather than once per kind. Both lazy sentinels are gone, and Type() and NumberType() are plain reads. This also settles what a string renders as. MySQL resolves an escape when it parses, so a unicode escape prints as the character it names; Vitess printed it verbatim if nothing had called Type() and unescaped if something had, which made the output depend on what the rest of the query happened to touch. Parsing a document of strings without escapes gets faster. Escaped strings pay the unescape up front rather than on first read, and long integers pay a conversion they used to defer; BenchmarkParse covers both. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
arthurschreiber
force-pushed
the
arthur/json-immutable-values
branch
from
July 28, 2026 10:16
6da804d to
770f8c7
Compare
5 tasks
Settling a number's kind at parse time left parseNumberType with no production caller. The one reader left is the test that checks the shape rule against the conversions, where it serves as the oracle, so it lives with that test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
| @@ -1195,10 +1257,6 @@ func (v *Value) Type() Type { | |||
| if v == nil { | |||
Collaborator
There was a problem hiding this comment.
We should update this concurrency contract now that parsing settles the value completely. The cached-plan use case in this PR deliberately shares one parsed value between concurrent readers, so saying that Value cannot be used concurrently seems to contradict the behavior being established.
Perhaps something like:
// Value may be read concurrently once parsing is complete.
// Concurrent mutation remains unsafe.The equivalent warning on Object should have the same read-only distinction.
Contributor
|
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
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.
Description
Value.Type()andValue.NumberType()cached their work into the value they were reading.Type()unescaped a string in place and rewrote its kind;NumberType()wrote back the kind it had just worked out. A parsed document is shared by every goroutine running a cached plan, so a read that rewrites what it read is a data race.Hashing one parsed document from eight goroutines, with
-race, over five runs:The last two race on
mainas well —WeightStringcalls bothType()andNumberType()on the value it is given. Containers are clean there only because it reads their length without descending into them. #20718 descends, which turns "a bare JSON literal" into "every value in the tree" and is what made this easy to hit; @GrahamCampbell reported it there, having reproduced torn reads and a wrongly cached result.Parsing now settles both, so
Type()andNumberType()are plain reads and a parsed value never changes again:parseRawValueStringmirrorsparseRawKey: it reports whether escapes were seen, from the scan it was doing anyway.readFloatalready saw. A fraction or an exponent answers the question outright; a short run of digits answers it without converting anything. Only integers long enough to straddle the int64 and uint64 limits are converted, and those once rather than once per kind.Both lazy sentinels,
typeRawStringandnumberTypeRaw, are gone.A rendering divergence goes with it
MySQL resolves an escape when it parses, so
"\u0061"is stored and printed as"a". Vitess printed it verbatim when nothing had calledType(), and unescaped when something had — so the output depended on what the rest of the query happened to touch. It is now always the MySQL form.Related Issue(s)
Stacked on #20722. Fixes the data race reported on #20718, which should merge after this.
Checklist
Backport justification
Labelled for release-23.0 and release-24.0. This is a data race on state shared by every goroutine running a cached plan, and it is already reachable on those branches: hashing a bare JSON string or number literal reports races today, because
WeightStringcalls bothType()andNumberType()on the value it is handed. What it produced when it went wrong was a torn read and a wrongly cached comparison result, not a clean failure.The race is narrower there than on
mainwith #20718, which is what made it easy to reproduce — but narrower is not absent, and a data race is not something to leave in a release branch on the grounds that it needs an awkward query to hit.This cannot be backported on its own. It is stacked on #20722, whose changes to
readFloatit builds on directly, so that has to go to the same branches first. #20722 is a behaviour change in its own right — documents carrying a number too large for a double stop parsing — so that decision should be made deliberately rather than inherited from this one.Also worth weighing for a release branch: the rendering change below is user-visible. A JSON string containing an escape has been printing inconsistently, so some queries will start returning a different — correct, MySQL-matching — string than they do today.
Tests
TestNumberKindMatchesParsingis the safety net under deciding a number's kind from its shape. The rule exists to avoid the conversions, so it has to reach the same answer they would: around 270 spellings across every length from 1 to 25 digits, both signs, leading zeros, and the values either side of int64 min, int64 max and uint64 max.TestParseConcurrentReadsis the regression test for the races above — one parsed document, eight readers, checking that fingerprints and renders agree.TestParseSettlesValueswalks a parsed tree asserting nothing is left undecided, and that reading a value does not change it.TestParseUnescapesStringspins the rendering against the MySQL form.BenchmarkParseis committed so the cost below stays checkable.go/mysql/datetime,go/mysql/decimal,go/mysql/json,go/sqltypes,go/vt/sqlparser,go/vt/vtgate/evalengine, the MySQL differential suite andgo/vt/vtgate/engineall pass.Benchmarks
BenchmarkParse, 1024-element documents, arm64,GOMAXPROCS=4,-count=6, via benchstat:Documents of plain strings parse faster, because settling the value removes a branch the reader used to take. The two regressions are both work moved earlier rather than work added: an escaped string is unescaped at parse instead of on first read, and a long integer is converted at parse instead of on first read. Each is only waste if the value is never read at all, and the escaped-strings case is a worst case where every one of the 1024 strings carries an escape.
Deciding a long integer's kind cost +255% when it went through the conversions in order; taking the shape into account first, and using a single
ParseUint64for the lengths that straddle the limits, brings that to +86%. I deliberately did not replace that last conversion with digit-string comparisons against the limits — the boundary is worth 20µs to get right by construction.Deployment Notes
A JSON string containing an escape now renders in its resolved form —
"\u0061"prints as"a"— consistently, and matching MySQL. Previously the output depended on whether anything in the query had inspected the value. 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 review feedback on #20718.