-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
efb71f4
95b3684
23a02e4
e4f743c
c084bd4
cb6ab55
4d46160
57692d6
4a7808a
497c411
281e701
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,13 +180,18 @@ func NewFromString(s string) (d Decimal, err error) { | |
| } | ||
| i++ | ||
| } | ||
|
|
||
| // The number starts after the leading whitespace, so that is where a sign | ||
| // 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] == '+': | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can misread fixed-width text because Should the 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 {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)"},
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes user-visible AGENTS.md reference: AGENTS.md:L231-L233 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 expPos is | ||
| // -1, so the second test collapses into the first. | ||
| if i != start && i != expPos+1 { | ||
| break next | ||
| } | ||
| case s[i] >= '0' && s[i] <= '9': | ||
|
|
@@ -215,17 +220,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) | ||
|
|
@@ -237,7 +246,7 @@ next: | |
|
|
||
| var expOverflow bool | ||
| if expPos != -1 { | ||
| e, _ := fastparse.ParseInt64(s[expPos+1:i], 10) | ||
| e, _ := fastparse.ParseInt64(strings.TrimPrefix(s[expPos+1:i], "+"), 10) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think accepting the exponent s71 := "0." + strings.Repeat("0", 71) + "1e+73"
s72 := "0." + strings.Repeat("0", 72) + "1e+74"This change correctly moves There’s a similar overflow boundary: 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed, but this seems to be a pre-existing issue. I opened #20742 to track it. |
||
| switch { | ||
| case e > ExponentLimit: | ||
| e = ExponentLimit | ||
|
|
@@ -249,6 +258,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 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
|
|
@@ -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", | ||
|
|
@@ -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 | ||
|
|
@@ -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. | ||
|
|
@@ -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 | ||
|
|
@@ -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 | ||
|
|
@@ -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) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This standalone test exercises only the unchanged AGENTS.md reference: AGENTS.md:L79-L80 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Verified before deciding what to do with it: the merge base has only generic One deliberate note: the |
||
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ var Cases = []TestCase{ | |
| {Run: LargeDecimals}, | ||
| {Run: LargeIntegers}, | ||
| {Run: DecimalClamping}, | ||
| {Run: SignedExponents}, | ||
| {Run: BitwiseOperatorsUnary}, | ||
| {Run: BitwiseOperators}, | ||
| {Run: WeightString}, | ||
|
|
@@ -914,6 +915,28 @@ func DecimalClamping(yield Query) { | |
| } | ||
| } | ||
|
|
||
| // SignedExponents covers numeric text carrying a written sign, on the number | ||
| // itself and on its exponent. MySQL reads both, so a cast or a JSON comparison | ||
| // over one has to land on the same value. | ||
| func SignedExponents(yield Query) { | ||
| mantissas := []string{"1", "+1", "-1", "1.5", "+1.5", "-1.5", "0", "+0", "-0"} | ||
| exponents := []string{"", "e5", "e+5", "E+5", "e-5", "E-5", "e+0", "e-0"} | ||
|
|
||
| for _, mantissa := range mantissas { | ||
| for _, exponent := range exponents { | ||
| literal := "'" + mantissa + exponent + "'" | ||
| yield(fmt.Sprintf("CAST(%s AS DECIMAL(20, 6))", literal), nil, false) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 95b3684
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Could we pin that rejection and tighten the JSON lexer at the same time? The decimal parser should continue accepting // JSON permits an optional minus, not a plus.
if s[i] == '-' {
i++
}The exponent branch would still accept
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. e4f743c should make sure the case you pointed out is covered. |
||
| yield(fmt.Sprintf("CAST(%s AS DOUBLE)", literal), nil, false) | ||
| yield(literal+" + 0", nil, false) | ||
|
|
||
| // 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) | ||
|
Comment on lines
+934
to
+937
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For the AGENTS.md reference: AGENTS.md:L79-L80 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — confirmed: Addressed in cb6ab55 with a direct expected-error test,
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we also carry a few representative spellings through a JSON string? The comparison here exercises JSON numbers through 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea — added in 4d46160, pretty much verbatim. |
||
| } | ||
| } | ||
| } | ||
|
|
||
| func BitwiseOperatorsUnary(yield Query) { | ||
| for _, op := range []string{"~", "BIT_COUNT"} { | ||
| for _, rhs := range inputBitwise { | ||
|
|
||
There was a problem hiding this comment.
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:
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 aftere?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.
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.