Introduce LargeNumHitsTopDocsCollectorManager#15660
Conversation
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left a couple of comments, thanks for working on this!
| final TopDocs[] topDocs = new TopDocs[collectors.size()]; | ||
| int i = 0; | ||
| for (LargeNumHitsTopDocsCollector collector : collectors) { | ||
| topDocs[i++] = collector.topDocs(); |
There was a problem hiding this comment.
This will throw if numHits is 0. It's an edge case. Also an existing small issue when interacting directly with the collector. Should we reject 0 and negative values in the constructor? Would make sense to do the same in the collector as well, perhaps as a follow-up.
There was a problem hiding this comment.
I believe we also need to handle the case where a collector returns 0 hits. This is really an existing problem with the collector impl. topDocs should perhaps return empty top docs in that case instead of throwing. We need a test for that scenario as well.
There was a problem hiding this comment.
Should we reject 0 and negative values in the constructor
Yes, we already have this logic.
I believe we also need to handle the case where a collector returns 0 hits.
Handled that case and add a new test for it.
There was a problem hiding this comment.
That looks good. I wonder if we should have modified the collector though to not throw. What do you think?
There was a problem hiding this comment.
That makes sense, we should make the behavior consistent between the collector and the collector manager, I'll open another PR for this change.
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
One minor comment on testing. Thanks!
| writer.commit(); | ||
| writer.addDocument(new Document()); | ||
| writer.commit(); | ||
| writer.close(); |
There was a problem hiding this comment.
could you clarify why we need a different directory, writer, reader and also a custom executor in this test, compared to the other tests?
There was a problem hiding this comment.
The index writer used here is to make sure we generate 2 segments, one segment would have non-zero search hits, another segment would have zero search hits because no document containing the search field; the custom executor is to force one collector per segment, if not the 2 segments may share one collector, then the zero search hits in one collector test case wouldn't be covered.
| final TopDocs[] topDocs = new TopDocs[collectors.size()]; | ||
| int i = 0; | ||
| for (LargeNumHitsTopDocsCollector collector : collectors) { | ||
| topDocs[i++] = collector.topDocs(); |
There was a problem hiding this comment.
That looks good. I wonder if we should have modified the collector though to not throw. What do you think?
2 participants
Description
This pr introduces LargeNumHitsTopDocsCollectorManager and switches TestLargeNumHitsTopDocsCollector.java to use search concurrency and move away from the deprecated search(Query, Collector) method.
Relates to #12892.