You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/10/31 20:43:40 UTC
[GitHub] [solr] rseitz opened a new pull request, #1154: Solr 16496
rseitz opened a new pull request, #1154:
URL: https://github.com/apache/solr/pull/1154
https://issues.apache.org/jira/browse/SOLR-16496
# Description
QueryElevationComponent now accepts a request parameter elevateFilteredDocs that defaults to false. When this parameter is set to false (the default), or not specified at all, elevation respects the fq parameter. That is to say, an elevated document will only be included in the result set if it matches all the provided filter queries. When this parameter is set to true, an elevated document will still be included in the result set even if it doesn't match all of the provided filter queries. In essence, this parameter allows a user to toggle the precedence of elevation versus filtering. Setting the parameter to true supports a use case where an elevated document should always be returned, regardless of other criteria.
# Solution
The setQuery method in QueryElevationComponent has been updated so that when elevateFilteredDocs=true, each provided filter query is transformed into a boolean OR of the original filter query with a query that matches all of the elevated documents. This is similar to the way the QueryElevationComponent modifies the primary query. When elevateFilteredDocs is omitted or set to false, the behavior is the same as it has been previously (that is to say, filters take precedence over elevation).
# Tests
A testFQ() method has been added in QueryElevationComponentTest. The test first establishes that by default, the QueryElevationComponent respects the fq parameter. However, when elevateFilteredDocs=true, an elevated document will still be included in the result set even if it would otherwise have been filtered out. Of course, when elevatedDocuments=true, a document that is not elevated should still be subject to the filter conditions. Finally, the behavior of elevateFilteredDocs is consistent when collapsing is involved.
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/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 Solr 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`.
- [X] I have added tests for my changes.
- [X] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014484618
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
Maybe `ResponseBuilder`. There is no great place because we don't capture this feature into any one class. RB is mostly a POJO so this method would seem quite random there. But not bad. I support that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1016175440
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
> Can I assume the SolrQueryRequest inside the ResponseBuilder (obtained via SolrRequestInfo.getRequestInfo().getResponseBuilder()) would be the same as the one in the FacetContext, or at least that its request context would have the same tags inside?
Yes. Tests would fail if we're wrong on this, I'd think.
Heh, funny, that TODO to not depend on ResponseBuilder was added by me. It wasn't an important TODO.
I changed my mind on QueryUtils suitability. First I thought "no" -- all methods on QueryUtils (from my memory) are about looking at Query classes; not about SolrQueryRequest. But I see a very recent refactoring where someone added another method here, parseFilterQueries that takes the SQR. I suppose why not, let this class increase in scope. Besides... "ResponseBuilder" does not at all sound like the name of a class that would have methods pertaining to accessing queries in a *request*.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014962373
##########
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:
Not useful, I think.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1016028039
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
Yes, thx for the encouragement to do it now :) Two questions came up in implementing the `ResponseBuilder` approach. 1) I noticed that `FacetProcessor#handleFilterExclusions()` has a comment about wanting to _remove_ the `ResponseBuilder` dependency there -- how significant is this? 2) I noticed that the facet processor code gets the "tagMap" via the `SolrQueryRequest` it finds in its `FacetContext`. Can I assume the `SolrQueryRequest` inside the `ResponseBuilder` (obtained via `SolrRequestInfo.getRequestInfo().getResponseBuilder()`) would be the same as the one in the FacetContext, or at least that its request context would have the same tags inside? To make things clearer, I sketched out what the refactor would look like with RB as the destination, as well as two other possible destinations: QueryUtils and SolrQueryRequest itself. Let me know if RB still seems best to you or if either of the others looks reasonable. See: https://github.com/rseitz/solr/commit/a0675c51eed9f29b35
d0a1a2fe626dcb16812077
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014939748
##########
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:
Removed the last line. Considered adding something about the `FilterQuery` wrapping but wondered if that's too low-level for the user guide... and as the code is still in flux, I left it alone for now.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015769158
##########
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:
Agree... added a comment.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on PR #1154:
URL: https://github.com/apache/solr/pull/1154#issuecomment-1309095797
Hmm... I see the precommit failed because of the (deliberate) use of reference equality in QueryUtilsTest... (`assertTrue(queries[0] != queries[1]);`). Got a clean run locally though... Looking to see if there's a way to suppress that rule... (advice welcome, ofc)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1017054253
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
I fixed the short comment lines that tidy would "let" me fix... seems like it introduces these short lines in some scenarios and then if I fix them manually and run tidy again, it doesn't _always_ reintroduce them. LMK if there's something else I should be aware of re: using tidy. Also, I added the CHANGES.txt entry.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015770080
##########
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:
Thanks for the explanation. Removed the TODO and added a comment.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on PR #1154:
URL: https://github.com/apache/solr/pull/1154#issuecomment-1300609618
Thanks for reviewing, @risdenk and thanks to @dsmiley for feedback on the JIRA ticket. I have reworked this PR in accordance with that feedback.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014646304
##########
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:
Updated the code to throw a SolrException when a collapse filter is tagged for exclusion; added a test for that and another test to show that FunctionRangeQuery can still be tagged and excluded like other query types. (Still working on the other changes concerning FilterQuery, etc.)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on PR #1154:
URL: https://github.com/apache/solr/pull/1154#issuecomment-1309117033
Use assertSame for reference equality
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley merged PR #1154:
URL: https://github.com/apache/solr/pull/1154
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1016977516
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
OK, I've done the deed. Agree, QueryUtils feels maybe a little better than RB given the presence of parseFilterQueries there. Thx again for the feedback and let me know how it looks.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] risdenk commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1017332836
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
make the comment a single line. then run `./gradlew tidy` it will automatically break the comment across lines. you can do this for any longer length comment.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014305574
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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);
+ 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(elevation.includeQuery, BooleanClause.Occur.SHOULD);
+ updatedFilters.add(queryBuilder.build());
Review Comment:
I've modified this code to create a WrappedQuery whenever we see an ExtendedQuery, preserving the cache and cost settings.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014411718
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
As a first step, I made a private static utility method in QueryElevationComponent to house this logic. Seeing that the signature is:
private static Set<Query> getTaggedQueries(ResponseBuilder rb, List<String> desiredTags)
...I'm wondering if the method could ultimately go on ResponseBuilder itself, where it could be:
public Set<Query> getTaggedQueries(List<String> tags)
I'll continue thinking re: the TaggedQueryContext idea you described... but wondering if you had any feedback on this possibility?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on PR #1154:
URL: https://github.com/apache/solr/pull/1154#issuecomment-1311719459
Thanks to @dsmiley for all the support and feedback on this PR that kept making it better, and thanks to @risdenk for having an early look and then helping with the last step.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014304329
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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) {
Review Comment:
Agree. I changed this to ignore any PostFilter, not just CollapsingPostFilter.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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) {
Review Comment:
Done.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1013431841
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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<>();
Review Comment:
The Boolean wasn't obvious to me but now I get it. We can use com.carrotsearch.hppc.ObjectIdentityHashSet instead of IdentityHashMap since we want a set.
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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) {
Review Comment:
please rename `q` to `filter` or `filterQuery` :-)
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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) {
Review Comment:
hard-coding a comparison to a specific Query type is troubling. Maybe generically the pattern should be a PostFilter?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
Definitely should be factored out. Hard to say where; I don't like QueryUtils as the destination. How about this -- imagine a TaggedQueryContext class that encompasses most logic associated with populating "tags" and parsing it out? Some of QParser's logic would go there as well, which is where it's populated today. The proposed class wouldn't be instantiated; it'd just be utility methods pertaining to this. One could argue it'd be nice if this class was also the holder of the value in the request context instead of the List<String,List<QParser>> directly. Maybe? WDYT?
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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);
+ 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(elevation.includeQuery, BooleanClause.Occur.SHOULD);
+ updatedFilters.add(queryBuilder.build());
Review Comment:
We should probably check to see if the filter query in question also had cache=false local-param (via ExtendedQuery check). If so, we need to ensure this BooleanQuery is wrapped so that it doesn't cache either.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015529531
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
FYI I'm planning to leave this change till the other items in the PR are resolved. And I have a little bit of reluctance to broadening this PR to include changes outside QueryElevationComponent itself :) But if you'd like to see how this refactor would look right now, LMK and I'll go for it.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015524337
##########
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:
Looking at the other parameter names, I see that some include a form of "elevate" (`enableElevation`, `forceElevation`, `useConfiguredElevatedOrder`, `elevateIds`) and others don't (`exclusive`, `markExcludes`, `excludeIds`). None of them include dot. Is it better to use a prefix naming standard for all new params, starting now, or try to be consistent with what's already there? I'm not seeing much of a pattern in the existing param names to try to match, so I'll plan to leave this as `elevate.excludeTags` unless others have feedback?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014960224
##########
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:
Definitely too low level detail. The use of the FilterQuery (itself an internal detail) will cause the cache usage to be the same as the user expects.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014403344
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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<>();
Review Comment:
Looks like we can also use Collections.newSetFromMap(new IdentityHashMap<>()). I prefer this because it's a java.util.Set. This makes a difference if we write a utility method that returns one of these. If the return type is java.util.Set we can return an immutable Collections.emptySet() if and when we want. If the return type is ObjectIdentityHashSet we have to instantiate a new one even if it's going to be empty, unless we keep a static instance of an empty one around... but there doesn't seem to be an easy way to make it immutable.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014559245
##########
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:
I see. FunctionRangeQuery would work; it's Collapsing that is special. Let's check if the user tagged a Collapse query to be excluded and throw a SolrException with a BAD_REQUEST if they try and do that.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014939210
##########
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:
I've updated the PR to wrap the original filter in a FilterQuery if it's configured to cache (and if it wasn't a FilterQuery already). The whole BooleanQuery combo is then placed in a WrappedQuery that's set not to cache.
Let me know if the changes match what you had in mind.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1014542480
##########
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:
I put that there because otherwise... if a collapse filter is tagged for exclusion, as in:
`fq={!collapse tag=test1 field=str_s sort='score desc'}&elevate.excludeTags=test1`
...we would create a BooleanQuery with the CollapsingPostFilter as one of the clauses. Ultimately, createWeight() would be called on the BooleanQuery, and then its components, leading to an UnsupportedOperationException from the CollapsingPostFilter.
The original version of this PR did not allow filters to be excluded selectively, so I thought it would be best to ignore the CollapsingPostFilter so a user could invoke the "exclude all filters" functionality and still collapse. Now that the PR supports selective exclusion, I agree, we should give the user an error if they specifically tag the collapsing filter for exclusion. Simplest would be to consider any PostFilter tagged for exclusion as an error -- what do you think of that approach? Other PostFilters to consider are AnalyticsQuery and FunctionRangeQuery...
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1018199451
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
Nice technique Kevin. For me -- I use IntelliJ configured with the Google Java Format plugin, so I can easily use my IDE to reformat lines using standard hotkeys etc. Often "tidy" has nothing left to do.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1017006252
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
some comment end of line reflow to do here (and elsewhere in your PR)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on PR #1154:
URL: https://github.com/apache/solr/pull/1154#issuecomment-1309110752
Precommit issue should be fixed now. Added `@SuppressWarnings("ReferenceEquality")` in `QueryUtilsTest`.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015553156
##########
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:
Fine keep it but it matters little.
In Solr 9, the execution order is fundamentally dictated by the internals of the Query (which can be "smart" / index statistic driven), not much by this user provided cost. `cost` is mostly useful for PostFilters when cost>=100. It is a tie-break for Queries that otherwise would produce the same internal cost, so I suppose that would be retained. For a nested query (i.e. inside a BQ), it's ignored.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015569162
##########
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:
I argue the current names are sloppy tech-debt that should change. Doing that is out of scope of this PR for sure. I recognize, this parameter looks different until this is rectified. A comment there could acknowledge this.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
dsmiley commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1015975637
##########
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java:
##########
@@ -553,6 +560,82 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
}
}
+ /**
+ * Updates any filters that have been tagged for exclusion. Filters can be tagged for exclusion
+ * via the syntax fq={!tag=t1}field1:value1&elevate.excludeTags=t1 This method modifies each
+ * "excluded" filter so that it becomes a boolean OR of the original filter with an "include
+ * query" that matches the elevated documents by their IDs. To be clear, the "excluded" filters
+ * are not ignored entirely, but rather broadened so that the elevated documents are allowed
+ * through.
+ */
+ private void setFilters(ResponseBuilder rb, Elevation elevation) {
+ SolrParams params = rb.req.getParams();
+
+ String tagStr = params.get(QueryElevationParams.ELEVATE_EXCLUDE_TAGS);
+ if (StringUtils.isEmpty(tagStr)) {
+ // the parameter that specifies tags for exclusion was not provided or had no value
+ return;
+ }
+
+ List<String> excludeTags = StrUtils.splitSmart(tagStr, ',');
+ excludeTags.removeIf(s -> StringUtils.isBlank(s));
+ if (excludeTags.isEmpty()) {
+ // the parameter that specifies tags for exclusion was provided but the tag names were blank
+ return;
+ }
+
+ List<Query> filters = rb.getFilters();
+ if (filters == null || filters.isEmpty()) {
+ // no filters were provided
+ return;
+ }
+
+ Map<?, ?> tagMap = (Map<?, ?>) rb.req.getContext().get("tags");
+ if (tagMap == null) {
+ // 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
Review Comment:
Let's do it; okay? ResponseBuilder is fine.
In my experience, better to do the refactoring as part of an issue like this because it will likely be forgotten if left to the future.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1017024804
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
I've been running `gradlew tidy` and it sometimes breaks my comments into very short likes as you pointed out there. Is there any secret to getting it not to do this, or should I just manually fix the comments and avoid running `tidy` one last time?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] rseitz commented on a diff in pull request #1154: SOLR-16496: provide option for Query Elevation Component to bypass filters
Posted by GitBox <gi...@apache.org>.
rseitz commented on code in PR #1154:
URL: https://github.com/apache/solr/pull/1154#discussion_r1017979012
##########
solr/core/src/test/org/apache/solr/search/QueryUtilsTest.java:
##########
@@ -0,0 +1,121 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import java.util.ArrayList;
+import java.util.Set;
+import org.apache.lucene.search.Query;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.params.CommonParams;
+import org.apache.solr.handler.component.QueryComponent;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.SearchComponent;
+import org.apache.solr.request.SolrQueryRequest;
+import org.apache.solr.response.SolrQueryResponse;
+import org.junit.Test;
+
+/** Tests the utility methods in QueryUtils. */
+public class QueryUtilsTest extends SolrTestCaseJ4 {
+
+ @Test
+ public void testGetTaggedQueries() throws Exception {
+ try {
+ initCore("solrconfig.xml", "schema.xml");
+
+ SearchComponent queryComponent =
+ h.getCore().getSearchComponent(QueryComponent.COMPONENT_NAME);
+
+ // first make sure QueryUtils.getTaggedQueries() behaves properly on an "empty" request
+ try (SolrQueryRequest request = req()) {
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ queryComponent.prepare(rb);
+ assertEquals(0, QueryUtils.getTaggedQueries(request, Set.of("t0")).size());
+ }
+
+ // now test with a request that includes multiple tagged queries
+ try (SolrQueryRequest request =
+ req(
+ CommonParams.Q, "{!tag=t0}text:zzzz",
+ CommonParams.FQ, "{!tag=t1,t2}str_s:a",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t3}str_s:b",
+ CommonParams.FQ, "{!tag=t4}str_s:b",
+ CommonParams.FQ, "str_s:b",
+ CommonParams.FQ, "str_s:c")) {
+
+ ResponseBuilder rb =
+ new ResponseBuilder(request, new SolrQueryResponse(), new ArrayList<>());
+
+ // the "tag map" will not be populated in the SolrQueryRequest's "context" until
+ // the QParsers have been initialized; before such time, QueryUtils.getTaggedQueries will
+ // always return
+ // an empty set
Review Comment:
Thanks @risdenk. Did that and it worked.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org