-
-
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_REMOVE on IndexedJsonDocument
#8103
Changes from 3 commits
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 |
---|---|---|
|
@@ -50,24 +50,25 @@ func getPreviousKey(ctx context.Context, cur *cursor) ([]byte, error) { | |
|
||
// newJsonCursor takes the root node of a prolly tree representing a JSON document, and creates a new JsonCursor for reading | ||
// JSON, starting at the specified location in the document. | ||
func newJsonCursor(ctx context.Context, ns NodeStore, root Node, startKey jsonLocation) (*JsonCursor, error) { | ||
func newJsonCursor(ctx context.Context, ns NodeStore, root Node, startKey jsonLocation, forRemoval bool) (*JsonCursor, bool, error) { | ||
cur, err := newCursorAtKey(ctx, ns, root, startKey.key, jsonLocationOrdering{}) | ||
if err != nil { | ||
return nil, err | ||
return nil, false, err | ||
} | ||
previousKey, err := getPreviousKey(ctx, cur) | ||
if err != nil { | ||
return nil, err | ||
return nil, false, err | ||
} | ||
jsonBytes := cur.currentValue() | ||
jsonDecoder := ScanJsonFromMiddleWithKey(jsonBytes, previousKey) | ||
|
||
jcur := JsonCursor{cur: cur, jsonScanner: jsonDecoder} | ||
err = jcur.AdvanceToLocation(ctx, startKey) | ||
|
||
found, err := jcur.AdvanceToLocation(ctx, startKey, forRemoval) | ||
if err != nil { | ||
return nil, err | ||
return nil, found, err | ||
} | ||
return &jcur, nil | ||
return &jcur, found, nil | ||
} | ||
|
||
func (j JsonCursor) Valid() bool { | ||
|
@@ -122,20 +123,20 @@ func (j *JsonCursor) isKeyInChunk(path jsonLocation) bool { | |
return compareJsonLocations(path, nodeEndPosition) <= 0 | ||
} | ||
|
||
func (j *JsonCursor) AdvanceToLocation(ctx context.Context, path jsonLocation) error { | ||
func (j *JsonCursor) AdvanceToLocation(ctx context.Context, path jsonLocation, forRemoval bool) (found bool, err error) { | ||
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. Some method docs would help readers quickly understand the params without having to read the implementation; especially since this method is exposed outside the package, it would be helpful to add docs. For example, it seems important for callers to know that if the third param is passed as 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. Added a docstring. |
||
if !j.isKeyInChunk(path) { | ||
// Our destination is in another chunk, load it. | ||
err := Seek(ctx, j.cur.parent, path.key, jsonLocationOrdering{}) | ||
if err != nil { | ||
return err | ||
return false, err | ||
} | ||
j.cur.nd, err = fetchChild(ctx, j.cur.nrw, j.cur.parent.currentRef()) | ||
if err != nil { | ||
return err | ||
return false, err | ||
} | ||
previousKey, err := getPreviousKey(ctx, j.cur) | ||
if err != nil { | ||
return err | ||
return false, err | ||
} | ||
j.jsonScanner = ScanJsonFromMiddleWithKey(j.cur.currentValue(), previousKey) | ||
} | ||
|
@@ -150,16 +151,21 @@ func (j *JsonCursor) AdvanceToLocation(ctx context.Context, path jsonLocation) e | |
// there is no path greater than the end-of-document path, which is always the last key. | ||
panic("Reached the end of the JSON document while advancing. This should not be possible. Is the document corrupt?") | ||
} else if err != nil { | ||
return err | ||
return false, err | ||
} | ||
cmp = compareJsonLocations(j.jsonScanner.currentPath, path) | ||
} | ||
// If the supplied path doesn't exist in the document, then we want to stop the cursor at the start of the point | ||
// were it would appear. This may mean that we've gone too far and need to rewind one location. | ||
if cmp > 0 { | ||
j.jsonScanner = previousScanner | ||
return false, nil | ||
} | ||
// If the element is going to be removed, rewind the scanner one location, to before the key of the removed object. | ||
if forRemoval { | ||
j.jsonScanner = previousScanner | ||
} | ||
return nil | ||
return true, nil | ||
} | ||
|
||
func (j *JsonCursor) AdvanceToNextLocation(ctx context.Context) (crossedBoundary bool, err error) { | ||
|
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) Would be nice to give the return values names, or to document them (the boolean in particular) so that it's easier for future readers to quickly understand what the boolean return param indicates.
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.
Added a comment and return variable names.