Skip to content
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

Jump to bottom

JSON Values #332

Merged
merged 9 commits into from
Mar 16, 2021
Merged

JSON Values #332

merged 9 commits into from
Mar 16, 2021

Conversation

andy-wm-arthur
Copy link
Contributor

@andy-wm-arthur andy-wm-arthur commented Mar 11, 2021 •
edited
Loading

This change moves the engine towards representing JSON values as Go values rather than strings.

I'm strongly considering a

type JSONValue interface{}

There seems to be no precedent for this

@andy-wm-arthur andy-wm-arthur changed the title Andy/json contains [WIP] JSON Values Mar 11, 2021
@andy-wm-arthur andy-wm-arthur marked this pull request as ready for review March 12, 2021 18:01
@andy-wm-arthur
Copy link
Contributor Author

I'm strongly considering a type JSONValue interface{}. There seems to be no precedent for this in the codebase, but I believe it will enable better performance and correctness. The type of a field value is used to fork logic throughout the code base and without a json value type, we can't differentiate between something like INT(11) and JSON(11). A json value type would also designate a value as a valid json document. This PR traverses the entire document to validate it on every Convert() call.

zachmu
zachmu reviewed Mar 12, 2021
View reviewed changes
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

I think this is the right call given how we've talked about the storage engine implementation (and thinking about how to implement various JSON functions)

@andy-wm-arthur andy-wm-arthur changed the title [WIP] JSON Values JSON Values Mar 16, 2021
zachmu
zachmu approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM. All my comments are basically about killing off the non-json array type

sql/arraytype.go
val, err := json.Marshal(v)
if err != nil {
return sqltypes.Value{}, err
var val []byte
Copy link
Member

Choose a reason for hiding this comment

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

Can we actually kill off this type? It's not actually supported in MySQL, so the semantics of it are very unclear. And it's getting all muddled in the with the JSON array logic. I'd rather have exactly one well supported implementation of arrays (use JSON).

Also kill off the EXPLODE function at the same time.

Could be a follow up after this PR to keep things smaller.

sql/expression/function/arraylength.go
if child == nil {
return nil, nil
}

if val, ok := child.(sql.JSONValue); ok {
Copy link
Member

Choose a reason for hiding this comment

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

Same comment here. This function is not part of MySQL and has poorly defined semantics. Kill it.

sql/json_value.go
// SearchableJSONValue is JSONValue supporting in-place access operations.
// The query engine can utilize these optimized access methods improve performance
// by minimizing the need to unmarshall a JSONValue into a JSONDocument.
type SearchableJSONValue interface {
Copy link
Member

Choose a reason for hiding this comment

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

Should have method doc for these interface methods. Fine to sketch out if they're not finalized

@andy-wm-arthur andy-wm-arthur merged commit 1d9d502 into master Mar 16, 2021
@andy-wm-arthur andy-wm-arthur deleted the andy/json-contains branch March 16, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachmu zachmu zachmu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andy-wm-arthur @zachmu