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.

Sign up for GitHub

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

Jump to bottom

LUCENE-10395: Introduce TotalHitCountCollectorManager #622

Merged
merged 4 commits into from Jan 31, 2022
Merged

LUCENE-10395: Introduce TotalHitCountCollectorManager #622

merged 4 commits into from Jan 31, 2022

Conversation

javanna
Copy link
Contributor

@javanna javanna commented Jan 25, 2022

This commit introduces TotalHitCountCollectorManager and replaces some more IndexSearcher#search(Query, Collector) usages with its corresponding variant that accepts a CollectorManager. Places where TotalHitCountCollector was used as the only collector, which are not replaced with IndexSearcher#count as part of #612, are migrated so that they use the newly introduced TotalHitCountCollectorManager instead.

This is another one of the many steps required to resolve LUCENE-10002

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Lucene maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

javanna
javanna commented Jan 25, 2022
View reviewed changes
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java
new MatchAllDocsQuery(),
MultiCollector.wrap(totalHitCount, new DocValuesStatsCollector(stats)));

searcher.search(new MatchAllDocsQuery(), new DocValuesStatsCollector(stats));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am possibly missing something here: two collectors were used, but the total hits were never retrieved from the total hit count collector. Removing its usage seems ok according to tests. Is this ok to change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm good question. I think so, but would like @gsmiller to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also revert this bit for now if we are not sure, this change is not strictly needed and I will encounter it again soon when trying to get rid of usages of the search method that takes a collector as argument

@javanna javanna changed the title Introduce TotalHitCountCollectorManager LUCENE-10002: Introduce TotalHitCountCollectorManager Jan 25, 2022
jpountz
jpountz reviewed Jan 25, 2022
View reviewed changes
lucene/core/src/java/org/apache/lucene/search/TotalHitCountCollectorManager.java Show resolved Hide resolved
lucene/core/src/test/org/apache/lucene/search/TestLRUQueryCache.java Outdated Show resolved Hide resolved
lucene/misc/src/test/org/apache/lucene/misc/search/TestDocValuesStatsCollector.java
new MatchAllDocsQuery(),
MultiCollector.wrap(totalHitCount, new DocValuesStatsCollector(stats)));

searcher.search(new MatchAllDocsQuery(), new DocValuesStatsCollector(stats));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm good question. I think so, but would like @gsmiller to confirm.

lucene/misc/src/test/org/apache/lucene/misc/search/TestMemoryAccountingBitsetCollector.java Show resolved Hide resolved
@javanna javanna requested a review from jpountz January 26, 2022 08:37
jpountz
jpountz approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

LGTM, can you add an entry to the changelog about this new addition under version 9.1?

lucene/core/src/test/org/apache/lucene/search/TestLRUQueryCache.java Outdated Show resolved Hide resolved
@javanna
Copy link
Contributor Author

javanna commented Jan 28, 2022

can you add an entry to the changelog about this new addition under version 9.1?

Sure! I see that in the changelog I need to specify the lucene issue, and this one was developed under LUCENE-10002 which is though not completed with this PR. Maybe it makes sense that I create a specific lucene issue for this addition then?

@jpountz
Copy link
Contributor

jpountz commented Jan 28, 2022

+1

@javanna javanna changed the title LUCENE-10002: Introduce TotalHitCountCollectorManager LUCENE-10395: Introduce TotalHitCountCollectorManager Jan 28, 2022
@javanna
Copy link
Contributor Author

javanna commented Jan 31, 2022

This should be ready now ;)

@jpountz jpountz merged commit df12e2b into apache:main Jan 31, 2022
jpountz pushed a commit that referenced this pull request Jan 31, 2022
@javanna @jpountz
LUCENE-10395: Introduce TotalHitCountCollectorManager (#622)
0c97682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reta reta reta left review comments

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@javanna @jpountz @reta