-
Notifications
You must be signed in to change notification settings - Fork 66
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 query logic #39
base: master
Are you sure you want to change the base?
Fix query logic #39
Conversation
cc @MichaelMure @ianopolous, @hinshun, @tobowers Unfortunately, I'm not sure when I'll have time to take this to completion but you should know about these issues/limitations. If any of you have the time/inclination to pick this up, I'd be happy to review a PR (and the query tests are now pretty thorough). |
13bfc9f
to
7ad790d
Compare
80f7668
to
09bdc7d
Compare
@Stebalien what are the functionality impacts of not having this PR + the additional fixes you mentioned merged in? |
|
91e4a16
to
a928190
Compare
I've written a query test suite to test all query combination.
This PR currently fixes offset handling but we still need to handle:
Unfortunately, these are non-trivial because we need to handle them in the following order:
To handle complex orders (i.e., anything other than "sort by key"), we can perform one query that handles 1-2 (prefix/filter) and then applies 3-5 via the naive query logic. However, getting this right is non-trivial.