Skip to content

Move the JSON parser from evalengine#12757

Closed
dbussink wants to merge 4 commits intovitessio:mainfrom
planetscale:dbussink/move-json-parser
Closed

Move the JSON parser from evalengine#12757
dbussink wants to merge 4 commits intovitessio:mainfrom
planetscale:dbussink/move-json-parser

Conversation

@dbussink
Copy link
Copy Markdown
Member

@dbussink dbussink commented Mar 29, 2023

This moves the JSON evalengine implementation so we can share it more across different components.

Specifically vreplication wants to also use the same JSON parser so it can handle dealing with the MySQL types inside JSON objects better.

It also implements the same parsing for the MySQL binary JSON format and a serialization to using SQL style functions to limit typing loss as much as possible.

There's still two types that cause problems. When using TIMESTAMP or BIT types inside a JSON document it's impossible to create an SQL format compatible with those types since there's no way to create literals for those two types.

Related Issue(s)

This helps with a better fix for #12731 that handles more types correctly.

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on the CI
  • Documentation was added or is not required

@dbussink
Move the JSON parser from evalengine
b27c5ce 
This moves the JSON evalengine implementation so we can share it more
across different components.

Specifically vreplication wants to also use the same JSON parser so it
can handle dealing with the MySQL types inside JSON objects better.

It also implements the same parsing for the MySQL binary JSON format and
a serialization to using SQL style functions to limit typing loss as
much as possible.

There's still two types that cause problems. When using `TIMESTAMP` or
`BIT` types inside a JSON document it's impossible to create an SQL
format compatible with those types since there's no way to create
literals for those two types.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
@vitess-bot vitess-bot bot added the NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work label Mar 29, 2023
@vitess-bot
Copy link
Copy Markdown
Contributor

vitess-bot bot commented Mar 29, 2023

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • If this is a change that users need to know about, please apply the release notes (needs details) label so that merging is blocked unless the summary release notes document is included.
  • If a test is added or modified, there should be a documentation on top of the test to explain what the expected behavior is what the test does.

If a new flag is being introduced:

  • Is it really necessary to add this flag?
  • Flag names should be clear and intuitive (as far as possible)
  • Help text should be descriptive.
  • Flag names should use dashes (-) as word separators rather than underscores (_).

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow should be required, the maintainer team should be notified.

Bug fixes

  • There should be at least one unit or end-to-end test.
  • The Pull Request description should include a link to an issue that describes the bug.

Non-trivial changes

  • There should be some code comments as to why things are implemented the way they are.

New/Existing features

  • Should be documented, either by modifying the existing documentation or creating new documentation.
  • New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • vtctl command output order should be stable and awk-able.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from VTop, if used there.

@vitess-bot vitess-bot bot added the NeedsWebsiteDocsUpdate What it says label Mar 29, 2023
vmg
vmg approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@vmg vmg left a comment

Choose a reason for hiding this comment

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

This is just moving/renaming, nothing to review 👌

frouioui
frouioui reviewed Mar 29, 2023
View reviewed changes
go/mysql/float.go
Comment on lines +1 to +2
package mysql

Copy link
Copy Markdown
Member

@frouioui frouioui Mar 29, 2023

Choose a reason for hiding this comment

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

We need to add a header here

frouioui
frouioui reviewed Mar 29, 2023
View reviewed changes
go/mysql/json/mysql_parser_test.go
Comment on lines +1 to +2
package json

Copy link
Copy Markdown
Member

@frouioui frouioui Mar 29, 2023

Choose a reason for hiding this comment

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

same here

frouioui
frouioui approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

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

LGTM

@frouioui frouioui removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 29, 2023
@frouioui frouioui added this to the v17.0.0 milestone Mar 29, 2023
vmg and others added 3 commits March 29, 2023 12:38
@vmg
mysql: move datetime from evalengine
9901cf2 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
json: simplify binary parser
b5d9e42 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@dbussink
Move time parsing to avoid future import loops
c7dcf9f 
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
GuptaManan100 pushed a commit to planetscale/vitess that referenced this pull request Mar 29, 2023
@dbussink @vmg
Move the JSON parser from evalengine (vitessio#12757)
7478d85 
* Move the JSON parser from evalengine

This moves the JSON evalengine implementation so we can share it more
across different components.

Specifically vreplication wants to also use the same JSON parser so it
can handle dealing with the MySQL types inside JSON objects better.

It also implements the same parsing for the MySQL binary JSON format and
a serialization to using SQL style functions to limit typing loss as
much as possible.

There's still two types that cause problems. When using `TIMESTAMP` or
`BIT` types inside a JSON document it's impossible to create an SQL
format compatible with those types since there's no way to create
literals for those two types.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

* mysql: move `datetime` from `evalengine`

Signed-off-by: Vicent Marti <vmg@strn.cat>

* json: simplify binary parser

Signed-off-by: Vicent Marti <vmg@strn.cat>

* Move time parsing to avoid future import loops

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>

---------

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Vicent Marti <vmg@strn.cat>
Co-authored-by: Vicent Marti <vmg@strn.cat>
@dbussink
Copy link
Copy Markdown
Member Author

dbussink commented Mar 29, 2023

Looks like this was merged but the PR not closed 😞

@dbussink dbussink closed this Mar 29, 2023
@dbussink dbussink deleted the dbussink/move-json-parser branch March 29, 2023 18:21
@rohit-nayak-ps rohit-nayak-ps restored the dbussink/move-json-parser branch March 30, 2023 19:41
@rohit-nayak-ps rohit-nayak-ps mentioned this pull request Mar 30, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmg vmg vmg approved these changes

@frouioui frouioui frouioui approved these changes

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@systay systay Awaiting requested review from systay systay is a code owner

@GuptaManan100 GuptaManan100 Awaiting requested review from GuptaManan100

@mattlord mattlord Awaiting requested review from mattlord mattlord is a code owner

Assignees

No one assigned

Labels

Component: Evalengine changes to the evaluation engine Component: VReplication Type: Internal Cleanup

Projects

None yet

Milestone

v17.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@dbussink @vmg @frouioui