dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014458212
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -590,52 +592,79 @@ private void setFilters(ResponseBuilder rb, Elevation
elevation) {
return;
}
- Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
- if (tagMap == null) {
+ Set<Query> excludeSet = getTaggedQueries(rb, excludeTags);
+ if (excludeSet.isEmpty()) {
// no filters were tagged
return;
}
- // TODO: this code is copied from FacetProcessor#handleFilterExclusions()
- // duplication could be avoided by placing this code in a common utility
method, perhaps in
- // QueryUtils
- IdentityHashMap<Query, Boolean> excludeSet = new IdentityHashMap<>();
- for (String excludeTag : excludeTags) {
- Object olst = tagMap.get(excludeTag);
- // tagMap has entries of List<String,List<QParser>>, but subject to
change in the future
- if (!(olst instanceof Collection)) continue;
- for (Object o : (Collection<?>) olst) {
- if (!(o instanceof QParser)) continue;
- QParser qp = (QParser) o;
- try {
- excludeSet.put(qp.getQuery(), Boolean.TRUE);
- } catch (SyntaxError syntaxError) {
- // This should not happen since we should only be retrieving a
previously parsed query
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
syntaxError);
- }
- }
- }
-
List<Query> updatedFilters = new ArrayList<Query>();
- for (Query q : filters) {
- if (!excludeSet.containsKey(q) || q instanceof
CollapsingQParserPlugin.CollapsingPostFilter) {
- updatedFilters.add(q);
+ for (Query filter : filters) {
+ if (!excludeSet.contains(filter) || filter instanceof PostFilter) {
+ 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
// elevated docs
BooleanQuery.Builder queryBuilder = new BooleanQuery.Builder();
- queryBuilder.add(q, BooleanClause.Occur.SHOULD);
+ queryBuilder.add(filter, BooleanClause.Occur.SHOULD);
Review Comment:
As I think about this more... the original "filter" (a "fq" param) should
probably be wrapped in FilterQuery _if it's configured to cache_, this way
what's being cached is just this filter, and thus would be more likely to get a
good cache hit rate. We'd set the BooleanQuery combo to not cache; it'll be
fast -- one of its components is already cached and the other is a trivial set
of documents pre-selected -- fast. If multiple queries come in with the same
filter queries, they will use the same cache entries, even if only some hit
certain QEC rules.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -590,52 +592,79 @@ private void setFilters(ResponseBuilder rb, Elevation
elevation) {
return;
}
- Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
- if (tagMap == null) {
+ Set<Query> excludeSet = getTaggedQueries(rb, excludeTags);
+ if (excludeSet.isEmpty()) {
// no filters were tagged
return;
}
- // TODO: this code is copied from FacetProcessor#handleFilterExclusions()
- // duplication could be avoided by placing this code in a common utility
method, perhaps in
- // QueryUtils
- IdentityHashMap<Query, Boolean> excludeSet = new IdentityHashMap<>();
- for (String excludeTag : excludeTags) {
- Object olst = tagMap.get(excludeTag);
- // tagMap has entries of List<String,List<QParser>>, but subject to
change in the future
- if (!(olst instanceof Collection)) continue;
- for (Object o : (Collection<?>) olst) {
- if (!(o instanceof QParser)) continue;
- QParser qp = (QParser) o;
- try {
- excludeSet.put(qp.getQuery(), Boolean.TRUE);
- } catch (SyntaxError syntaxError) {
- // This should not happen since we should only be retrieving a
previously parsed query
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
syntaxError);
- }
- }
- }
-
List<Query> updatedFilters = new ArrayList<Query>();
- for (Query q : filters) {
- if (!excludeSet.containsKey(q) || q instanceof
CollapsingQParserPlugin.CollapsingPostFilter) {
- updatedFilters.add(q);
+ for (Query filter : filters) {
+ if (!excludeSet.contains(filter) || filter instanceof PostFilter) {
Review Comment:
Again on this PostFilter issue... assuming this is your logic and not simply
taking it from somewhere else... what situation caused you to add this?
Depending on how the PostFilter is implemented, it may or may not be possible
to have the QEC slip docs past it like it can others. In such a scenario, we
should probably give the user an error because we can't, instead of silently
ignoring that they asked to ignore this tagged filter but we actually won't.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -590,52 +592,79 @@ private void setFilters(ResponseBuilder rb, Elevation
elevation) {
return;
}
- Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
- if (tagMap == null) {
+ Set<Query> excludeSet = getTaggedQueries(rb, excludeTags);
+ if (excludeSet.isEmpty()) {
// no filters were tagged
return;
}
- // TODO: this code is copied from FacetProcessor#handleFilterExclusions()
- // duplication could be avoided by placing this code in a common utility
method, perhaps in
- // QueryUtils
- IdentityHashMap<Query, Boolean> excludeSet = new IdentityHashMap<>();
- for (String excludeTag : excludeTags) {
- Object olst = tagMap.get(excludeTag);
- // tagMap has entries of List<String,List<QParser>>, but subject to
change in the future
- if (!(olst instanceof Collection)) continue;
- for (Object o : (Collection<?>) olst) {
- if (!(o instanceof QParser)) continue;
- QParser qp = (QParser) o;
- try {
- excludeSet.put(qp.getQuery(), Boolean.TRUE);
- } catch (SyntaxError syntaxError) {
- // This should not happen since we should only be retrieving a
previously parsed query
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
syntaxError);
- }
- }
- }
-
List<Query> updatedFilters = new ArrayList<Query>();
- for (Query q : filters) {
- if (!excludeSet.containsKey(q) || q instanceof
CollapsingQParserPlugin.CollapsingPostFilter) {
- updatedFilters.add(q);
+ for (Query filter : filters) {
+ if (!excludeSet.contains(filter) || filter instanceof PostFilter) {
+ 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
// elevated docs
BooleanQuery.Builder queryBuilder = new BooleanQuery.Builder();
- queryBuilder.add(q, BooleanClause.Occur.SHOULD);
+ queryBuilder.add(filter, BooleanClause.Occur.SHOULD);
queryBuilder.add(elevation.includeQuery, BooleanClause.Occur.SHOULD);
- updatedFilters.add(queryBuilder.build());
+ 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());
Review Comment:
FYI @risdenk RE immutability of all Query classes. Maybe this slips past us
for now but it'll have to change.
##########
solr/solrj/src/java/org/apache/solr/common/params/QueryElevationParams.java:
##########
@@ -59,4 +59,11 @@ public interface QueryElevationParams {
* set this to true. False by default.
*/
String ELEVATE_ONLY_DOCS_MATCHING_QUERY = "elevateOnlyDocsMatchingQuery";
+
+ /**
+ * By default, the component respects the fq parameter. If you want to
elevate documents that do
+ * not match the provided filters, tag the filters in question via the local
parameter syntax
+ * fq={!tag=t1}field1:value1 and then specify the tags for exclusion via
elevate.excludeTag=t1
Review Comment:
```suggestion
* fq={!tag=t1}field1:value1 and then specify the tags for exclusion via
elevate.excludeTags=t1
```
##########
solr/solr-ref-guide/modules/query-guide/pages/query-elevation-component.adoc:
##########
@@ -234,5 +234,18 @@
http://localhost:8983/solr/techproducts/elevate?q=ipod&df=text&elevateIds=IW-02,
=== The fq Parameter with Elevation
-Query elevation respects the standard filter query (`fq`) parameter.
+By default, query elevation respects the standard filter query (`fq`)
parameter.
That is, if the query contains the `fq` parameter, all results will be within
that filter even if `elevate.xml` adds other documents to the result set.
+
+If you want elevated documents to be included in the result set whether or not
they match specific filter queries, you can tag those filter queries using
xref:local-params.adoc[LocalParams syntax] and then specify the tags for
exclusion via the `elevate.excludeTags` request parameter.
+Both the `tag` local param and the `elevate.excludeTags` request parameter may
specify multiple values by separating them with commas.
+
+[source,text]
+q=mainquery&fq=status:public&fq={!tag=dt}doctype:pdf&elevate.excludeTags=dt
+
+[source,text]
+q=mainquery&fq=status:public&fq={!tag=t1,t2}a:b&fq={!tag=t3}c:d&fq={!tag=t4}e:f&elevate.excludeTags=t1,t4
+
+When a filter is tagged for exclusion, it is not ignored completely; rather it
is modified so that the elevated documents can pass through.
+Documents that are not elevated are still subject to the filter.
+This feature may have a performance impact when filter caching is enabled and
the modified filters are not found in the cache.
Review Comment:
This last line will no longer be needed to say once we use `FilterQuery` as
I suggest.
##########
solr/solrj/src/java/org/apache/solr/common/params/QueryElevationParams.java:
##########
@@ -59,4 +59,11 @@ public interface QueryElevationParams {
* set this to true. False by default.
*/
String ELEVATE_ONLY_DOCS_MATCHING_QUERY = "elevateOnlyDocsMatchingQuery";
+
+ /**
+ * By default, the component respects the fq parameter. If you want to
elevate documents that do
+ * not match the provided filters, tag the filters in question via the local
parameter syntax
+ * fq={!tag=t1}field1:value1 and then specify the tags for exclusion via
elevate.excludeTag=t1
Review Comment:
It's sad this component didn't use a common prefix naming standard like the
other components; ah well.
--
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]
