-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add Bookmark support for mango json queries #740
Conversation
This adds a bookmark that is sent with each query and can be used to continue a query from a specific key. This will allow users to paginate mango queries.
testing a basic case where no results are returned, I get a response:
However, passing "nil" back as a bookmark returns an error:
|
suggest adding a pagination test where each document emits the same value into the index to ensure progress is made using |
Thanks @willholley I've fixed the issue and added another test. |
nil; | ||
unpack(Packed) -> | ||
try | ||
binary_to_term(couch_util:decodeBase64Url(Packed)) |
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 part got me thinking a bit. What is a valid vs invalid bookmark? So when unpacked, a json bookmark is a start_key and end_key range, and a text bookmark is a node+shard range. So the composition is slightly different. So a text index bookmark should be considered invalid for a json index bookmark right? When I tried that particular scenario, the bookmark passed correctly Here's a sample text bookmark: "g2wAAAACaAJkAA5zdGFydGtleV9kb2NpZG0AAAAkZTkwMDAwMWQtYmM0OC00OGE2LTliMWEtYWM5YTFmNWQxYTAzaAJkAAhzdGFydGtleWsAAStq"
.
If you did it the other way around...substitute a json index bookmark into a text index bookmark, it fails with invalid_bookmark.
Wondering if we should be more consistent/strict and and verify both types of bookmarks?
catch _:_ -> | ||
?MANGO_ERROR({invalid_bookmark, Packed}) | ||
end. | ||
|
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.
missing new line
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.
I don't quite follow? Do you want me to remove that new line or add another?
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.
weird, it showed no new end of line. just normal end of file new line is fine
src/mango/src/mango_cursor_view.erl
Outdated
@@ -305,8 +315,20 @@ doc_member(Db, RowProps, Opts) -> | |||
end | |||
end. | |||
|
|||
|
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.
whitespace change
@tonysun83 thanks for the review. I've make some fixes based on your feedback |
+1 after that fixing that weird end of line issue |
This adds a bookmark that is sent with each query and can be used to continue a query from a specific key. This will allow users to paginate mango queries.
This adds a bookmark that is sent with each query and can be used to continue a query from a specific key. This will allow users to paginate mango queries.
How exactly is bookmark implmented? [I have no knowledge of Erlang, but have written a couchdb client in dart]. Is it base64 enocded of json of last index[or doc] sent to user ? |
@anuragvohraec the code is here https://github.com/apache/couchdb/blob/master/src/mango/src/mango_json_bookmark.erl |
This adds a bookmark that is sent with each query and can be used to
continue a query from a specific key. This will allow users to paginate
mango queries.
Checklist