gsmiller commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r635152354
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,15 +174,32 @@ private BooleanQuery getBooleanQuery() {
return bq.build();
}
- Query getBaseQuery() {
+ /**
+ * Returns the internal baseQuery of the DrillDownQuery
+ *
+ * @return The baseQuery used on initialization of DrillDownQuery
+ */
+ public Query getBaseQuery() {
return baseQuery;
}
- Query[] getDrillDownQueries() {
+ /**
+ * Returns the dimension queries added either via {@link #add(String,
Query)} or {@link
+ * #add(String, String...)}
+ *
+ * @return The array of dimQueries
+ */
+ public Query[] getDrillDownQueries() {
+ if (isDimQueriesDirty == false) {
+ // returns previously built dimQueries
+ return builtDimQueries;
+ }
Query[] dimQueries = new Query[this.dimQueries.size()];
Review comment:
You shouldn't necessarily need to allocate a new array here right? If
you've previously built the queries (i.e., it's non-null), you should be able
to use `ArrayUtil` to grow the array (if necessary) and then repopulate it
directly. I suppose it depends a little bit on whether-or-not we want to
provide the caller any guarantees around whether-or-not the contents of the
array we return to them can change out from underneath them. But in this case,
I don't think that matters too much (but I would document it in the javadoc).
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,11 +170,22 @@ private BooleanQuery getBooleanQuery() {
return bq.build();
}
- Query getBaseQuery() {
+ /**
+ * Returns the internal baseQuery of the DrillDownQuery
+ *
+ * @return The baseQuery used on initialization of DrillDownQuery
+ */
+ public Query getBaseQuery() {
return baseQuery;
}
- Query[] getDrillDownQueries() {
+ /**
+ * Returns the dimension queries added either via {@link #add(String,
Query)} or {@link
+ * #add(String, String...)}
+ *
+ * @return The array of dimQueries
+ */
+ public Query[] getDrillDownQueries() {
Query[] dimQueries = new Query[this.dimQueries.size()];
Review comment:
Thanks for giving this optimization a shot @gautamworah96. Generally
looks great! Left a couple comments on the approach.
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -53,6 +53,8 @@ public static Term term(String field, String dim, String...
path) {
private final Query baseQuery;
private final List<BooleanQuery.Builder> dimQueries = new ArrayList<>();
private final Map<String, Integer> drillDownDims = new LinkedHashMap<>();
+ private boolean isDimQueriesDirty = true;
Review comment:
What about tracking the need to rebuild per dimension? If there's a case
where a user adds a large number of drill downs but keeps only modifying a
single one (e.g., adding additional terms) in-between calls to `getDrillDowns`
we could do a fair amount of wasteful rebuilding. You could keep a
`List<Boolean>` to track the status of each dimension and then only rebuild
those that have changed.
--
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
