gsmiller commented on code in PR #13702:
URL: https://github.com/apache/lucene/pull/13702#discussion_r1739438582
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysQuery.java:
##########
@@ -58,15 +61,33 @@ class DrillSidewaysQuery extends Query {
*/
DrillSidewaysQuery(
Query baseQuery,
- CollectorOwner<?, ?> drillDownCollectorOwner,
- List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners,
+ List<CollectorManager<K, R>> drillSidewaysCollectorManagers,
+ Query[] drillDownQueries,
+ boolean scoreSubDocsAtOnce) {
+ this(
+ baseQuery,
+ drillSidewaysCollectorManagers,
+ Collections.synchronizedList(new ArrayList<>()),
+ drillDownQueries,
+ scoreSubDocsAtOnce);
+ }
+
+ /**
+ * Needed for {@link Query#rewrite(IndexSearcher)}. Ensures the same
"managed" lists get used
+ * since {@link DrillSideways} accesses references to these through the
original {@code
+ * DrillSidewaysQuery}.
+ */
+ private DrillSidewaysQuery(
+ Query baseQuery,
+ List<CollectorManager<K, R>> drillSidewaysCollectorManagers,
+ final List<List<K>> managedDrillSidewaysCollectors,
Review Comment:
nit: let's remove `final` to be consistent?
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java:
##########
@@ -480,59 +462,61 @@ private void searchSequentially(
}
Query[] drillDownQueries = query.getDrillDownQueries();
- DrillSidewaysQuery dsq =
- new DrillSidewaysQuery(
- baseQuery,
- // drillDownCollectorOwner,
- // Don't pass drill down collector because drill down is collected
by IndexSearcher
- // itself.
- // TODO: deprecate drillDown collection in DrillSidewaysQuery?
- null,
- drillSidewaysCollectorOwners,
- drillDownQueries,
- scoreSubDocsAtOnce());
-
- searcher.search(dsq, drillDownCollectorOwner);
- // This method doesn't return results as each dimension might have its own
result type.
- // But we call getResult to trigger results reducing, so that users don't
have to worry about
- // it.
- drillDownCollectorOwner.getResult();
- if (drillSidewaysCollectorOwners != null) {
- for (CollectorOwner<?, ?> sidewaysOwner : drillSidewaysCollectorOwners) {
- sidewaysOwner.getResult();
+ DrillSidewaysQuery<K, R> dsq =
+ new DrillSidewaysQuery<>(
+ baseQuery, drillSidewaysCollectorManagers, drillDownQueries,
scoreSubDocsAtOnce());
+
+ T collectorResult = searcher.search(dsq, drillDownCollectorManager);
+ List<R> drillSidewaysResults = new ArrayList<>(drillDownDims.size());
+ assert drillSidewaysCollectorManagers != null
+ : "Case without drill sideways dimensions is handled above";
+ int numSlices = dsq.managedDrillSidewaysCollectors.size();
+ for (int dim = 0; dim < drillDownDims.size(); dim++) {
+ List<K> collectorsForDim = new ArrayList<>(numSlices);
+ for (int slice = 0; slice < numSlices; slice++) {
+
collectorsForDim.add(dsq.managedDrillSidewaysCollectors.get(slice).get(dim));
}
+ drillSidewaysResults.add(
+ dim,
drillSidewaysCollectorManagers.get(dim).reduce(collectorsForDim));
}
+ return new Result<>(collectorResult, drillSidewaysResults);
}
- private void searchConcurrently(
+ private <C extends Collector, T, K extends Collector, R> Result<T, R>
searchConcurrently(
final DrillDownQuery query,
- final CollectorOwner<?, ?> drillDownCollectorOwner,
- final List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners)
+ final CollectorManager<C, T> drillDownCollectorManager,
+ final List<CollectorManager<K, R>> drillSidewaysCollectorManagers)
throws IOException {
Review Comment:
nit: this can no longer throw `IOException` so you can pull it. It also
drives me a little nuts that we even have this concurrent implementation. It
seems so silly/wasteful to me to just fire off N different versions of the
query instead of taking the partial evaluation approach. Oh well. If we port
this functionality to `IndexSearcher` as I'd like, maybe this can die when we
do that (unless I'm missing some reason it's useful to do this?).
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysQuery.java:
##########
@@ -41,12 +42,14 @@
// TODO change the way DrillSidewaysScorer is used, this query does not work
// with filter caching
-class DrillSidewaysQuery extends Query {
+class DrillSidewaysQuery<K extends Collector, R> extends Query {
final Query baseQuery;
- final CollectorOwner<?, ?> drillDownCollectorOwner;
- final List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners;
+ // final CollectorManager<C, T> drillDownCollectorManager;
Review Comment:
Let's pull out the commented code in this class?
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysQuery.java:
##########
@@ -58,15 +61,33 @@ class DrillSidewaysQuery extends Query {
*/
DrillSidewaysQuery(
Query baseQuery,
- CollectorOwner<?, ?> drillDownCollectorOwner,
- List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners,
+ List<CollectorManager<K, R>> drillSidewaysCollectorManagers,
+ Query[] drillDownQueries,
+ boolean scoreSubDocsAtOnce) {
+ this(
Review Comment:
We used to have the following comment in here before the recent changes.
Could we bring it back since this was a bit of a sharp edge in the past?
```
// Note that the "managed" facet collector lists are synchronized here
since bulkScorer()
// can be invoked concurrently and needs to remain thread-safe. We're OK
with synchronizing
// on the whole list as contention is expected to remain very low:
```
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java:
##########
@@ -391,12 +377,11 @@ public <R> ConcurrentDrillSidewaysResult<R> search(
if (query.getDims().isEmpty() == false) {
drillSidewaysDims = query.getDims().keySet().toArray(new String[0]);
int numDims = query.getDims().size();
- assert drillSidewaysCollectorOwners != null;
- assert drillSidewaysCollectorOwners.size() == numDims;
+ assert drillSidewaysCollectorManagers != null;
+ assert drillSidewaysCollectorManagers.size() == numDims;
drillSidewaysCollectors = new FacetsCollector[numDims];
for (int dim = 0; dim < numDims; dim++) {
- drillSidewaysCollectors[dim] =
- (FacetsCollector)
drillSidewaysCollectorOwners.get(dim).getResult();
+ drillSidewaysCollectors[dim] = (FacetsCollector)
result.drillSidewaysResults.get(dim);
Review Comment:
note: if you take my suggestion back on line 353 you can remove the cast here
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java:
##########
@@ -344,31 +331,30 @@ public <R> ConcurrentDrillSidewaysResult<R> search(
// Main query
FacetsCollectorManager drillDownFacetsCollectorManager =
createDrillDownFacetsCollectorManager();
- final CollectorOwner<?, ?> mainCollectorOwner;
+ final CollectorManager<?, ?> mainCollectorManager;
if (drillDownFacetsCollectorManager != null) {
// Make sure we populate a facet collector corresponding to the base
query if desired:
- mainCollectorOwner =
- new CollectorOwner<>(
- new MultiCollectorManager(drillDownFacetsCollectorManager,
hitCollectorManager));
+ mainCollectorManager =
+ new MultiCollectorManager(drillDownFacetsCollectorManager,
hitCollectorManager);
} else {
- mainCollectorOwner = new CollectorOwner<>(hitCollectorManager);
+ mainCollectorManager = hitCollectorManager;
}
// Drill sideways dimensions
- final List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners;
+ final List<CollectorManager<FacetsCollector, FacetsCollector>>
drillSidewaysCollectorManagers;
if (query.getDims().isEmpty() == false) {
- drillSidewaysCollectorOwners = new ArrayList<>(query.getDims().size());
+ drillSidewaysCollectorManagers = new ArrayList<>(query.getDims().size());
for (int i = 0; i < query.getDims().size(); i++) {
- drillSidewaysCollectorOwners.add(
- new CollectorOwner<>(createDrillSidewaysFacetsCollectorManager()));
+
drillSidewaysCollectorManagers.add(createDrillSidewaysFacetsCollectorManager());
}
} else {
- drillSidewaysCollectorOwners = null;
+ drillSidewaysCollectorManagers = null;
}
// Execute query
+ final Result<?, ?> result;
Review Comment:
I think you can tighten this up to `<?, FacetsCollector>` right?
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java:
##########
@@ -414,62 +399,59 @@ public <R> ConcurrentDrillSidewaysResult<R> search(
/**
* Search using DrillDownQuery with custom collectors. This method can be
used with any {@link
- * CollectorOwner}s. It doesn't return anything because it is expected that
you read results from
- * provided {@link CollectorOwner}s.
- *
- * <p>To read the results, run {@link CollectorOwner#getResult()} for drill
down and all drill
- * sideways dimensions.
- *
- * <p>Note: use {@link Collections#unmodifiableList(List)} to wrap {@code
- * drillSidewaysCollectorOwners} to convince compiler that it is safe to use
List here.
- *
- * <p>Use {@link MultiCollectorManager} wrapped by {@link CollectorOwner} to
collect both hits and
- * facets for the entire query and/or for drill-sideways dimensions.
+ * CollectorManager}s.
*
- * <p>TODO: Class CollectorOwner was created so that we can ignore
CollectorManager type C,
- * because we want each dimensions to be able to use their own types.
Alternatively, we can use
- * typesafe heterogeneous container and provide CollectorManager type for
each dimension to this
- * method? I do like CollectorOwner approach as it seems more intuitive?
+ * <p>Note: Use {@link MultiCollectorManager} to collect both hits and
facets for the entire query
+ * and/or for drill-sideways dimensions. You can also use it to wrap
different types of {@link
+ * CollectorManager} for drill-sideways dimensions.
*/
- public void search(
+ public <C extends Collector, T, K extends Collector, R> Result<T, R> search(
final DrillDownQuery query,
Review Comment:
nit: strip off `final` here to be consistent?
##########
lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java:
##########
@@ -414,62 +399,59 @@ public <R> ConcurrentDrillSidewaysResult<R> search(
/**
* Search using DrillDownQuery with custom collectors. This method can be
used with any {@link
- * CollectorOwner}s. It doesn't return anything because it is expected that
you read results from
- * provided {@link CollectorOwner}s.
- *
- * <p>To read the results, run {@link CollectorOwner#getResult()} for drill
down and all drill
- * sideways dimensions.
- *
- * <p>Note: use {@link Collections#unmodifiableList(List)} to wrap {@code
- * drillSidewaysCollectorOwners} to convince compiler that it is safe to use
List here.
- *
- * <p>Use {@link MultiCollectorManager} wrapped by {@link CollectorOwner} to
collect both hits and
- * facets for the entire query and/or for drill-sideways dimensions.
+ * CollectorManager}s.
*
- * <p>TODO: Class CollectorOwner was created so that we can ignore
CollectorManager type C,
- * because we want each dimensions to be able to use their own types.
Alternatively, we can use
- * typesafe heterogeneous container and provide CollectorManager type for
each dimension to this
- * method? I do like CollectorOwner approach as it seems more intuitive?
+ * <p>Note: Use {@link MultiCollectorManager} to collect both hits and
facets for the entire query
+ * and/or for drill-sideways dimensions. You can also use it to wrap
different types of {@link
+ * CollectorManager} for drill-sideways dimensions.
*/
- public void search(
+ public <C extends Collector, T, K extends Collector, R> Result<T, R> search(
final DrillDownQuery query,
- CollectorOwner<?, ?> drillDownCollectorOwner,
- List<CollectorOwner<?, ?>> drillSidewaysCollectorOwners)
+ CollectorManager<C, T> drillDownCollectorManager,
+ List<CollectorManager<K, R>> drillSidewaysCollectorManagers)
throws IOException {
- if (drillDownCollectorOwner == null) {
+ if (drillDownCollectorManager == null) {
throw new IllegalArgumentException(
"This search method requires client to provide drill down collector
manager");
}
- if (drillSidewaysCollectorOwners == null) {
+ if (drillSidewaysCollectorManagers == null) {
Review Comment:
I wonder if we should deprecate the `createDrillDownFacetsCollectorManager`
and `createDrillSidewaysFacetsCollectorManager` hooks with this change? I think
this new functionality should make it unnecessary to use those semi-clumsy
override hooks? WDYT? Maybe I'm overlooking something important...
##########
lucene/facet/src/test/org/apache/lucene/facet/TestDrillSideways.java:
##########
@@ -284,7 +284,7 @@ public void testCollectionTerminated() throws Exception {
Weight dimWeight = searcher.createWeight(dimQ,
ScoreMode.COMPLETE_NO_SCORES, 1f);
Scorer dimScorer = dimWeight.scorer(ctx);
- FacetsCollector baseFC = new FacetsCollector();
+ // FacetsCollector baseFC = new FacetsCollector();
Review Comment:
Let's just remove? (same for the other places where you have commented-out
code in here)
--
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-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
