gsmiller commented on code in PR #13702:
URL: https://github.com/apache/lucene/pull/13702#discussion_r1742821648
##########
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:
Thanks! I personally think it might be a little aggressive to deprecate all
these methods, especially so close to the planned 10.0 release. I'm worried
we're overlooking legitimate needs/use-cases. Let's follow up with a general
`DrillSideways` cleanup in 10.0? Whether that's moving everything into
IndexSearcher or simplifying `DrillSideways`, I think we've got some options,
but I'd prefer to take our time a little more.
To this end, I reverted the latest commit that adds the deprecation on your
branch (thanks for keeping it separate!) and am merging/back-porting. If you
feel strongly that we should move for deprecation in 9.12 please let me know
and we can still consider it (but as a separate commit). Does that sound
reasonable?
--
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
