Implement and test top-level all_of #392
Merged
Conversation
This file contains hidden or 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
f56c519 to
7bb7667
Compare
863bc53 to
2ba0a6f
Compare
2ba0a6f to
5e096fe
Compare
|
|
b5ba782 to
b39b987
Compare
a0cccb4 to
fa00135
Compare
b39b987 to
ce08cb9
Compare
fa00135 to
583817a
Compare
ce08cb9 to
d5adb32
Compare
583817a to
3477d47
Compare
myronmarston
requested changes
Apr 1, 2025
Collaborator
myronmarston
left a comment
There was a problem hiding this comment.
Looking good! Left some feedback.
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
myronmarston
reviewed
Apr 1, 2025
Collaborator
myronmarston
left a comment
There was a problem hiding this comment.
Also, can we add some coverage for how all_of interacts with not and empty filters?
d5adb32 to
d2e7b15
Compare
1f27653 to
a5d6764
Compare
myronmarston
requested changes
Apr 4, 2025
Collaborator
myronmarston
left a comment
There was a problem hiding this comment.
This is coming along nicely! Left some more feedback. Almost ready to merge...
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
elasticgraph-graphql/spec/integration/elastic_graph/graphql/datastore_query/filtering_spec.rb
Show resolved
Hide resolved
elasticgraph-graphql/spec/unit/elastic_graph/graphql/datastore_query/filtering_spec.rb
Outdated
Show resolved
Hide resolved
38f96fe to
ce975cc
Compare
d2e7b15 to
0f76b93
Compare
ce975cc to
9b222f1
Compare
0f76b93 to
f651f46
Compare
9b222f1 to
6411b5e
Compare
6411b5e to
9d53588
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a follow up to the schema change in #355. While this PR mostly updates the unit, integration, and acceptance tests, there is a key implementation change in the filter interpreter such that
all_offilters will now build a new bool sub-filter for each subexpression and push it onto the parent’s:filterarray. This ensures an ANDing across the subexpressions.Performance benchmarking
Given the above change will result in an extra bool filter term, I performed benchmarking using the
test_es_query.rbscript adapted to an all_of query with nested any_satisfy blocks.Results from current schema:
Average took value: 504ms
Median took value: 454ms
P99 took value: 1190ms
vs PR schema:
Average took value: 505ms
Median took value: 448ms
P99 took value: 1054ms
We can see there is no significant impact to performance with the additional bool filter terms.