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.

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

fix bookmark passing with text indexes #3116

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Conversation

tonysun83
Copy link
Contributor

@tonysun83 tonysun83 commented Aug 31, 2020

Overview

Previously, we passed in the unpacked version of the bookmark with
the cursor inside the options field. This worked fine for _find because
we didn't need to return it to the user. But for _explain, we return
the value back as unpacked tuple instead of a string and jiffy:encode/1
complains. Now we correctly extract the bookmark out of options, unpack
it, and then pass it separately in it's own field. This way options
retains it's original string form for the user so that invalid_ejson
is not thrown.

Testing recommendations

Regression tests should pass. If you create a text index, simply use _explain with a bookmark and verify the results.

Related Issues or Pull Requests

Checklist

Previously, we passed in the unpacked version of the bookmark with
the cursor inside the options field. This worked fine for _find because
we didn't need to return it to the user. But for _explain, we return
the value back as unpacked tuple instead of a string and jiffy:encode/1
complains. Now we correctly extract the bookmark out of options, unpack
it, and then pass it separately in it's own field. This way options
retains it's original string form for the user so that invalid_ejson
is not thrown.
@tonysun83
Copy link
Contributor Author

text index tests also pass with full regression

Copy link
Member

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@tonysun83 tonysun83 merged commit 0c3c4b6 into master Aug 31, 2020
@tonysun83 tonysun83 deleted the fix-explain-text-indexes branch August 31, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants