Skip to content

[release-23.0] mysql/json: read numbers the way MySQL does (#20722) - #20736

Open
vitess-bot[bot] wants to merge 2 commits into
release-23.0from
backport-20722-to-release-23.0
Open

[release-23.0] mysql/json: read numbers the way MySQL does (#20722)#20736
vitess-bot[bot] wants to merge 2 commits into
release-23.0from
backport-20722-to-release-23.0

Conversation

@vitess-bot

@vitess-bot vitess-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

This is a backport of #20722

Copilot AI review requested due to automatic review settings July 28, 2026 22:32
@vitess-bot vitess-bot Bot added Type: Bug Component: Query Serving Backport This is a backport Component: Evalengine changes to the evaluation engine Skip CI Skip CI actions from running Merge Conflict labels Jul 28, 2026
@vitess-bot

vitess-bot Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Hello @arthurschreiber, there are conflicts in this backport.

Please address them in order to merge this Pull Request. You can execute the snippet below to reset your branch and resolve the conflict manually.

Make sure you replace origin by the name of the vitessio/vitess remote

git fetch --all
gh pr checkout 20736
git reset --hard origin/release-23.0
git cherry-pick -m 1 ec6fdee9830a1f7117db2e6a59f634c4f0982713

@github-actions github-actions Bot added this to the v23.0.6 milestone Jul 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot could not run the full agentic suite for this review because it was automatically requested on a bot-authored pull request. Request a review from Copilot under Reviewers to retry with the full agentic suite. Improved support for bot-authored pull requests is coming soon.

Aligns Vitess’s JSON number parsing behavior with MySQL/RapidJSON (including double-range acceptance rules), and adds tests/benchmarks plus licensing attribution for the RapidJSON-derived logic.

Changes:

  • Tighten JSON number grammar and enforce MySQL’s “must fit in double” boundary during parsing.
  • Add extensive tests for number grammar, overflow/underflow boundaries, and error message abbreviation.
  • Add parsing benchmarks and RapidJSON license/attribution.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
go/mysql/json/parser.go Updates number parsing + adds MySQL/RapidJSON-compatible double-fit checks; improves error truncation.
go/mysql/json/parser_test.go Expands tests for number grammar and double-fit boundary behavior.
go/mysql/json/parser_bench_test.go Adds benchmarks and a helper to ensure benchmark docs hit intended parsing paths.
go/mysql/json/marshal.go Adds/updates raw-JSON-to-SQL streaming marshaling logic (but currently has merge conflicts).
go/mysql/json/marshal_test.go Adds tests for AppendMarshalSQL behavior (but currently has merge conflicts).
go/mysql/json/LICENSE.rapidjson Adds required third-party license and attribution for RapidJSON-derived logic.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go/mysql/json/parser_test.go Outdated
Comment on lines +34 to +45
<<<<<<< HEAD
flen, ok := readFloat(s)
if !ok {
t.Fatalf("unexpected error when parsing '%s'", s)
}
||||||| parent of ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))
flen, ok := readFloat(s)
require.Truef(t, ok, "unexpected error when parsing '%s'", s)
=======
flen, _, ok := readFloat(s)
require.Truef(t, ok, "unexpected error when parsing '%s'", s)
>>>>>>> ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))
Comment thread go/mysql/json/parser.go Outdated
Comment on lines +772 to +796
<<<<<<< HEAD
func readFloat(s string) (i int, ok bool) {
// optional sign
||||||| parent of ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))
func readFloat[S string | []byte](s S) (i int, ok bool) {
// optional sign
=======
// readFloat reads a JSON number off the front of s, returning how much of s it
// covers and how far its exponent moves the decimal point. Whether the number
// is one a double can hold is mysqlNumberFits's job; the exponent is reported so
// that question only has to be asked of numbers whose digits could reach that
// far.
//
// That distance is bounded rather than exact. An exponent can be written to more
// digits than it takes to leave every double behind, and one that reaches
// exponentCeiling below is left there instead of read out to the end. So it
// answers how far is far enough to matter and nothing finer; mysqlNumberFits
// reads the exponent itself off the number again.
//
// What counts as a number is JSON's grammar rather than Go's: a written plus,
// a missing digit on either side of the decimal point, and an integer part
// that opens with a zero are all rejected.
func readFloat[S string | []byte](s S) (i, exponent int, ok bool) {
// optional minus. JSON numbers carry no written plus.
>>>>>>> ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))
Comment thread go/mysql/json/marshal.go Outdated
Comment on lines +178 to +193
<<<<<<< HEAD
newVal := sqltypes.MakeTrusted(querypb.Type_RAW, jsonVal.MarshalSQLTo(nil))
return &newVal, nil
||||||| parent of ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))
// AppendMarshalSQL converts text JSON into a SQL expression using
// JSON_OBJECT/JSON_ARRAY syntax, writing directly to buf. It scans
// the raw bytes directly without building an intermediate tree.
//
// This has O(recursion depth) memory overhead versus O(total nodes * 72
// bytes) for the tree-based Value.MarshalSQLTo, and avoids the per-token
// heap allocations of encoding/json.Decoder.Token.
//
// The output format matches the tree-based encoder so that MySQL stores
// identical binary JSON, including preservation of large integer precision
// via bare numeric literals.
func AppendMarshalSQL(buf *bytes2.Buffer, raw []byte) error {
Comment thread go/mysql/json/marshal_test.go Outdated
Comment on lines +88 to +93
<<<<<<< HEAD
||||||| parent of ec6fdee983 (mysql/json: read numbers the way MySQL does (#20722))

// TestAppendMarshalSQLDepthLimit verifies that AppendMarshalSQL enforces
// the same nesting depth limit as Parser.Parse.
func TestAppendMarshalSQLDepthLimit(t *testing.T) {
marshal.go and marshal_test.go stay as release-23.0 has them: the
upstream change there adapted AppendMarshalSQL's readFloat call, and
that writer does not exist on this branch. parser.go takes the upstream
readFloat wholesale, and parser_test.go keeps this branch's t.Fatalf
style around the widened readFloat return.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
@arthurschreiber arthurschreiber removed Skip CI Skip CI actions from running Merge Conflict labels Jul 28, 2026
Copilot AI review requested due to automatic review settings July 28, 2026 22:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@arthurschreiber
arthurschreiber marked this pull request as ready for review July 28, 2026 22:56
@arthurschreiber
arthurschreiber enabled auto-merge (squash) July 28, 2026 22:56
@github-actions

Copy link
Copy Markdown
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:

  • Push additional commits to the associated branch.
  • Remove the stale label.
  • Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

@github-actions github-actions Bot added the Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. label Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backport This is a backport Component: Evalengine changes to the evaluation engine Component: Query Serving Stale Marks PRs as stale after a period of inactivity, which are then closed after a grace period. Type: Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants