You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/02/02 20:26:43 UTC
[GitHub] [lucene] gsmiller commented on a change in pull request #632: LUCENE-10050 Remove DrillSideways#search(DrillDownQuery,Collector) in favor of DrillSideways#search(DrillDownQuery,CollectorManager)
gsmiller commented on a change in pull request #632:
URL: https://github.com/apache/lucene/pull/632#discussion_r797965578
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -192,7 +192,11 @@ protected Facets buildFacetsResult(
* ExecutorService} was supplied to the ctor, since {@code Collector}s are not thread-safe. If
* interested in concurrent drill sideways, please use one of the other static {@code search}
* methods.
+ *
+ * @deprecated This method is deprecated in interest of the {@link #search(DrillDownQuery,
+ * CollectorManager)} method.
*/
+ @Deprecated
Review comment:
Let's remove this completely on `main` and `@deprecate` on `branch_9x`. Could you open a separate PR against 9.x with the deprecated approach and remove completely in this PR please?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -285,53 +289,42 @@ public DrillSidewaysResult search(
}
final int fTopN = Math.min(topN, limit);
- if (executor != null) { // We have an executor, let use the multi-threaded version
-
- final CollectorManager<TopFieldCollector, TopFieldDocs> collectorManager =
- new CollectorManager<>() {
-
- @Override
- public TopFieldCollector newCollector() {
- return TopFieldCollector.create(sort, fTopN, after, Integer.MAX_VALUE);
- }
-
- @Override
- public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) {
- final TopFieldDocs[] topFieldDocs = new TopFieldDocs[collectors.size()];
- int pos = 0;
- for (TopFieldCollector collector : collectors)
- topFieldDocs[pos++] = collector.topDocs();
- return TopDocs.merge(sort, topN, topFieldDocs);
- }
- };
- ConcurrentDrillSidewaysResult<TopFieldDocs> r = searchConcurrently(query, collectorManager);
- TopFieldDocs topDocs = r.collectorResult;
- if (doDocScores) {
- TopFieldCollector.populateScores(topDocs.scoreDocs, searcher, query);
- }
- return new DrillSidewaysResult(
- r.facets,
- topDocs,
- r.drillDownFacetsCollector,
- r.drillSidewaysFacetsCollector,
- r.drillSidewaysDims);
+ final CollectorManager<TopFieldCollector, TopFieldDocs> collectorManager =
+ new CollectorManager<>() {
+
+ @Override
+ public TopFieldCollector newCollector() {
+ return TopFieldCollector.create(sort, fTopN, after, Integer.MAX_VALUE);
+ }
+
+ @Override
+ public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) {
+ final TopFieldDocs[] topFieldDocs = new TopFieldDocs[collectors.size()];
+ int pos = 0;
+ for (TopFieldCollector collector : collectors)
+ topFieldDocs[pos++] = collector.topDocs();
+ return TopDocs.merge(sort, topN, topFieldDocs);
+ }
+ };
+ final ConcurrentDrillSidewaysResult<TopFieldDocs> r;
+ if (executor != null) {
+ // We have an executor, let use the multi-threaded version
+ r = searchConcurrently(query, collectorManager);
} else {
+ r = searchSequentially(query, collectorManager);
Review comment:
This is so much better, thank you! It was very trappy before. If the IndexSearcher is setup with an executor, we don't leverage it with the old solution since we just create a single Collector. With this change, we can leverage concurrent search within IndexSearcher if it was setup to do so. Nice!
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/AssertingSubDocsAtOnceCollectorManager.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.facet;
+
+import java.util.Collection;
+import org.apache.lucene.search.CollectorManager;
+
+public class AssertingSubDocsAtOnceCollectorManager
Review comment:
Please make this pkg-private
##########
File path: lucene/CHANGES.txt
##########
@@ -75,6 +75,10 @@ API Changes
* LUCENE-10368: IntTaxonomyFacets has been deprecated and is no longer a supported extension point
for user-created faceting implementations. (Greg Miller)
+* LUCENE-10050: Deprecate DrillSideways#search(Query, Collector) in favor of DrillSideways#search(Query, CollectorManager).
Review comment:
minor: Please try to keep this to a column width of ~100
##########
File path: lucene/facet/src/test/org/apache/lucene/facet/AssertingSubDocsAtOnceCollectorManager.java
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.facet;
+
+import java.util.Collection;
+import org.apache.lucene.search.CollectorManager;
+
+public class AssertingSubDocsAtOnceCollectorManager
Review comment:
minor: Now that `AssertingSubDocsAtOnceCollector` is only referenced by this new class you've created, I'd suggest moving it into this class as an internal static class definition just to help organize the code a bit.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -351,45 +344,36 @@ public DrillSidewaysResult search(ScoreDoc after, DrillDownQuery query, int topN
}
final int fTopN = Math.min(topN, limit);
- if (executor != null) { // We have an executor, let use the multi-threaded version
+ final CollectorManager<TopScoreDocCollector, TopDocs> collectorManager =
+ new CollectorManager<>() {
- final CollectorManager<TopScoreDocCollector, TopDocs> collectorManager =
- new CollectorManager<>() {
+ @Override
+ public TopScoreDocCollector newCollector() {
+ return TopScoreDocCollector.create(fTopN, after, Integer.MAX_VALUE);
+ }
- @Override
- public TopScoreDocCollector newCollector() {
- return TopScoreDocCollector.create(fTopN, after, Integer.MAX_VALUE);
- }
-
- @Override
- public TopDocs reduce(Collection<TopScoreDocCollector> collectors) {
- final TopDocs[] topDocs = new TopDocs[collectors.size()];
- int pos = 0;
- for (TopScoreDocCollector collector : collectors)
- topDocs[pos++] = collector.topDocs();
- return TopDocs.merge(topN, topDocs);
- }
- };
- ConcurrentDrillSidewaysResult<TopDocs> r = searchConcurrently(query, collectorManager);
- return new DrillSidewaysResult(
- r.facets,
- r.collectorResult,
- r.drillDownFacetsCollector,
- r.drillSidewaysFacetsCollector,
- r.drillSidewaysDims);
+ @Override
+ public TopDocs reduce(Collection<TopScoreDocCollector> collectors) {
+ final TopDocs[] topDocs = new TopDocs[collectors.size()];
+ int pos = 0;
+ for (TopScoreDocCollector collector : collectors) topDocs[pos++] = collector.topDocs();
Review comment:
Could you please add brackets and move this to its own line to help with readability. Maybe this was a spotless thing?
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -285,53 +289,42 @@ public DrillSidewaysResult search(
}
final int fTopN = Math.min(topN, limit);
- if (executor != null) { // We have an executor, let use the multi-threaded version
-
- final CollectorManager<TopFieldCollector, TopFieldDocs> collectorManager =
- new CollectorManager<>() {
-
- @Override
- public TopFieldCollector newCollector() {
- return TopFieldCollector.create(sort, fTopN, after, Integer.MAX_VALUE);
- }
-
- @Override
- public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) {
- final TopFieldDocs[] topFieldDocs = new TopFieldDocs[collectors.size()];
- int pos = 0;
- for (TopFieldCollector collector : collectors)
- topFieldDocs[pos++] = collector.topDocs();
- return TopDocs.merge(sort, topN, topFieldDocs);
- }
- };
- ConcurrentDrillSidewaysResult<TopFieldDocs> r = searchConcurrently(query, collectorManager);
- TopFieldDocs topDocs = r.collectorResult;
- if (doDocScores) {
- TopFieldCollector.populateScores(topDocs.scoreDocs, searcher, query);
- }
- return new DrillSidewaysResult(
- r.facets,
- topDocs,
- r.drillDownFacetsCollector,
- r.drillSidewaysFacetsCollector,
- r.drillSidewaysDims);
+ final CollectorManager<TopFieldCollector, TopFieldDocs> collectorManager =
+ new CollectorManager<>() {
+
+ @Override
+ public TopFieldCollector newCollector() {
+ return TopFieldCollector.create(sort, fTopN, after, Integer.MAX_VALUE);
+ }
+
+ @Override
+ public TopFieldDocs reduce(Collection<TopFieldCollector> collectors) {
+ final TopFieldDocs[] topFieldDocs = new TopFieldDocs[collectors.size()];
+ int pos = 0;
+ for (TopFieldCollector collector : collectors)
+ topFieldDocs[pos++] = collector.topDocs();
+ return TopDocs.merge(sort, topN, topFieldDocs);
+ }
+ };
+ final ConcurrentDrillSidewaysResult<TopFieldDocs> r;
+ if (executor != null) {
Review comment:
What about just doing `final ConcurrentDrillSidewaysResult<TopDocs> r = search(query, collectorManager);`? It handles delegating to sequential/concurrent based on the executor already.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -351,45 +344,36 @@ public DrillSidewaysResult search(ScoreDoc after, DrillDownQuery query, int topN
}
final int fTopN = Math.min(topN, limit);
- if (executor != null) { // We have an executor, let use the multi-threaded version
+ final CollectorManager<TopScoreDocCollector, TopDocs> collectorManager =
+ new CollectorManager<>() {
- final CollectorManager<TopScoreDocCollector, TopDocs> collectorManager =
- new CollectorManager<>() {
+ @Override
+ public TopScoreDocCollector newCollector() {
+ return TopScoreDocCollector.create(fTopN, after, Integer.MAX_VALUE);
+ }
- @Override
- public TopScoreDocCollector newCollector() {
- return TopScoreDocCollector.create(fTopN, after, Integer.MAX_VALUE);
- }
-
- @Override
- public TopDocs reduce(Collection<TopScoreDocCollector> collectors) {
- final TopDocs[] topDocs = new TopDocs[collectors.size()];
- int pos = 0;
- for (TopScoreDocCollector collector : collectors)
- topDocs[pos++] = collector.topDocs();
- return TopDocs.merge(topN, topDocs);
- }
- };
- ConcurrentDrillSidewaysResult<TopDocs> r = searchConcurrently(query, collectorManager);
- return new DrillSidewaysResult(
- r.facets,
- r.collectorResult,
- r.drillDownFacetsCollector,
- r.drillSidewaysFacetsCollector,
- r.drillSidewaysDims);
+ @Override
+ public TopDocs reduce(Collection<TopScoreDocCollector> collectors) {
+ final TopDocs[] topDocs = new TopDocs[collectors.size()];
+ int pos = 0;
+ for (TopScoreDocCollector collector : collectors) topDocs[pos++] = collector.topDocs();
+ return TopDocs.merge(topN, topDocs);
+ }
+ };
+ final ConcurrentDrillSidewaysResult<TopDocs> r;
+ if (executor != null) {
Review comment:
What about just doing ` final ConcurrentDrillSidewaysResult<TopDocs> r = search(query, collectorManager);`? It handles delegating to sequential/concurrent based on the executor already.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org