-
-
Notifications
You must be signed in to change notification settings - Fork 498
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
Optimize JSON_SET and JSON_REPLACE on IndexedJsonDocument
#8107
Conversation
@nicktobey DOLT
|
IndexedJsonDocument
IndexedJsonDocument
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.
Looks good! Just a couple comments about two pieces of duplicated code that I noticed.
// The supplied path may be 0-indexing into a scalar, which is the same as referencing the scalar. Remove | ||
// the index and try again. |
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.
This is interesting. I didn't know MySQL's JSON path logic would allow this. Would this logic be worth considering moving into JsonCursor
? I'm wondering if this is more generic than just for JSON_SET
and would make sense to support for other JSON operations.
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.
There's definitely cases that we don't cover here, such as including the [0] index in the middle of a path.
Example: the number in { "a": { "b": 1 } }
could be accessed via the path $[0].a[0].b[0]
.
This isn't the right place for a comprehensive solution, this is just to handle some cases that we currently test for. A comprehensive solution would go somewhere else, but I don't think is worth implementing unless a user actually needs this behavior, given how odd it is.
convertToIndexedJsonDocument := func(t *testing.T, s interface{}) interface{} { | ||
return newIndexedJsonDocumentFromValue(t, ctx, ns, s) | ||
} |
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.
(minor) this function is defined in at least 6 places. Might be a small improvement to pull that out into a shared variable that all the tests could reuse. Not a huge deal, but could cut down on some duplicated test code.
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.
The function is a closure that captures the context and node store from the surrounding block. Pulling it out makes that more complicated and doesn't seem worth it.
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
This PR includes a new implementation of the JSON_SET and JSON_REPLACE functions that leverage the new indexed JSON storage format.
For JSON documents that span multiple chunks, only the affected chunks need to be loaded and modified, allowing operations to scale with the size of the removed value instead of the size of the entire document.