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 – sorted limited offset mount queries #124

Merged
merged 15 commits into from
Mar 23, 2019

Conversation

michaelavila
Copy link
Contributor

@michaelavila michaelavila commented Mar 22, 2019

Based on: #123
Supersedes: #121

Use NaiveLimit and NaiveOffset to add limit/offset features to queries on mount.

@ghost ghost assigned michaelavila Mar 22, 2019
@ghost ghost added the status/in-progress In progress label Mar 22, 2019
mount/mount.go Outdated Show resolved Hide resolved
@michaelavila
Copy link
Contributor Author

I'm working on more tests now.

@michaelavila michaelavila force-pushed the fix/sorted-limited-offset-mount-queries branch from bff153a to 9550a82 Compare March 22, 2019 16:46
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

(sorry for leaving you with the annoying part)

mount/mount.go Outdated Show resolved Hide resolved
query/query_impl.go Outdated Show resolved Hide resolved
mount/mount.go Outdated
if !f.Filter(head.next.Entry) {
continue
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Honestly, we could probably switch to the naive filter as well. I'm not sure why I did it this way (well, it'll be slightly faster but not by much).

Copy link

@eingenito eingenito Mar 22, 2019

Choose a reason for hiding this comment

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

@Stebalien Actually we're finding that we might have to use the NaiveFilter implementation because if the underlying datastores support filter they're going to be filtering only on the subkey (if they filter on key) meaning they'll exclude their mount prefix. So we probably have to treat filters in the same way we do offsets by stashing them and then removing them from the Query that is passed to each datastore, and then applying them at then end when the keys are full keys. So we're going to do as you suggest here and use NaiveFilter. It reads a little clearer too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a commit that fixes filtering by using NaiveFilter: 5b4de5b

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good catch. We definitely need to remove the filters from the query we're forwarding. Actually, we should probably just create a new query.

@michaelavila michaelavila force-pushed the fix/sorted-limited-offset-mount-queries branch from f4f30df to 55e8dfe Compare March 22, 2019 18:06
@michaelavila michaelavila force-pushed the fix/sorted-limited-offset-mount-queries branch from 55e8dfe to 1633052 Compare March 22, 2019 18:13
@michaelavila michaelavila changed the title [WIP] Fix – sorted limited offset mount queries Fix – sorted limited offset mount queries Mar 22, 2019
query/query.go Outdated Show resolved Hide resolved
@michaelavila michaelavila force-pushed the fix/sorted-limited-offset-mount-queries branch from 340ca11 to e192b45 Compare March 22, 2019 23:34
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM!

@Stebalien Stebalien merged commit bce485c into master Mar 23, 2019
@Stebalien Stebalien deleted the fix/sorted-limited-offset-mount-queries branch March 23, 2019 04:36
@ghost ghost removed the status/in-progress In progress label Mar 23, 2019
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.

3 participants