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 query logic #39

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix query logic #39

wants to merge 6 commits into from

Conversation

Stebalien
Copy link
Member

I've written a query test suite to test all query combination.

This PR currently fixes offset handling but we still need to handle:

  • Ordering
  • Filtering

Unfortunately, these are non-trivial because we need to handle them in the following order:

  1. Prefix
  2. Filter
  3. Order
  4. Offset
  5. Limit

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.

@Stebalien
Copy link
Member Author

Stebalien commented Oct 1, 2019

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).

@Stebalien Stebalien mentioned this pull request Oct 5, 2019
@Stebalien Stebalien marked this pull request as ready for review February 12, 2020 22:16
@Stebalien Stebalien closed this Feb 12, 2020
@Stebalien Stebalien reopened this Feb 12, 2020
@Stebalien Stebalien force-pushed the fix/in-order-query branch 2 times, most recently from 80f7668 to 09bdc7d Compare February 12, 2020 22:20
@obo20
Copy link

obo20 commented Apr 23, 2020

@Stebalien what are the functionality impacts of not having this PR + the additional fixes you mentioned merged in?

@Stebalien
Copy link
Member Author

  1. The S3 datastore probably still works for blocks, but may not be safe to use for other records.
  2. The test suite isn't running so there may be other bugs that we just haven't noticed.

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.

2 participants