Introduce CachingCollectorManager to parallelize search when using CachingCollector#16247
Introduce CachingCollectorManager to parallelize search when using CachingCollector#16247gaobinlong wants to merge 15 commits into
Conversation
…chingCollector 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.
Great to see movement in this area, thanks for working on this @gaobinlong ! I left some comments
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
github-actions
Bot
added
module:core/store
module:analysis
module:core/index
module:demo
module:highlighter
module:benchmark
module:suggest
module:spatial
module:luke
module:core/codecs
module:facet
module:core/FSTs
module:queryparser
module:join
module:classification
module:test-framework
module:replicator
module:expressions
module:sandbox
labels
|
@javanna all comments are addressed yet, please help to review again, thanks! |
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
a couple more comments, this is close! Thanks again!
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
Thanks for all the work, I left a new batch of comments, I 'd expect these to be the last ones.
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.
last remarks. LGTM otherwise
| Collection<SearchGroup<T>> topSearchGroups = | ||
| (Collection<SearchGroup<T>>) firstRoundResults[resultIdx++]; | ||
| if (topSearchGroups.isEmpty()) { | ||
| return new TopGroups<>(new SortField[0], new SortField[0], 0, 0, new GroupDocs[0], Float.NaN); |
There was a problem hiding this comment.
when we exit early in this scenario, matchingGroups and matchingGroupHeads are not set. I suspect this will cause NPEs down the line for users relying on those two being always non-null.
There was a problem hiding this comment.
Set them to non-null values and added some tests.
There was a problem hiding this comment.
I think you could have set them to actual values being computed, as opposed to always empty?
There was a problem hiding this comment.
That calls for either adjusting the test you added or introducing a new that hits that scenario where with the current changes we lose info when we hit the isEmpty conditional.
There was a problem hiding this comment.
Changed that, thanks!
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
Signed-off-by: Binlong Gao <gbinlong@amazon.com>
2 participants
Description
This PR introduces CachingCollectorManager, switches GroupingSearch to use search concurrency and move away from the deprecated search(Query, Collector) method.
In addition, remove experimental GroupingSearch constructor that takes a GroupSelector as argument in favor of providing a GroupSelector supplier.
Relates to #12892.