javanna commented on code in PR #15557:
URL: https://github.com/apache/lucene/pull/15557#discussion_r3110494559
##########
lucene/grouping/src/test/org/apache/lucene/search/grouping/TestAllGroupsCollector.java:
##########
@@ -121,12 +127,13 @@ private void addGroupField(Document doc, String
groupField, String value) {
doc.add(new SortedDocValuesField(groupField, new BytesRef(value)));
}
- private AllGroupsCollector<?> createRandomCollector(String groupField) {
+ private AllGroupsCollectorManager<?> createRandomCollectorManager(String
groupField) {
if (random().nextBoolean()) {
- return new AllGroupsCollector<>(new TermGroupSelector(groupField));
+ return new AllGroupsCollectorManager<>(() -> new
TermGroupSelector(groupField));
} else {
ValueSource vs = new BytesRefFieldSource(groupField);
- return new AllGroupsCollector<>(new ValueSourceGroupSelector(vs, new
HashMap<>()));
+ return new AllGroupsCollectorManager<>(
+ () -> new ValueSourceGroupSelector(vs, new HashMap<>()));
Review Comment:
it does not matter in this case given that the map is empty, but technically
all selectors should share the same map instance? I would correct this just to
prevent similar mistakes in the future.
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/AllGroupsCollectorManager.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.search.grouping;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A CollectorManager implementation for AllGroupsCollector.
+ *
+ * @lucene.experimental
+ */
+public class AllGroupsCollectorManager<T>
+ implements CollectorManager<AllGroupsCollector<T>, Collection<T>> {
+
+ private final Supplier<GroupSelector<T>> groupSelectorFactory;
+
+ /**
+ * Creates a new AllGroupsCollectorManager.
+ *
+ * @param groupSelectorFactory factory to create group selectors for each
collector
+ */
+ public AllGroupsCollectorManager(Supplier<GroupSelector<T>>
groupSelectorFactory) {
+ this.groupSelectorFactory = groupSelectorFactory;
+ }
+
+ @Override
+ public AllGroupsCollector<T> newCollector() {
+ return new AllGroupsCollector<>(groupSelectorFactory.get());
+ }
+
+ @Override
+ public Collection<T> reduce(Collection<AllGroupsCollector<T>> collectors) {
+ if (collectors.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ if (collectors.size() == 1) {
+ return collectors.iterator().next().getGroups();
+ }
Review Comment:
nit: are the above two shortcuts really needed? could we omit them?
##########
lucene/grouping/src/java/org/apache/lucene/search/grouping/AllGroupsCollectorManager.java:
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.search.grouping;
+
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Supplier;
+import org.apache.lucene.search.CollectorManager;
+
+/**
+ * A CollectorManager implementation for AllGroupsCollector.
+ *
+ * @lucene.experimental
+ */
+public class AllGroupsCollectorManager<T>
+ implements CollectorManager<AllGroupsCollector<T>, Collection<T>> {
+
+ private final Supplier<GroupSelector<T>> groupSelectorFactory;
+
+ /**
+ * Creates a new AllGroupsCollectorManager.
+ *
+ * @param groupSelectorFactory factory to create group selectors for each
collector
+ */
+ public AllGroupsCollectorManager(Supplier<GroupSelector<T>>
groupSelectorFactory) {
+ this.groupSelectorFactory = groupSelectorFactory;
+ }
+
+ @Override
+ public AllGroupsCollector<T> newCollector() {
+ return new AllGroupsCollector<>(groupSelectorFactory.get());
+ }
+
+ @Override
+ public Collection<T> reduce(Collection<AllGroupsCollector<T>> collectors) {
+ if (collectors.isEmpty()) {
+ return Collections.emptyList();
+ }
+
+ if (collectors.size() == 1) {
+ return collectors.iterator().next().getGroups();
+ }
+
+ // Merge groups from all collectors
+ Set<T> allGroups = new HashSet<>();
+ for (AllGroupsCollector<T> collector : collectors) {
+ Collection<T> groups = collector.getGroups();
+ if (groups != null) {
Review Comment:
can the groups ever be null? I believe it is initialized with each
`AllGroupsCollector` instance, hence there is no need to check for null here.
Does that make sense?
--
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]
