From a98cd1dce5beebd05d3ad26ca23675bfafa9d023 Mon Sep 17 00:00:00 2001 From: Greg Miller Date: Fri, 6 Sep 2024 09:02:17 -0700 Subject: [PATCH 1/5] Remove usage of IndexSearcher#Search(Query, Collector) from monitor package --- .../apache/lucene/monitor/CollectingMatcher.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java index b6300dafa755..1d4ff0fb21fb 100644 --- a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java +++ b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java @@ -18,7 +18,9 @@ package org.apache.lucene.monitor; import java.io.IOException; +import java.util.Collection; import java.util.Map; +import org.apache.lucene.search.CollectorManager; import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.Query; import org.apache.lucene.search.Scorable; @@ -37,7 +39,19 @@ abstract class CollectingMatcher extends CandidateMatcher< @Override public void matchQuery(final String queryId, Query matchQuery, Map metadata) throws IOException { - searcher.search(matchQuery, new MatchCollector(queryId, scoreMode)); + CollectorManager collectorManager = + new CollectorManager<>() { + @Override + public MatchCollector newCollector() { + return new MatchCollector(queryId, scoreMode); + } + + @Override + public T reduce(Collection collectors) { + return null; // no-op since MatchCollector directly updates CandidateMatcher#matches + } + }; + searcher.search(matchQuery, collectorManager); } /** From 3d5fd893843c0b03d69a65c37ffe8468b3040908 Mon Sep 17 00:00:00 2001 From: Greg Miller Date: Mon, 9 Sep 2024 07:36:26 -0700 Subject: [PATCH 2/5] version with wrapping factory method in CM --- .../lucene/search/CollectorManager.java | 26 +++++++++++++++++++ .../lucene/monitor/CollectingMatcher.java | 15 +---------- 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java index d9969e03ed42..802c2d14f7c5 100644 --- a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java @@ -18,6 +18,8 @@ import java.io.IOException; import java.util.Collection; +import java.util.concurrent.Executor; +import org.apache.lucene.index.IndexReader; /** * A manager of collectors. This class is useful to parallelize execution of search requests and has @@ -46,4 +48,28 @@ public interface CollectorManager { * called after collection is finished on all provided collectors. */ T reduce(Collection collectors) throws IOException; + + /** + * Wrap a provided {@link Collector} with a thin {@code CollectorManager} wrapper for use with + * {@link IndexSearcher#search(Query, CollectorManager)}. The wrapping {@code CollectorManager} + * provides no {@link CollectorManager#reduce(Collection)} implementation, so the wrapped {@code + * Collector} needs to do all relevant work while collecting. + * + *

Note: This is only safe to use when {@code IndexSearcher} is created with no executor (see: + * {@link IndexSearcher#IndexSearcher(IndexReader, Executor)}), or the provided collector is + * threadsafe. + */ + static CollectorManager wrap(C in) { + return new CollectorManager() { + @Override + public C newCollector() { + return in; + } + + @Override + public Void reduce(Collection collectors) { + return null; + } + }; + } } diff --git a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java index 1d4ff0fb21fb..45194a580a2b 100644 --- a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java +++ b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java @@ -18,7 +18,6 @@ package org.apache.lucene.monitor; import java.io.IOException; -import java.util.Collection; import java.util.Map; import org.apache.lucene.search.CollectorManager; import org.apache.lucene.search.IndexSearcher; @@ -39,19 +38,7 @@ abstract class CollectingMatcher extends CandidateMatcher< @Override public void matchQuery(final String queryId, Query matchQuery, Map metadata) throws IOException { - CollectorManager collectorManager = - new CollectorManager<>() { - @Override - public MatchCollector newCollector() { - return new MatchCollector(queryId, scoreMode); - } - - @Override - public T reduce(Collection collectors) { - return null; // no-op since MatchCollector directly updates CandidateMatcher#matches - } - }; - searcher.search(matchQuery, collectorManager); + searcher.search(matchQuery, CollectorManager.wrap(new MatchCollector(queryId, scoreMode))); } /** From 46b96b9ce82039d158143140d0d98a2281fb4dcf Mon Sep 17 00:00:00 2001 From: Greg Miller Date: Mon, 9 Sep 2024 13:23:27 -0700 Subject: [PATCH 3/5] pr feedback --- .../lucene/search/CollectorManager.java | 20 +++++++++++++------ .../lucene/monitor/CollectingMatcher.java | 4 +++- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java index 802c2d14f7c5..af97044dbd58 100644 --- a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java @@ -51,23 +51,31 @@ public interface CollectorManager { /** * Wrap a provided {@link Collector} with a thin {@code CollectorManager} wrapper for use with - * {@link IndexSearcher#search(Query, CollectorManager)}. The wrapping {@code CollectorManager} - * provides no {@link CollectorManager#reduce(Collection)} implementation, so the wrapped {@code - * Collector} needs to do all relevant work while collecting. + * {@link IndexSearcher#search(Query, CollectorManager)} when doing single-threaded searching. The + * wrapping {@code CollectorManager} provides no {@link CollectorManager#reduce(Collection)} + * implementation, so the wrapped {@code Collector} needs to do all relevant work while + * collecting. * *

Note: This is only safe to use when {@code IndexSearcher} is created with no executor (see: - * {@link IndexSearcher#IndexSearcher(IndexReader, Executor)}), or the provided collector is - * threadsafe. + * {@link IndexSearcher#IndexSearcher(IndexReader, Executor)}). */ - static CollectorManager wrap(C in) { + static CollectorManager wrapForSingleThreadedSearch(C in) { return new CollectorManager() { + private boolean newCollectorInvoked; + @Override public C newCollector() { + if (newCollectorInvoked) { + throw new IllegalStateException( + "newCollector should be invoked at most once. Ensure your IndexSearcher has been created without an Executor."); + } + newCollectorInvoked = true; return in; } @Override public Void reduce(Collection collectors) { + assert collectors.size() == 1 : "collectors should contain exactly one collector instance"; return null; } }; diff --git a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java index 45194a580a2b..c64605e6661a 100644 --- a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java +++ b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java @@ -38,7 +38,9 @@ abstract class CollectingMatcher extends CandidateMatcher< @Override public void matchQuery(final String queryId, Query matchQuery, Map metadata) throws IOException { - searcher.search(matchQuery, CollectorManager.wrap(new MatchCollector(queryId, scoreMode))); + searcher.search( + matchQuery, + CollectorManager.wrapForSingleThreadedSearch(new MatchCollector(queryId, scoreMode))); } /** From e79cc89bad5d013c1a09958878224105e74974ec Mon Sep 17 00:00:00 2001 From: Greg Miller Date: Tue, 10 Sep 2024 07:14:13 -0700 Subject: [PATCH 4/5] renaming --- .../src/java/org/apache/lucene/search/CollectorManager.java | 2 +- .../src/java/org/apache/lucene/monitor/CollectingMatcher.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java index af97044dbd58..e3df418aab5d 100644 --- a/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java +++ b/lucene/core/src/java/org/apache/lucene/search/CollectorManager.java @@ -59,7 +59,7 @@ public interface CollectorManager { *

Note: This is only safe to use when {@code IndexSearcher} is created with no executor (see: * {@link IndexSearcher#IndexSearcher(IndexReader, Executor)}). */ - static CollectorManager wrapForSingleThreadedSearch(C in) { + static CollectorManager forSequentialExecution(C in) { return new CollectorManager() { private boolean newCollectorInvoked; diff --git a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java index c64605e6661a..6175fec5ca33 100644 --- a/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java +++ b/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java @@ -40,7 +40,7 @@ public void matchQuery(final String queryId, Query matchQuery, Map Date: Tue, 10 Sep 2024 13:10:44 -0700 Subject: [PATCH 5/5] changes entry --- lucene/CHANGES.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 42ab9c6fc635..9f339cf383d2 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -293,6 +293,9 @@ API Changes * GITHUB#13568: Add DrillSideways#search method that supports any CollectorManagers for drill-sideways dimensions or drill-down. (Egor Potemkin) +* GITHUB#13735: Add CollectorManager#forSequentialExecution to make CollectorManager creation more convenient + for users of the deprecated IndexSearcher#search(Query, Collector). (Greg Miller) + New Features --------------------- @@ -328,6 +331,9 @@ Improvements * GITHUB#13201: Better cost estimation on MultiTermQuery over few terms. (Michael Froh) +* GITHUB#13735: Migrate monitor package usage of deprecated IndexSearcher#search(Query, Collector) + to IndexSearcher#search(Query, CollectorManager). (Greg Miller) + Optimizations ---------------------