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 2021/06/07 21:36:06 UTC
[GitHub] [lucene] gsmiller commented on a change in pull request #159: LUCENE-9945: extend DrillSideways to expose FacetCollector and drill-down dimensions
gsmiller commented on a change in pull request #159:
URL: https://github.com/apache/lucene/pull/159#discussion_r646946460
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
Review comment:
I'm finding this documentation a little difficult to parse when I read it. Maybe something like:
```
/**
* Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}.
* The {@link FacetsCollector}s for the drill down and drill sideways dimensions are also exposed
* for advanced use-cases that need access to them as an alternative to accessing the
* {@code Facets}.
*/
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -277,7 +278,10 @@ public ScoreMode scoreMode() {
drillDownCollector,
drillSidewaysCollectors,
drillDownDims.keySet().toArray(new String[0])),
- null);
+ null,
+ drillDownCollector,
+ drillSidewaysCollectors,
+ drillDownDims.keySet().toArray(new String[0]));
Review comment:
It would be nice to store the result of `drillDownDims.keySet().toArray(new String[0]))` locally and then reference it in these two places instead of calling this `toArray` method twice.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
+ * FacetsCollector} for drill down and drill sideways passed to {@link #buildFacetsResult} to
+ * compute {@link Facets} are also exposed here as an alternative to subclassing DrillSideways to
+ * obtain them
+ */
public static class DrillSidewaysResult {
/** Combined drill down and sideways results. */
public final Facets facets;
/** Hits. */
public final TopDocs hits;
+ /** FacetsCollector for drill down. */
+ public final FacetsCollector drillDowns;
+
+ /**
+ * Facets Collector for drill sideways Array of FacetsCollector maps directly to Array of
+ * String[] drillSidewaysDims i.e. ith drillSidewaysDims String maps to ith drillSideways
+ * FacetsCollector
+ */
+ public final FacetsCollector[] drillSideways;
Review comment:
I'd propose a more descriptive name, like `drillSidewaysFacetsCollectors`
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -545,7 +584,10 @@ private DrillDownQuery getDrillDownQuery(
drillSidewaysCollectors,
drillDownDims.keySet().toArray(new String[0])),
null,
- collectorResult);
+ collectorResult,
+ drillDownCollector,
+ drillSidewaysCollectors,
+ drillDownDims.keySet().toArray(new String[0]));
Review comment:
As with above, there's an opportunity to store the result of the `toArray()` call locally instead of calling it twice.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
+ * FacetsCollector} for drill down and drill sideways passed to {@link #buildFacetsResult} to
+ * compute {@link Facets} are also exposed here as an alternative to subclassing DrillSideways to
+ * obtain them
+ */
public static class DrillSidewaysResult {
/** Combined drill down and sideways results. */
public final Facets facets;
/** Hits. */
public final TopDocs hits;
+ /** FacetsCollector for drill down. */
+ public final FacetsCollector drillDowns;
+
+ /**
+ * Facets Collector for drill sideways Array of FacetsCollector maps directly to Array of
+ * String[] drillSidewaysDims i.e. ith drillSidewaysDims String maps to ith drillSideways
+ * FacetsCollector
+ */
Review comment:
I think some users might have difficulty understanding this documentation. Maybe something like:
```suggestion
/**
* FacetsCollectors populated for each drill sideways dimension. Each collector exposes
* the hits that match on all DrillDownQuery dimensions, but treating their corresponding
* sideways dimension as optional. This array provides a FacetsCollector for each drill
* down dimension present in the original DrillDownQuery, and the associated dimension
* for each FacetsCollector can be determined using the parallel `drillSidewaysDims`
* array. Useful for advanced use-cases that want to compute Facets results separate
* from the provided Facets in this result.
*/
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
+ * FacetsCollector} for drill down and drill sideways passed to {@link #buildFacetsResult} to
+ * compute {@link Facets} are also exposed here as an alternative to subclassing DrillSideways to
+ * obtain them
+ */
public static class DrillSidewaysResult {
/** Combined drill down and sideways results. */
public final Facets facets;
/** Hits. */
public final TopDocs hits;
+ /** FacetsCollector for drill down. */
+ public final FacetsCollector drillDowns;
+
+ /**
+ * Facets Collector for drill sideways Array of FacetsCollector maps directly to Array of
+ * String[] drillSidewaysDims i.e. ith drillSidewaysDims String maps to ith drillSideways
+ * FacetsCollector
+ */
+ public final FacetsCollector[] drillSideways;
+
+ /** drill sideways dimensions */
Review comment:
This could also be a little more descriptive. Maybe something like:
```suggestion
/** Dimensions that correspond to to the `drillSideways` FacetsCollectors */
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
+ * FacetsCollector} for drill down and drill sideways passed to {@link #buildFacetsResult} to
+ * compute {@link Facets} are also exposed here as an alternative to subclassing DrillSideways to
+ * obtain them
+ */
public static class DrillSidewaysResult {
/** Combined drill down and sideways results. */
public final Facets facets;
/** Hits. */
public final TopDocs hits;
+ /** FacetsCollector for drill down. */
+ public final FacetsCollector drillDowns;
Review comment:
I'd suggest a more descriptive name. Maybe something like `drillDownFacetsCollector`
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -612,7 +654,10 @@ private DrillDownQuery getDrillDownQuery(
buildFacetsResult(
mainFacetsCollector, facetsCollectors, drillDownDims.keySet().toArray(new String[0])),
null,
- collectorResult);
+ collectorResult,
+ mainFacetsCollector,
+ facetsCollectors,
+ drillDownDims.keySet().toArray(new String[0]));
Review comment:
Another opportunity to reuse the results of `toArray` instead of calling twice.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillSideways.java
##########
@@ -390,18 +398,44 @@ protected boolean scoreSubDocsAtOnce() {
return false;
}
- /** Result of a drill sideways search, including the {@link Facets} and {@link TopDocs}. */
+ /**
+ * Result of a drill sideways search, including the {@link Facets} and {@link TopDocs} {@link
+ * FacetsCollector} for drill down and drill sideways passed to {@link #buildFacetsResult} to
+ * compute {@link Facets} are also exposed here as an alternative to subclassing DrillSideways to
+ * obtain them
+ */
public static class DrillSidewaysResult {
/** Combined drill down and sideways results. */
public final Facets facets;
/** Hits. */
public final TopDocs hits;
+ /** FacetsCollector for drill down. */
Review comment:
I might make this documentation a little more verbose since this is a tricky domain and really only exposed for less-traditional use-cases. Maybe something like:
```suggestion
/**
* FacetsCollector populated based on hits that match the full DrillDownQuery,
* treating all drill down dimensions as required clauses. Useful for advanced
* use-cases that want to compute Facets results separate from the provided
* Facets in this result.
*/
```
--
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.
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