rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015454539
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -601,31 +602,71 @@ private void setFilters(ResponseBuilder rb, Elevation
elevation) {
List<Query> updatedFilters = new ArrayList<Query>();
for (Query filter : filters) {
- if (!excludeSet.contains(filter) || filter instanceof PostFilter) {
+
+ // filters that weren't tagged for exclusion are left unchanged
+ if (!excludeSet.contains(filter)) {
updatedFilters.add(filter);
continue;
}
- // we're looking at a filter that has been tagged for exclusion
- // transform it into a boolean OR of the original filter with the
"include query" matching the
+ // if a collapse filter was tagged for exclusion, throw an Exception;
+ // the desired semantics of tagging a collapse filter this way is
unclear;
+ // furthermore, CollapsingPostFilter is a special case that
+ // cannot be included as a clause in a BooleanQuery;
+ // its createWeight() method would throw an
UnsupportedOperationException when called by the
+ // BooleanQuery's own createWeight()
+ if (filter instanceof CollapsingQParserPlugin.CollapsingPostFilter) {
+ throw new SolrException(
+ SolrException.ErrorCode.BAD_REQUEST, "collapse filter cannot be
tagged for exclusion");
+ }
+
+ // we're looking at a filter that has been tagged for exclusion;
+ // first, figure out whether it avoids the cache;
+ // if the original filter had the Local Param "cache" and/or "cost", it
will be an
+ // ExtendedQuery; unless it specifically had cache=false, it's cacheable
+ boolean avoidCache =
+ (filter instanceof ExtendedQuery) && !((ExtendedQuery)
filter).getCache();
+
+ // transform the filter into a boolean OR of the original filter with
the "include query"
+ // matching the
// elevated docs
BooleanQuery.Builder queryBuilder = new BooleanQuery.Builder();
- queryBuilder.add(filter, BooleanClause.Occur.SHOULD);
+ if (avoidCache || filter instanceof FilterQuery) {
+ // if the original filter avoids the cache, we add it to the
BooleanQuery as-is;
+ // we do the same if the filter is _already_ a FilterQuery -- there's
no need to wrap it in
+ // another;
+ // note that FilterQuery.getCache() returns false, so in this
scenario, avoidCache will
+ // be true and the instanceof check is not necessary; however, it is
left in place for
+ // clarity and as a
+ // failsafe in case the behavior of FilterQuery.getCache() should ever
change
+ queryBuilder.add(filter, BooleanClause.Occur.SHOULD);
+ } else {
+ // the original filter is cacheable and not already a FilterQuery;
+ // wrap it in a FilterQuery so that it always consults the
+ // filter cache even though it will be represented as a clause within
a larger non-caching
+ // BooleanQuery
+ queryBuilder.add(new FilterQuery(filter), BooleanClause.Occur.SHOULD);
+ }
queryBuilder.add(elevation.includeQuery, BooleanClause.Occur.SHOULD);
BooleanQuery updatedFilter = queryBuilder.build();
- if (!(filter instanceof ExtendedQuery)) {
- updatedFilters.add(updatedFilter);
- } else {
- // if the original filter had the Local Param "cache" and/or "cost",
it will be an
- // ExtendedQuery;
- // in this case, the updated filter should be wrapped with the same
cache and cost settings
- ExtendedQuery eq = (ExtendedQuery) filter;
- WrappedQuery wrappedUpdatedFilter = new WrappedQuery(updatedFilter);
- wrappedUpdatedFilter.setCache(eq.getCache());
- wrappedUpdatedFilter.setCost(eq.getCost());
- updatedFilters.add(wrappedUpdatedFilter);
+ // we don't want to cache the BooleanQuery that we've built from the
original filter and the
+ // elevated doc ids;
+ // the first clause of the BooleanQuery will be a FilterQuery if the
original filter was
+ // cacheable, and
+ // FilterQueries always consult the cache; the second clause is a set of
doc ids that should
+ // be fast on its own
+ WrappedQuery wrappedUpdatedFilter = new WrappedQuery(updatedFilter);
+ wrappedUpdatedFilter.setCache(false);
+
+ // if the original filter is an ExtendedQuery, it has a cost; copy that
cost to the outer
+ // WrappedQuery
+ // TODO: is this necessary?
+ if (filter instanceof ExtendedQuery) {
+ wrappedUpdatedFilter.setCost(((ExtendedQuery) filter).getCost());
Review Comment:
My thinking was: if a user configures a filter as non-cached and specifies a
cost, presumably because they want to influence the filter's execution order
w.r.t. the other filters, we'd like the modified filter to have that same cost
so it would execute in the desired order. But perhaps it's sufficient that the
original filter, which is now nested inside the modified filter, still has the
original cost? What happens if WrappedQuery with cost 0 contains BooleanQuery
which contains a clause that's a WrappedQuery with cost 200, for example?
Assuming everything works out as user would hope in that scenario, I'll be glad
to be able to simply the code and get rid of these lines... What are your
thoughts @dsmiley ? Would you confirm that I should I go ahead and remove?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
