javanna commented on code in PR #15574:
URL: https://github.com/apache/lucene/pull/15574#discussion_r3288275851
##########
lucene/grouping/src/test/org/apache/lucene/search/grouping/TestGrouping.java:
##########
@@ -1479,30 +1598,23 @@ private TopGroups<BytesRef> searchShards(
: ScoreMode.COMPLETE_NO_SCORES,
1);
final List<Collection<SearchGroup<BytesRef>>> shardGroups = new
ArrayList<>();
- List<FirstPassGroupingCollector<?>> firstPassGroupingCollectors = new
ArrayList<>();
- FirstPassGroupingCollector<?> firstPassCollector = null;
-
String groupField = "group";
+ // TermGroupSelector or ValueSourceGroupSelector
+ final boolean isTermGroupSelector = random().nextBoolean();
+
for (int shardIDX = 0; shardIDX < subSearchers.length; shardIDX++) {
+ final FirstPassGroupingCollectorManager<?>
firstPassGroupingCollectorManager =
+ createFirstPassCollectorManager(
+ isTermGroupSelector, groupField, groupSort, 0, groupOffset +
topNGroups);
- // First shard determines whether we use IDV or not;
- // all other shards match that:
- if (firstPassCollector == null) {
- firstPassCollector =
- createRandomFirstPassCollector(groupField, groupSort, groupOffset
+ topNGroups);
- } else {
- firstPassCollector =
- createFirstPassCollector(
- groupField, groupSort, groupOffset + topNGroups,
firstPassCollector);
- }
if (VERBOSE) {
System.out.println(" shard=" + shardIDX + " groupField=" +
groupField);
- System.out.println(" 1st pass collector=" + firstPassCollector);
+ System.out.println(" 1st pass collector manager=" +
firstPassGroupingCollectorManager);
}
- firstPassGroupingCollectors.add(firstPassCollector);
- subSearchers[shardIDX].search(w, firstPassCollector);
- final Collection<SearchGroup<BytesRef>> topGroups =
getSearchGroups(firstPassCollector, 0);
+ final Collection<SearchGroup<BytesRef>> topGroups =
+ toByteRefSearchGroups(
+ subSearchers[shardIDX].search(query,
firstPassGroupingCollectorManager));
Review Comment:
This will call the general `search(Query, Collector)` against the shard
searcher. That though is not overridden and will search the entire index and
not only the leaf associated with the shard searcher. I believe that's an
issue. Perhaps this showcases a weakness in the ShardSearcher implementation,
in that it exposes a new method without overriding the others, and lets
consumers call the other methods that are though not going to access only the
meant leaf. Does that make sense?
##########
lucene/grouping/src/test/org/apache/lucene/search/grouping/TestGrouping.java:
##########
@@ -1479,30 +1598,23 @@ private TopGroups<BytesRef> searchShards(
: ScoreMode.COMPLETE_NO_SCORES,
1);
final List<Collection<SearchGroup<BytesRef>>> shardGroups = new
ArrayList<>();
- List<FirstPassGroupingCollector<?>> firstPassGroupingCollectors = new
ArrayList<>();
- FirstPassGroupingCollector<?> firstPassCollector = null;
-
String groupField = "group";
+ // TermGroupSelector or ValueSourceGroupSelector
+ final boolean isTermGroupSelector = random().nextBoolean();
+
for (int shardIDX = 0; shardIDX < subSearchers.length; shardIDX++) {
+ final FirstPassGroupingCollectorManager<?>
firstPassGroupingCollectorManager =
+ createFirstPassCollectorManager(
+ isTermGroupSelector, groupField, groupSort, 0, groupOffset +
topNGroups);
- // First shard determines whether we use IDV or not;
- // all other shards match that:
- if (firstPassCollector == null) {
- firstPassCollector =
- createRandomFirstPassCollector(groupField, groupSort, groupOffset
+ topNGroups);
- } else {
- firstPassCollector =
- createFirstPassCollector(
- groupField, groupSort, groupOffset + topNGroups,
firstPassCollector);
- }
if (VERBOSE) {
System.out.println(" shard=" + shardIDX + " groupField=" +
groupField);
- System.out.println(" 1st pass collector=" + firstPassCollector);
+ System.out.println(" 1st pass collector manager=" +
firstPassGroupingCollectorManager);
}
- firstPassGroupingCollectors.add(firstPassCollector);
- subSearchers[shardIDX].search(w, firstPassCollector);
- final Collection<SearchGroup<BytesRef>> topGroups =
getSearchGroups(firstPassCollector, 0);
+ final Collection<SearchGroup<BytesRef>> topGroups =
+ toByteRefSearchGroups(
+ subSearchers[shardIDX].search(query,
firstPassGroupingCollectorManager));
Review Comment:
I would suggest making the shard searcher more robust. Override all the
public methods, by either filtering them to access only the meant leave, or
throw `UnsupportedOperationException` if they are not mean to be called
directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
