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/05/13 22:16:18 UTC
[GitHub] [lucene] gautamworah96 opened a new pull request #138: Lucene 9956 Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
gautamworah96 opened a new pull request #138:
URL: https://github.com/apache/lucene/pull/138
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene:
* https://issues.apache.org/jira/projects/LUCENE
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
LUCENE must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# Description
It would be great if users could access the baseQuery of a DrillDownQuery. I think this can be useful for folks who want to access/test the clauses of a BooleanQuery (for example) after they've already wrapped it into a DrillDownQuery.
Currently the Query getBaseQuery() method is package private by default.
Based on comments from Greg Miller in the JIRA ticket, I decided to expose getDrillDownQueries as well
# Solution
Change the method signature to public
# Tests
Not added
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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
[GitHub] [lucene] gsmiller merged pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller merged pull request #138:
URL: https://github.com/apache/lucene/pull/138
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r639247815
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,15 +184,43 @@ 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 (isAnyDimQueryDirty == false) {
+ // returns previously built dimQueries
+ Query[] builtDimQueriesCopy = new Query[builtDimQueries.size()];
+ return builtDimQueries.toArray(builtDimQueriesCopy);
+ }
Query[] dimQueries = new Query[this.dimQueries.size()];
for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ if (isDimQueryDirty.get(i) == true) {
Review comment:
What if you alternatively kept a list of query indexes that are "dirty" and then only iterate through that list, rebuilding each one. If that list is empty, you know nothing has been updated. This seems more efficient than tracking a global boolean and a boolean for each dimension.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r633758104
##########
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:
Got it. That makes sense @gautamworah96. In that case, what do you think of caching the built queries and keeping track if they've been modified since last being built? So `getDrillDownQueries` would first check a "dirty" status and build the queries only if the boolean clauses have been modified since last being built (or if they've never been built). This way repeated calls to `getDrillDownQueries` wouldn't unnecessarily rebuild them. This isn't important currently since the method is package-private and its use is tightly coupled with `DrillSideways`, but if this is being made public, callers could unknowingly be doing wasted work by calling this thing multiple times.
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r632800907
##########
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:
I wonder if it would make more sense to build each dim query when they're added and change dimQueries to `List<BooleanQuery>`. From a quick glance at the code, I don't see a need to store these as boolean clauses and then only build them when needed. If we expose this as public, we could wind up building these queries multiple times, which is wasteful.
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r641822305
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,15 +184,43 @@ 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 (isAnyDimQueryDirty == false) {
+ // returns previously built dimQueries
+ Query[] builtDimQueriesCopy = new Query[builtDimQueries.size()];
+ return builtDimQueries.toArray(builtDimQueriesCopy);
+ }
Query[] dimQueries = new Query[this.dimQueries.size()];
for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ if (isDimQueryDirty.get(i) == true) {
Review comment:
Hmmm. That makes sense. I've implemented a `Set<int indexes of dims that need to be rebuilt> ` approach in the next commit
--
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
[GitHub] [lucene] gsmiller commented on pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller commented on pull request #138:
URL: https://github.com/apache/lucene/pull/138#issuecomment-852283912
Looks great. Thanks @gautamworah96!
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r633155578
##########
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:
So this ties back into why we use a `BooleanQuery.Builder` and not a `BooleanQuery` for storing `dimQueries`. IMO the reason behind this is so that you can call `public void add(String dim, Query subQuery)` multiple times with the same `dim` and keep on adding `subQueries`. This is possible because you are just adding `subQueries` with `Occur.SHOULD` clauses to the `Builder`. However, this is not possible with an already built `BooleanQuery`
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r642711762
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * 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 (dirtyDimQueryIndex.isEmpty()) {
+ // returns previously built dimQueries
+ Query[] builtDimQueriesCopy = new Query[builtDimQueries.size()];
+ return builtDimQueries.toArray(builtDimQueriesCopy);
+ }
+ for (int i = 0; i < this.dimQueries.size(); ++i) {
+ if (dirtyDimQueryIndex.contains(i)) {
+ builtDimQueries.set(i, this.dimQueries.get(i).build());
+ dirtyDimQueryIndex.remove(i);
+ }
}
- return dimQueries;
+ assert dirtyDimQueryIndex.isEmpty();
+ // by this time builtDimQueries has all the built queries and dirtyDimQueryIndex is empty
+ return getDrillDownQueries();
Review comment:
Added. Looks cleaner
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r637197151
##########
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:
In general I could not imagine a real world case where a user would want to add drill down subqueries in between calls to getDrillDownQueries. Usually, we would expect them to add subqueries in one go as soon as they are done with processing some external request. This also introduces some complexity in a few places.
The next commit has implemented the tracking *dirty status* per subquery approach
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r633155578
##########
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:
So this ties back into why we use a `BooleanQuery.Builder` and not a `BooleanQuery` for storing `dimQueries`. IMO the reason behind this is so that you can call `public void add(String dim, Query subQuery)` multiple times with the same `dim` and keep on adding `subQueries`. This is possible because you are just adding `subQueries` with `Occur.SHOULD` clauses to the `Builder`. However, this is not possible with an already built `BooleanQuery`
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r637197151
##########
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:
In general I could not imagine a real world case where a user would want to add drill down subqueries in between calls to getDrillDownQueries. Usually, we would expect them to add subqueries in one go as soon as they are done with processing some external request. This also introduces some complexity in a few places. But it is still worth trying out :)
The next commit has implemented the tracking *dirty status* per subquery approach
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gsmiller commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r641848546
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * 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 (dirtyDimQueryIndex.isEmpty()) {
+ // returns previously built dimQueries
+ Query[] builtDimQueriesCopy = new Query[builtDimQueries.size()];
+ return builtDimQueries.toArray(builtDimQueriesCopy);
+ }
+ for (int i = 0; i < this.dimQueries.size(); ++i) {
+ if (dirtyDimQueryIndex.contains(i)) {
+ builtDimQueries.set(i, this.dimQueries.get(i).build());
+ dirtyDimQueryIndex.remove(i);
+ }
}
- return dimQueries;
+ assert dirtyDimQueryIndex.isEmpty();
+ // by this time builtDimQueries has all the built queries and dirtyDimQueryIndex is empty
+ return getDrillDownQueries();
Review comment:
What about something like:
```
public Query[] getDrillDownQueries() {
for (Integer dirtyIndex : dirtyDimQueryIndex) {
builtDimQueries.set(dirtyIndex, dimQueries.get(dirtyIndex).build());
}
dirtyDimQueryIndex.clear();
return builtDimQueries.toArray(new Query[0]);
}
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * Returns the dimension queries added either via {@link #add(String, Query)} or {@link
+ * #add(String, String...)}
+ *
+ * @return The array of dimQueries
+ */
+ public Query[] getDrillDownQueries() {
Review comment:
Let's take advantage of the built query caching in `getBooleanQuery` as well! (Sorry for missing this earlier). We should be able to replace the explicit building of the drill down queries with something like:
```
for (Query query : getDrillDownQueries()) {
bq.add(query, Occur.FILTER);
}
```
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -53,6 +55,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 List<Query> builtDimQueries = new ArrayList<>();
+ private Set<Integer> dirtyDimQueryIndex = new HashSet<>();
Review comment:
Can you make these final please?
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r642711762
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * 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 (dirtyDimQueryIndex.isEmpty()) {
+ // returns previously built dimQueries
+ Query[] builtDimQueriesCopy = new Query[builtDimQueries.size()];
+ return builtDimQueries.toArray(builtDimQueriesCopy);
+ }
+ for (int i = 0; i < this.dimQueries.size(); ++i) {
+ if (dirtyDimQueryIndex.contains(i)) {
+ builtDimQueries.set(i, this.dimQueries.get(i).build());
+ dirtyDimQueryIndex.remove(i);
+ }
}
- return dimQueries;
+ assert dirtyDimQueryIndex.isEmpty();
+ // by this time builtDimQueries has all the built queries and dirtyDimQueryIndex is empty
+ return getDrillDownQueries();
Review comment:
Added
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * Returns the dimension queries added either via {@link #add(String, Query)} or {@link
+ * #add(String, String...)}
+ *
+ * @return The array of dimQueries
+ */
+ public Query[] getDrillDownQueries() {
Review comment:
Added
--
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
[GitHub] [lucene] gautamworah96 commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
gautamworah96 commented on a change in pull request #138:
URL: https://github.com/apache/lucene/pull/138#discussion_r642711788
##########
File path: lucene/facet/src/java/org/apache/lucene/facet/DrillDownQuery.java
##########
@@ -170,16 +185,36 @@ 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() {
- Query[] dimQueries = new Query[this.dimQueries.size()];
- for (int i = 0; i < dimQueries.length; ++i) {
- dimQueries[i] = this.dimQueries.get(i).build();
+ /**
+ * Returns the dimension queries added either via {@link #add(String, Query)} or {@link
+ * #add(String, String...)}
+ *
+ * @return The array of dimQueries
+ */
+ public Query[] getDrillDownQueries() {
Review comment:
Added. Another use of this nice optimization!
--
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
[GitHub] [lucene] gsmiller commented on a change in pull request #138: LUCENE-9956: Make getBaseQuery, getDrillDownQueries API from DrillDownQuery public
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org