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
LUCENE-10378 Implement Weight#count for PointRangeQuery #658
Conversation
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 left some minor comments in addition to Ignacio's.
lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java
Outdated
Show resolved
Hide resolved
Still needs some logic to handle leaf nodes Refactored common check args to a separate function
…at matched, fix leafNode counting logic, edit comment about the condition under which our optimization is fired
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 have not added any new tests. This new Weight#count
implementation merely changes the flow of the code and not the actual correctness itself. The upstream caller aka IndexSearcher#count
still has numerous tests for PointRangeQuery
in TestPointQueries
lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java
Outdated
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java
Outdated
Show resolved
Hide resolved
…BiFunction and Predicate for internal node and leaf node respectively. Simplify code overall
lucene/core/src/java/org/apache/lucene/search/PointRangeQuery.java
Outdated
Show resolved
Hide resolved
…ren lie in the range are directly added to the matchingNodeCount array, no intermediaries
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.
That is very close. I think we should add a test for this change and then we are ready to merge.
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.
LGTM
I will push this tomorrow if it is ok with you @gautamworah96 |
Yes. I was wondering why you had kept it open :) |
Implement Weight#count for PointRangeQuery to provide a faster way to calculate the number of matching range docs when each doc has at-most one point and the points are 1-dimensional.
@iverase I see you already opened a backport PR. Thanks! I'll approve that as well |
Implement Weight#count for PointRangeQuery to provide a faster way to calculate the number of matching range docs when each doc has at-most one point and the points are 1-dimensional.
Implement Weight#count for PointRangeQuery to provide a faster way to calculate the number of matching range docs when each doc has at-most one point and the points are 1-dimensional.
Description
Implement Weight#count on PointRangeQuery. Also fixed a small style inconsistency that I noticed
Issue: https://issues.apache.org/jira/browse/LUCENE-10378
Solution
Use a similar approach to what we've done for
TermQuery
orNormsFieldExistsQuery
. Added initial checks for validating the input and then checking if all documents have at-least one point, the field is single dimensional and the number of points equals the number of documents.Note: @jpountz I think I've misinterpreted your
by only counting matches on the two leaves that cross with the query.
comment and have implemented a brute force approach.Tests
Tests have not been added. I'm in the process of writing them
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.