-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
perf(gatsby): Support lte
for indexed fast filters
#22932
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
8a15a90
Rename function to better fit what it returns
pvdz c1a5984
Use `filterCacheKey` more consistently
pvdz 8849d7a
Consistency around `nodesPerValue`
pvdz ca5576e
Fix the name and typing of array of sets of nodes
pvdz c0b3c5e
Fix typing, fix perf bug
pvdz 18f78f8
Extend the `FilterCache` type to support op-specific meta data
pvdz 4074561
Replace wild usage of `FilterType`
pvdz 93a3243
Refactor some old comments / naming to be consistent with current cod…
pvdz 1c56b81
Suggested changes
pvdz 333dd02
Enable `lte` for fast filters
pvdz 2f85a1d
Mandatory lint/type fix commit
pvdz 5603814
And the loki commit
pvdz 9232f9a
Enable/disable query and eleeMatch ops in one place
pvdz 04349bb
Fix tests, fix lte binary search lookup
pvdz c3525c2
Support `null` properly and handle not found binary search case
pvdz 493b85d
Sigh, this is why I had to introduce that type in the first place
pvdz b746293
Forgot to remove an argument from followup work
pvdz 8aebc8f
Merge branch 'master' into ltegte
pvdz 54e9cf8
Apply suggestions
pvdz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Enable
lte
for fast filters
- Loading branch information
commit 333dd02742e1fd879679e8fef4837891fb11eaae
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Question here - do we operate on materialized view values or node values? Is it possible we get actual Date type here - and if so, can that cause problems somewhere in the code?
I tried to quickly skim through the code and seems like we only use
<=
and>
comparisons, so we should be good even if we end up with unexpected types ... as long as you can actually use comparisons there to sort values out?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.
As far as I know the values are only ever str/num/bool and a hint of null.
@vladar or @freiksenet can answer this better.
This approach would later be used for
ne
andin
as well so it's good to know regardless.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 did some checks and filter value is always a
string
. Even for dates. So it works fine when the node has a date field expressed as astring
.But there is a bug when a node field contains an instance of
Date
. Sift filtering is broken for this situation (as we compare string vs date). At least I tried it and it didn't work.The proper fix would be to convert the filter value to
Date
when running sift (docs).An alternative option would be to always store dates as strings in nodes (maybe a better one). But this may be a breaking change.
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.
Added this PR with a failing test case (skipped for now): #23096
We should address it in v3 (as it is a breaking change). It will also ensure that filtering with indexes is consistent with sift.
Also, in the worst case, it will just fallback to querying without index so I think this shouldn't be a blocker for this PR.