decimal: read numeric text the way MySQL does - #20721
Conversation
NewFromString accepted a sign only as a leading minus, so a plus anywhere
stopped the scan. Because the scan returns whatever it parsed up to that
point alongside its error, and two evalengine call sites drop that error,
the wrong value reached the caller rather than a failure:
CAST('1e+5' AS DECIMAL(20,2)) evaluated to 1.00 and CAST('1.5e+3' ...) to
1.50, where MySQL returns 100000.00 and 1500.00. JSON comparison saw the
error and refused to compare the value at all.
A sign is now read at the start of the number and at the start of the
exponent, and stripped again before fastparse, which reads a minus but not
a plus.
The differential cases added alongside surfaced a second divergence, older
and unrelated to the sign: scaling zero by a positive power of ten kept the
exponent, so CAST('0e5' AS DECIMAL(20,6)) formatted as 000000.000000 where
MySQL prints 0.000000. String trimmed the padding away, which is why only
the fixed-scale formatting showed it. Zero now drops a positive exponent
while keeping a negative one, which is the scale it was written to.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20721 +/- ##
===========================================
+ Coverage 69.67% 85.48% +15.80%
===========================================
Files 1614 85 -1529
Lines 216793 22583 -194210
===========================================
- Hits 151044 19304 -131740
+ Misses 65749 3279 -62470
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:
|
| // A sign is allowed at the start of the number and at the start of | ||
| // the exponent, nowhere else. With no exponent seen yet expPos is | ||
| // -1, so the second test collapses into the first. | ||
| if i != 0 && i != expPos+1 { |
There was a problem hiding this comment.
Could we use the position after the leading-whitespace scan as the start of the mantissa here, rather than absolute index 0? As written, a sign is still rejected whenever whitespace precedes it, and because evalengine uses the partial value while dropping the error, the PR head currently produces:
CAST(' +1' AS DECIMAL(20,6)) -- 0.000000
CAST(' -1e+5' AS DECIMAL(20,6)) -- 0.000000MySQL 5.7.44 and 8.4.10 return 1.000000 and -100000.000000 respectively. Maybe this could track the numeric start after skipping whitespace and use it both for the sign check and the mantissa slices:
start := i
// ...
if i != start && i != expPos+1 {
break next
}
// Use start rather than zero in each mantissa-building branch.
si = s[start:i]
si = strings.TrimPrefix(si, "+")That would seem to cover a written sign at the actual start of the number while retaining the exponent-sign check.
| for _, mantissa := range mantissas { | ||
| for _, exponent := range exponents { | ||
| literal := "'" + mantissa + exponent + "'" | ||
| yield(fmt.Sprintf("CAST(%s AS DECIMAL(20, 6))", literal), nil, false) |
There was a problem hiding this comment.
Would it be worth adding an actual JSON comparison here as well? The comment and PR description mention the JSON comparison failure, but these generated cases currently only exercise decimal casts, double casts, and + 0.
For the JSON-valid mantissas, perhaps something along these lines would cover that path directly:
if !strings.HasPrefix(mantissa, "+") {
yield(fmt.Sprintf(
"CAST(%s AS JSON) = CAST(%s AS JSON)",
literal, literal,
), nil, false)
}The leading-plus mantissas need skipping because they are not valid JSON numbers, but a case such as 1e+5 would fail before this fix and exercise the comparison path end to end.
There was a problem hiding this comment.
One wrinkle I missed when suggesting that the leading-plus rows be skipped: Vitess's JSON lexer currently accepts them. This decimal fix therefore changes:
CAST('+1' AS JSON) = CAST('1' AS JSON)from a range error on the base to true at this head, while MySQL rejects the cast as invalid JSON.
Could we pin that rejection and tighten the JSON lexer at the same time? The decimal parser should continue accepting + for ordinary numeric text, but JSON's leading sign could remain minus-only:
// JSON permits an optional minus, not a plus.
if s[i] == '-' {
i++
}The exponent branch would still accept 1e+2. This also prevents the stacked #20723 from settling +1 as signed even though Int64() cannot parse it. Since this PR can be merged and backported independently, it seems safest for the small parser fix to land with or before it.
There was a problem hiding this comment.
e4f743c should make sure the case you pointed out is covered.
NewFromString skips leading whitespace before reading anything, but the sign
check compared against the start of the string rather than the start of the
number, so a sign that followed whitespace ended the scan. Since the scan
returns what it parsed so far and two evalengine call sites drop the error,
CAST(' +1' AS DECIMAL(20,6)) evaluated to 0 rather than 1.000000. A leading
minus was affected the same way and had been since before the plus was
handled at all: CAST(' -1' AS DECIMAL(20,6)) was 0 where MySQL returns
-1.000000.
The scan now remembers where the number starts and reads both the sign and
the mantissa from there.
The generated cases gain a JSON comparison. JSON reads a number through a
decimal too, so the spellings this fixes could not be compared at all, and
nothing exercised that path: only the decimal cast, the double cast and
+ 0 were covered. Spellings with a leading plus are skipped, having no JSON
document to compare.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
…tion Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The differential suite excuses a MySQL "Invalid JSON text" error when the local evaluation succeeds, so it cannot catch a cast that wrongly starts accepting these spellings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
| var expOverflow bool | ||
| if expPos != -1 { | ||
| e, _ := fastparse.ParseInt64(s[expPos+1:i], 10) | ||
| e, _ := fastparse.ParseInt64(strings.TrimPrefix(s[expPos+1:i], "+"), 10) |
There was a problem hiding this comment.
I think accepting the exponent + exposes another MySQL conversion boundary here:
s71 := "0." + strings.Repeat("0", 71) + "1e+73"
s72 := "0." + strings.Repeat("0", 72) + "1e+74"This change correctly moves s71 from 0 to 10, matching MySQL, but it also moves s72 from 0 to 10 while MySQL 8.0 and 8.4 still return 0. MySQL limits the mantissa to its decimal buffer before applying the exponent, whereas this path retains the extra digit and shifts it back into significance.
There’s a similar overflow boundary: 1e+18446744073709551615 becomes the largest decimal in both MySQL and Vitess, but the next exponent becomes zero in MySQL while the signed ParseInt64 calls saturate and Vitess still returns the largest value.
Should these string conversions use a separate MySQL-compatible coercion path which applies the existing mantissa limits before the exponent and distinguishes uint64 overflow? It should probably cover both string branches in evalToDecimal, while preserving prefix behaviour such as 1e+5x returning 100000.
There was a problem hiding this comment.
Confirmed, but this seems to be a pre-existing issue. I opened #20742 to track it.
| // JSON comparison reads the number through a decimal too, so it is | ||
| // worth exercising directly. A leading plus is not a JSON number, | ||
| // so those spellings pin the cast being rejected rather than a value. | ||
| yield(fmt.Sprintf("CAST(%s AS JSON) = CAST(%s AS JSON)", literal, literal), nil, false) |
There was a problem hiding this comment.
Should we also carry a few representative spellings through a JSON string? The comparison here exercises JSON numbers through Value.Decimal(), but a nested cast reaches the separate JSON-string branch in evalToDecimal.
These cases fail on the parent and match MySQL with this change:
for _, jsonString := range []string{"1e+5", "+1", " -1"} {
literal := fmt.Sprintf(`'"%s"'`, jsonString)
yield(fmt.Sprintf(
"CAST(CAST(%s AS JSON) AS DECIMAL(20, 6))",
literal,
), nil, false)
}Keeping it to these three rows should cover that caller without repeating the full generated matrix.
There was a problem hiding this comment.
Good idea — added in 4d46160, pretty much verbatim.
| i++ | ||
| } | ||
|
|
||
| // The number starts after the leading whitespace, so that is where a sign |
There was a problem hiding this comment.
I think #20725 is still outstanding at this head. You mentioned fixing it in this PR, but the two cases from the issue remain:
"\v+1" -> Vitess 0, MySQL 1
"1e +2" -> Vitess 1, MySQL 100
Because evalengine drops the parse error, both become silent wrong results. Should the outer whitespace handling add '\v' and '\f', with a separate exponent start that skips only space or tab immediately after e?
Cases such as "1e\n+2", "1e+ 2", and "1 e+2" should remain partial-value errors, so the general whitespace helper probably should not be reused inside the exponent.
There was a problem hiding this comment.
You were right, this was still outstanding — fixed now, and the PR closes #20725.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Vertical tab and form feed count as whitespace around a number, and the exponent is read past spaces and tabs between its marker and its sign. Fixes #20725 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The reader consults latin1's character table whatever the string's own charset is, so the byte counts as whitespace wherever it can stand on its own, which is latin1 and binary text; in utf8mb4 the character is 0xC2 0xA0, and the 0xC2 ends the number first. The exponent scan now anchors expStart past the whitespace it steps over instead of trimming it back out of the substring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
| // Negative sign is allowed at the start and at the start | ||
| // of the exponent. | ||
| if i != 0 && expPos == -1 && i != expPos+1 { | ||
| case s[i] == '-' || s[i] == '+': |
There was a problem hiding this comment.
I think this can misread fixed-width text because evalToDecimal passes raw encoded bytes into this scanner. For example, _utf16le X'2B31' is one nonnumeric U+312B character, but this head reads it as ASCII +1 and returns 1, while the base and MySQL return 0.
Should the evalBytes bridge normalise UTF-16, UTF-16LE, UCS-2 and UTF-32 to latin1 first, matching MySQL's mbminlen > 1 path?
raw := e.bytes
if col := colldata.Lookup(e.col.Collation); col != nil {
cs := col.Charset()
switch cs.(type) {
case charset.Charset_utf16, charset.Charset_utf16le,
charset.Charset_ucs2, charset.Charset_utf32:
raw, _ = charset.Convert(nil, charset.Charset_latin1{}, raw, cs)
}
}
dec, _ := decimal.NewFromString(hack.String(raw))A couple of TestCompilerSingle rows should cover both sides and exercise the AST and compiled paths:
{expression: `CAST(_utf16le X'2B31' AS DECIMAL(20,6))`,
result: "DECIMAL(0.000000)"},
{expression: `CAST(_utf16le X'2B003100' AS DECIMAL(20,6))`,
result: "DECIMAL(1.000000)"},There was a problem hiding this comment.
I fixed this in 497c411, but the same issue exists for float and int types.
MySQL reads numeric text through latin1 when the character set's smallest character is wider than one byte (str2my_decimal), so UTF-16, UTF-16LE, UCS-2 and UTF-32 text is decoded rather than read as raw bytes, and a character with no latin1 form becomes '?', which ends the number. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 497c411b92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if col := colldata.Lookup(e.col.Collation); col != nil { | ||
| switch cs := col.Charset(); cs.(type) { | ||
| case charset.Charset_utf16, charset.Charset_utf16le, charset.Charset_ucs2, charset.Charset_utf32: | ||
| raw, _ = charset.Convert(nil, charset.Charset_latin1{}, raw, cs) |
There was a problem hiding this comment.
Reject malformed UTF-16 before converting decimals
When numeric text contains an isolated UTF-16 surrogate, such as CAST(_utf16 X'D800' AS DECIMAL), this newly added call never returns: Charset_utf16.DecodeRune reports width 0 for the incomplete surrogate, and charset.Convert repeatedly processes the same bytes without advancing. This makes a user-supplied expression hang its VTGate evaluation goroutine; validate malformed fixed-width input or ensure conversion always advances before invoking it here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 281e701. The root cause was in the decoders, not this call site: Charset_utf16be/Charset_utf16le.DecodeRune reported width 0 for a surrogate code unit with no pair remaining, so any byte-walking caller of charset.Convert looped forever — including the pre-existing CONVERT(_utf16 X'D800' USING latin1) path on main, not just this new call. The invalid unit is now reported with its own two-byte width, so conversion always advances and substitutes ? (callers that stop at invalid input still stop, since they treat any RuneError with width < 3 as such).
A new contract test (TestDecodeRuneAlwaysAdvances) sweeps all charsets over ~74k adversarial inputs asserting 1 <= width <= len(input); it also caught sjis/cp932 reporting width 2 for a lead byte ending the input (an out-of-bounds slice for the same callers), fixed in the same commit. End-to-end coverage: TestCompilerSingle now pins CAST(_utf16 X'D800' AS DECIMAL(20, 6)), the utf16le mirror, and the CONVERT ... USING route on both evaluation paths.
| // Negative sign is allowed at the start and at the start | ||
| // of the exponent. | ||
| if i != 0 && expPos == -1 && i != expPos+1 { | ||
| case s[i] == '-' || s[i] == '+': |
There was a problem hiding this comment.
Document the changed numeric-cast behavior
This changes user-visible CAST(... AS DECIMAL) behavior for leading signs, additional whitespace, and fixed-width character sets, but the commit contains no release or deployment note. Add an explicit upgrade-facing callout for these observable correctness changes as required by the repository policy.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We're not adding a changelog callout for this. These are compatibility fixes that make the evalengine match MySQL in more cases — matching MySQL is the baseline contract Vitess users already expect, especially for edge cases like these, so advertising each parity fix as a behavior change adds noise rather than information. Queries that vtgate pushes down to MySQL were never affected either way. Changelog callouts remain for changes that diverge from or go beyond MySQL semantics.
A surrogate code unit with no pair left DecodeRune reporting width 0, so byte-walking callers like charset.Convert, Expand and Length never advanced and looped forever on input a user can supply, e.g. CAST(_utf16 X'D800' AS DECIMAL) or CONVERT(_utf16 X'D800' USING latin1). The invalid unit is now reported with its own two-byte width; callers that stop at invalid input still stop, as they treat any RuneError with a width below 3 as such. The same contract test caught sjis and cp932 reporting a two-byte width for a lead byte that ends the input, which would step callers past the end of the slice; that is now reported as a single invalid byte. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
|
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. |
Description
decimal.NewFromStringread a sign only where it did not help. Two separate mistakes:Neither failed cleanly. The scan returns whatever it parsed up to the point it gave up, alongside its error, and two evalengine call sites drop that error — so a wrong value reached the caller instead of a failure:
CAST('1e+5' AS DECIMAL(20,2))CAST('1.5e+3' AS DECIMAL(20,2))CAST('+1' AS DECIMAL(20,2))CAST(' +1' AS DECIMAL(20,6))CAST(' -1' AS DECIMAL(20,6))CAST(' -1e+5' AS DECIMAL(20,6))'1e+5' + 0That last row is the tell: the float path already handled these, so Vitess disagreed with itself depending on which type the expression took. JSON comparison reads a number through a decimal as well, so it could not compare these values at all.
The scan now remembers where the number starts, after any leading whitespace, and reads both the sign and the mantissa from there. A plus is stripped again before
fastparse, which reads a minus but not a plus.NewFromMySQLalready accepted a leading plus, so onlyNewFromStringwas affected.The differential cases added alongside surfaced a third divergence, older and unrelated to signs: scaling zero by a positive power of ten kept the exponent, so
CAST('0e5' AS DECIMAL(20,6))formatted as000000.000000where MySQL prints0.000000.Stringtrims the padding away, which is why only the fixed-scale formatting showed it. Zero now drops a positive exponent while keeping a negative one, which is the scale it was written to.The branch has
mainmerged in to pick up #20722, which tightened the JSON number lexer to reject a leading plus the way MySQL does. That closes the interaction this PR would otherwise have opened —CAST('+1' AS JSON)staying an error rather than becoming a comparable document — and the review rounds on it grew the test surface here beyond the decimal fix itself; see Tests.Related Issue(s)
Found while reviewing #20691 and working on #20718. Review findings on this branch filed #20739 (JSON number classification), #20741 (the double conversion's whitespace and truncation) and #20742 (the decimal conversion's mantissa buffer and exponent overflow) — all divergences that predate this PR and are out of scope here.
Fixes #20725: vertical tab and form feed now count as whitespace around numeric text, and the exponent is read past spaces and tabs — those two bytes only, as MySQL's exponent reader skips — between its marker and its sign. A byte sweep against MySQL 8.0.46 across every single-byte character set shows they all mark exactly one byte past ASCII as whitespace, 0xA0 — even ascii, where the byte is not a valid character, and macroman, whose actual no-break space sits elsewhere — so the decimal reader consults latin1's table whatever the text's charset is, and the scanner now does the same. The byte can only stand on its own in latin1 and binary text; in utf8mb4 the character is 0xC2 0xA0, and the 0xC2 ends the number first, which the utf8mb4 test spellings pin.
Checklist
Backport justification
Labelled for release-23.0 and release-24.0. Every case above is a silently wrong value rather than an error: a cast that should produce 100000.00 produces 1.00, and nothing in the query reports a problem. The same expression can also answer differently depending on which type it took, since the float path was always correct, which makes the wrong answer look like a typing subtlety rather than a bug.
The change only affects text that Vitess previously refused to read past. Any spelling that parsed correctly before parses identically now, so the risk is confined to values that were already being converted wrongly.
Tests
testTableScientificNotation, which the existingTestNewFromStringwalks and also negates.TestNewFromStringLeadingPluscovers signs the negating table cannot —-+1is not a number — including the whitespace-prefixed spellings.TestNewFromStringZeroExponentpins the formatting of a zero written with a positive exponent, and that a zero written to a scale keeps it.SignedExponentscrosses signed mantissas with signed exponents overCAST(… AS DECIMAL),CAST(… AS DOUBLE),+ 0and a JSON comparison, checked against a live MySQL by the integration suite. This is what surfaced the zero-exponent divergence. Reverting the fix fails it on exactly the JSON comparisons the change repairs, so the case covers the path end to end rather than adding volume. With mysql/json: read numbers the way MySQL does #20722 merged in, the leading-plus mantissas run through the JSON comparison too, where both engines reject the document.TestCastInvalidJSONasserts thatCAST('+1' AS JSON)and its whitespace, fraction and too-big siblings error on both the AST and compiled evaluation paths. The differential suite cannot pin that direction — its comparison excuses a MySQLInvalid JSON texterror when the local evaluation succeeds — so the rejection is pinned directly, per review.TestNewFromStringWhitespaceandTestNewFromStringWhitespaceBoundarycover the whitespace MySQL reads as part of numeric text — the\v/\f, 0xA0 and exponent-whitespace spellings from mysql/decimal: match MySQL whitespace handling in numeric text #20725, plus the edges around them: multiple spaces after the marker, mixed space and tab, an unsigned exponent after a space, and the spellings that must stay partial-value errors (1e\n+2,1e+ 2,1 e+2,1e\xa05,+ 1). Every expectation was probed against MySQL 8.0.46 first.NumericTextWhitespaceruns the same spellings throughCAST … AS DECIMAL(20, 6)against a live MySQL, with hex-literal introducer rows (_latin1 X'A031',_binary X'A031',_utf8mb4 X'C2A031', and 0xA0 beside the exponent) covering the bytes a quoted literal cannot carry.TestCompilerSinglepins the same introducer spellings on both evaluation paths. It all stays offCAST … AS DOUBLE: MySQL's double conversion has different whitespace rules — it stops at the exponent marker and does not read 0xA0 — and the float path's own gaps predate this PR and are tracked in fastparse: string-to-double conversion reads less whitespace and truncates exponents differently than MySQL #20741.go/mysql/jsonnumber tests now walk the boundary spellings RapidJSON's own suite pins: an exponent missing its digits, the integer widths a significand accumulates through, the stop where a negative exponent's int would overflow, the largest double written into a fraction (0.017976931348623157e+310and its rejected…159sibling), and the near-800-digit fraction that exercises the seventeen-significant-digit cutoff. Every case was checked against MySQL 8.0.46 before being written down.go/mysql/decimal,go/mysql/json,go/sqltypes,go/vt/sqlparser,go/vt/vtgate/evalengineand the MySQL differential suite pass locally.Deployment Notes
Numeric text carrying a written sign now converts to the value it spells rather than to a truncated one:
CAST('1e+5' AS DECIMAL(20,2))is 100000.00 where it used to be 1.00, andCAST(' -1' AS DECIMAL(20,6))is −1.000000 where it used to be 0. A zero written with a positive exponent formats as0.000000rather than000000.000000. Numeric text converted to a decimal also reads whitespace the way MySQL does: a leading vertical tab, form feed or 0xA0 byte no longer truncates the value to 0, andCAST('1e +2' AS DECIMAL(20,6))is 100 where it used to be 1. No migrations or configuration changes.AI Disclosure
Claude Code wrote this one, including the tests — I reviewed it and provided direction.