Skip to content

decimal: read numeric text the way MySQL does #20721

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arthurschreiber wants to merge 11 commits into
base: main
Choose a base branch
from
Open

decimal: read numeric text the way MySQL does #20721

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
efb71f4
decimal: read a written plus in numeric text, as MySQL does
arthurschreiber Jul 27, 2026
95b3684
decimal: anchor the sign to where the number starts
arthurschreiber Jul 28, 2026
23a02e4
Merge upstream/main into arthur/decimal-exponent-sign
arthurschreiber Jul 28, 2026
e4f743c
test: compare leading-plus spellings as JSON too, pinning their rejec…
arthurschreiber Jul 28, 2026
c084bd4
test: pin the number boundaries RapidJSON's own suite walks
arthurschreiber Jul 28, 2026
cb6ab55
test: assert invalid JSON casts fail on both evaluation paths
arthurschreiber Jul 28, 2026
4d46160
test: cast JSON strings to decimal in the signed-exponent cases
arthurschreiber Jul 29, 2026
57692d6
decimal: read the whitespace MySQL reads in numeric text
arthurschreiber Jul 29, 2026
4a7808a
decimal: read 0xA0 as the non-breaking space MySQL sees in numeric text
arthurschreiber Jul 29, 2026
497c411
evalengine: decode fixed-width text before reading it as a decimal
arthurschreiber Jul 29, 2026
281e701
charset: always advance past an unpaired UTF-16 surrogate
arthurschreiber Jul 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions go/mysql/decimal/decimal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ var testTableScientificNotation = map[string]string{
"123.456e0": "123.456",
"123.456e2": "12345.6",
"123.456e10": "1234560000000",
// MySQL accepts a written plus on the exponent.
"1e+9": "1000000000",
"1E+9": "1000000000",
"245E+3": "245000",
"123.456e+2": "12345.6",
"0e+5": "0",
}

func init() {
Expand Down Expand Up @@ -192,6 +198,127 @@ func TestNewFromString(t *testing.T) {
}
}

// TestNewFromStringLeadingPlus covers a sign the negating entries in
// testTableScientificNotation cannot: MySQL reads a leading plus as the number
// it introduces, so CAST('+1' AS DECIMAL) is 1.
func TestNewFromStringLeadingPlus(t *testing.T) {
for in, want := range map[string]string{
"+1": "1",
"+1.5": "1.5",
"+0": "0",
"+1e9": "1000000000",
"+1e+9": "1000000000",
"+123.456e-2": "1.23456",
// The number starts after any leading whitespace, and so does the sign.
" +1": "1",
" -1": "-1",
" +1.5": "1.5",
" -1e+5": "-100000",
"\t-2": "-2",
" 1.5e+3": "1500",
} {
t.Run(in, func(t *testing.T) {
d, err := NewFromString(in)
require.NoError(t, err)
require.Equal(t, want, d.String())
})
}
}

// TestNewFromStringWhitespace covers the whitespace MySQL skips around numeric
// text: a vertical tab, a form feed and a 0xA0 are leading and trailing space
// like a blank or a tab, and a blank or a tab may sit between the exponent
// marker and the exponent it introduces. MySQL 8.0.46 reads every spelling
// here to the same value.
func TestNewFromStringWhitespace(t *testing.T) {
for in, want := range map[string]string{
"\v+1": "1",
"\f-1": "-1",
"\v1": "1",
"1\v": "1",
"1\f": "1",
"\v\f\n\r 1": "1",
// 0xA0 is a non-breaking space in latin1, and the reader reads a string
// through latin1's character table whatever its own charset is.
"\xa01": "1",
"1\xa0": "1",
"\xa0-1": "-1",
"\xa0\t \xa01": "1",
"1e +5": "100000",
"1e -5": "0.00001",
"1e\t-5": "0.00001",
"1e \t+5": "100000",
"1e 5": "100000",
"1e 5": "100000",
"1e\t+5": "100000",
"1E -5": "0.00001",
"1.5e 2": "150",
".5e +3": "500",
"-1.5e +3": "-1500",
" 1e 2 ": "100",
} {
t.Run(in, func(t *testing.T) {
d, err := NewFromString(in)
require.NoError(t, err)
require.Equal(t, want, d.String())
})
}
}

// TestNewFromStringWhitespaceBoundary pins the spellings that stop short of a
// number MySQL would read whole. Each one keeps the mantissa parsed so far as
// its value and reports the string as invalid, so a caller that ignores the
// error lands on the same partial value MySQL truncates to.
func TestNewFromStringWhitespaceBoundary(t *testing.T) {
for in, want := range map[string]string{
// Whitespace belongs before the exponent's sign, not after it.
"1e+ 5": "1",
"1e + 5": "1",
"1e +": "1",
"1e +x": "1",
// The exponent marker itself has to follow the mantissa directly.
"1 e+5": "1",
// Only a blank or a tab is skipped after the marker.
"1e\v5": "1",
"1e\f5": "1",
"1e\n5": "1",
"1e \v5": "1",
"1e\xa05": "1",
// Whitespace after the marker still needs an exponent behind it.
"1e ": "1",
// One sign, not two.
"1e --5": "1",
"1e -+5": "1",
// A sign introduces a number, so whitespace cannot follow it either.
"+ 1": "0",
} {
t.Run(in, func(t *testing.T) {
d, err := NewFromString(in)
require.ErrorContains(t, err, "invalid decimal string")
require.Equal(t, want, d.String())
})
}
}

// TestNewFromStringZeroExponent pins the formatting of a zero written with a
// positive exponent. String trims the padding away, but FormatMySQL keeps the
// scale it is asked for, so a stale exponent surfaces as leading zeros.
func TestNewFromStringZeroExponent(t *testing.T) {
for _, in := range []string{"0e5", "-0e5", "+0e5", "0E+5", "0e2", "0"} {
t.Run(in, func(t *testing.T) {
d, err := NewFromString(in)
require.NoError(t, err)
require.Equal(t, "0", d.String())
require.Equal(t, "0.000000", string(d.FormatMySQL(6)))
})
}

// A zero written to a scale keeps it: only a positive exponent is dropped.
d, err := NewFromString("0.00")
require.NoError(t, err)
require.Equal(t, int32(-2), d.Exponent())
}

func TestFloat64(t *testing.T) {
t.Skipf("Float64 does not check for exact")

Expand Down
59 changes: 45 additions & 14 deletions go/mysql/decimal/scan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ func NewFromString(s string) (d Decimal, err error) {

dotPos := -1
expPos := -1
expStart := -1
i := 0
var num bool
var exp int64
Expand All @@ -180,13 +181,18 @@ func NewFromString(s string) (d Decimal, err error) {
}
i++
}

// The number starts after the leading whitespace, so that is where a sign

@GrahamCampbell GrahamCampbell Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@arthurschreiber arthurschreiber Jul 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, this was still outstanding — fixed now, and the PR closes #20725.

// is allowed and where the mantissa is read from.
start := i
next:
for i < maxLen {
switch {
case s[i] == '-':
// 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] == '+':

@GrahamCampbell GrahamCampbell Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)"},

@arthurschreiber arthurschreiber Jul 29, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in 497c411, but the same issue exists for float and int types.

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@arthurschreiber arthurschreiber Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sign is allowed at the start of the number and at the start of
// the exponent, nowhere else. With no exponent seen yet expStart is
// -1, so the second test never matches.
if i != start && i != expStart {
break next
}
case s[i] >= '0' && s[i] <= '9':
Expand All @@ -198,12 +204,21 @@ next:
break next
}
case s[i] == 'e' || s[i] == 'E':
if expPos == -1 {
expPos = i
num = false
} else {
if expPos != -1 {
break next
}
expPos = i
// MySQL reads the exponent with my_strtoll10, which steps over
// spaces and tabs before the sign and the digits, so '1e +5' is
// 100000. Only those two bytes: a vertical tab or a newline here
// leaves the exponent unread and the mantissa as a partial value.
i++
for i < maxLen && (s[i] == ' ' || s[i] == '\t') {
i++
}
expStart = i
num = false
continue
default:
break next
}
Expand All @@ -215,17 +230,21 @@ next:
var si string
switch {
case dotPos == -1 && expPos == -1:
si = s[:i]
si = s[start:i]
case expPos == -1:
si = s[:dotPos] + s[dotPos+1:i]
si = s[start:dotPos] + s[dotPos+1:i]
exp -= int64(i - dotPos - 1)
case dotPos == -1:
si = s[:expPos]
si = s[start:expPos]
default:
si = s[:dotPos] + s[dotPos+1:expPos]
si = s[start:dotPos] + s[dotPos+1:expPos]
exp -= int64(expPos - dotPos - 1)
}

// fastparse reads a leading minus but not a leading plus, which the scanner
// above accepts because MySQL does.
si = strings.TrimPrefix(si, "+")

if len(si) <= 18 {
var v int64
v, err = fastparse.ParseInt64(si, 10)
Expand All @@ -237,7 +256,7 @@ next:

var expOverflow bool
if expPos != -1 {
e, _ := fastparse.ParseInt64(s[expPos+1:i], 10)
e, _ := fastparse.ParseInt64(strings.TrimPrefix(s[expStart:i], "+"), 10)
switch {
case e > ExponentLimit:
e = ExponentLimit
Expand All @@ -249,6 +268,13 @@ next:
exp += e
}

// Scaling zero by a positive power of ten leaves zero, but keeping the
// exponent renders it as that many leading zeros: '0e5' would format as
// 000000 where MySQL prints 0. A negative exponent is left alone, since it
// is the scale a zero is written to.
if exp > 0 && d.value.Sign() == 0 {
exp = 0
}
d.exp = int32(exp)

for i < maxLen {
Expand Down Expand Up @@ -354,9 +380,14 @@ func parseLargeDecimal(integral, fractional []byte) (*big.Int, error) {
return new(big.Int).SetBits(z), nil
}

// isSpace reports the bytes MySQL skips around numeric text. It reads them
// through latin1's character table whatever the string's own charset is, so a
// vertical tab, a form feed and a 0xA0 all count as space. 0xA0 is reachable
// from latin1 and binary text, where it stands for a non-breaking space; in
// utf8mb4 that character is 0xC2 0xA0, and the 0xC2 ends the number first.
func isSpace(c byte) bool {
switch c {
case ' ', '\t', '\n', '\r':
case ' ', '\t', '\n', '\v', '\f', '\r', 0xA0:
return true
default:
return false
Expand Down
80 changes: 80 additions & 0 deletions go/mysql/json/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,18 @@ func TestParseRawNumber(t *testing.T) {
func TestParseNumberTooBigForDouble(t *testing.T) {
tooManyDigits := "1" + strings.Repeat("0", 309)

// The subnormal 2.2250738585072011e-308, written out to nearly eight
// hundred digits of fraction.
longFraction := "2.22507385850720113605740979670913197593481954635164564802342610972482222202107694551652952390813508" +
"7914149158913039621106870086438694594645527657207407820621743379988141063267329253552286881372149012" +
"9811224514518898490572223072852551331557550159143974763979834118019993239625482890171070818506906306" +
"6665599493827577257201576306269066333264756530000924588831643303777979186961204949739037782970490505" +
"1080609940730262937128958950003583799967207254304360284078895771796150945516748243471030702609144621" +
"5722898802581825451803257070188608721131280795122334262883686223215037756666225039825343359745688844" +
"2390026549819838548794829220689472168983109969836584681402285424333066033985088644580400103493397042" +
"7567186443383770486037861622771738545623065874679014086723327636718751234567890123456789012345678901" +
"e-308"

t.Run("accepted", func(t *testing.T) {
for _, doc := range []string{
"1e308",
Expand All @@ -93,6 +105,7 @@ func TestParseNumberTooBigForDouble(t *testing.T) {
"-1.7976931348623157e308",
"99999999999999999999999999999999999999999",
"1" + strings.Repeat("0", 307),
"1" + strings.Repeat("0", 308),
// Underflow keeps the document valid and reads as zero.
"1e-400",
"1e-1000",
Expand Down Expand Up @@ -121,7 +134,14 @@ func TestParseNumberTooBigForDouble(t *testing.T) {
"0.00e310",
"0.1e309",
"0.01e310",
// The largest double itself, written so that its exponent stands
// past the bound until the fraction buys the places back.
"0.017976931348623157e+310",
"0." + strings.Repeat("0", 400) + "1e700",
// A fraction contributes seventeen significant digits; the
// hundreds behind these move neither the value nor the decimal
// point, however many there are.
longFraction,
} {
t.Run(startEndString(doc), func(t *testing.T) {
var p Parser
Expand Down Expand Up @@ -155,6 +175,7 @@ func TestParseNumberTooBigForDouble(t *testing.T) {
// Within the written bound, but too big once converted.
"10e308",
"1" + strings.Repeat("0", 30) + "e279",
"0.017976931348623159e+310",
// More digits than a double has room for. The digits are read before
// the exponent is applied, so a negative exponent does not buy the
// room back however far it moves the decimal point afterwards.
Expand Down Expand Up @@ -203,6 +224,31 @@ func TestParseNumberTooBigForDouble(t *testing.T) {
}
})

// The int a negative exponent accumulates into stops taking digits once
// another could overflow it. Everything written by then already sits far
// below the smallest double, so however much further the spelling runs,
// these stay valid and read as zero. MySQL 8.0.46 reads each of them the
// same way.
t.Run("a negative exponent around the stop of the int it accumulates into", func(t *testing.T) {
for _, doc := range []string{
"1e-214748363",
"1e-214748364",
"1e-21474836311",
"1e-00011111111111",
"-1e-00011111111111",
} {
t.Run(doc, func(t *testing.T) {
var p Parser
v, err := p.Parse(doc)
require.NoError(t, err)

f, ok := v.Float64()
require.True(t, ok)
require.Zero(t, f)
})
}
})

// The significand accumulates one digit at a time, and each step rounds
// the multiplication and the addition separately, the way MySQL's builds
// run the loop. Fusing the two into one rounding — which the Go compiler
Expand Down Expand Up @@ -288,10 +334,13 @@ func TestParseNumberGrammar(t *testing.T) {
"007", "-003", "01", "00", "00.5", "01.5", "[007]",
// A decimal point missing a digit on one side.
".2", "-.2", "12.", "-12.", "1.e5", `{"a": .2}`, "[12.]",
// An exponent missing its digits.
"1e", "1e_", "1e+", "1e-", "[1e]",
// A written plus.
"+1", "+1.5", "+0", "[+1]",
// Not a number at all.
"nan", "NaN", "NAN", "[nan]", `{"a": nan}`, "-nan", ".", "-",
"inf", "-inf", "Infinity", "[inf]",
} {
t.Run(doc, func(t *testing.T) {
var p Parser
Expand All @@ -302,6 +351,37 @@ func TestParseNumberGrammar(t *testing.T) {
})
}

// TestParseNumberIntegerBoundaries walks the spellings on either side of each
// integer width a JSON number can outgrow — int32, uint32, int64, uint64 —
// until only a double can hold it. Every one of them is a number to MySQL,
// exact while an int64 or a uint64 still holds it and approximate once only a
// double does.
func TestParseNumberIntegerBoundaries(t *testing.T) {

@chatgpt-codex-connector chatgpt-codex-connector Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove tests that pass without the fix

This standalone test exercises only the unchanged Parser.Parse and Value.NumberType paths: the commit modifies no production file in go/mysql/json, and the decimal change is reached through Value.Decimal, which this test never calls. Consequently, every case passes against the parent implementation as well, adding CI work without guarding this fix; remove it from this change or pair it with the production change it is intended to protect.

AGENTS.md reference: AGENTS.md:L79-L80

Useful? React with 👍 / 👎.

@arthurschreiber arthurschreiber Jul 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The observation matches this PR's diff, but the full context is a PR split: the production change this test was written against — the RapidJSON-style number reading in go/mysql/json/parser.go — merged separately as #20722, which is now this branch's merge base. The test was authored while that parser change still lived on this branch, and the split orphaned it here.

Verified before deciding what to do with it: the merge base has only generic NumberType assertions (Signed for integers, Float for non-integers, in the round-trip tests) and pins none of these specific boundary values — no 9223372036854775808, 4294967296, or 18446744073709551616 anywhere. So this is novel regression coverage for #20722's merged behavior, not duplicate characterization, and it stays here rather than in a separate test-only PR.

One deliberate note: the 4294967296 → NumberTypeSigned row pins current behavior that #20739 will flip to Unsigned to match MySQL — it's a characterization pin that PR will consciously update, not an endorsement of the current classification.

for _, tc := range []struct {
doc string
n NumberType
}{
{"-2147483648", NumberTypeSigned},
{"-2147483649", NumberTypeSigned},
{"4294967295", NumberTypeSigned},
{"4294967296", NumberTypeSigned},
{"9223372036854775807", NumberTypeSigned},
{"9223372036854775808", NumberTypeUnsigned},
{"-9223372036854775808", NumberTypeSigned},
{"-9223372036854775809", NumberTypeFloat},
{"18446744073709551615", NumberTypeUnsigned},
{"18446744073709551616", NumberTypeFloat},
} {
t.Run(tc.doc, func(t *testing.T) {
var p Parser
v, err := p.Parse(tc.doc)
require.NoError(t, err)
require.Equal(t, TypeNumber, v.Type())
require.Equal(t, tc.n, v.NumberType())
})
}
}

// TestParseErrorAbbreviatesTheDocument covers how much of a rejected document
// its error names. Nothing bounds how long a document may be, and Parse copies
// the message it wraps, so naming the text in full hands a client its own
Expand Down
Loading
Loading