You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/02/15 16:19:50 UTC
[GitHub] [lucene-solr] bruno-roustant commented on a change in pull request #2134: SOLR-15038: Add elevateDocsWithoutMatchingQ and onlyElevatedReprese…
bruno-roustant commented on a change in pull request #2134:
URL: https://github.com/apache/lucene-solr/pull/2134#discussion_r576267796
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -101,6 +101,8 @@
import org.xml.sax.InputSource;
import org.xml.sax.SAXException;
+import static org.apache.solr.common.params.QueryElevationParams.ELEVATE_DOCS_WITHOUT_MATCHING_Q;
Review comment:
We should not use a static import to be consistent with other existing use of the QueryElevationParams.
##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -125,22 +126,22 @@ field collapsing (with ngroups) when the number of distinct groups
**/
public class CollapsingQParserPlugin extends QParserPlugin {
-
+
Review comment:
There are a lot of space-only diff that pollute the review.
Could you revert them to keep only the meaningful modified lines please?
(If this class needs a formatting cleanup, this should be done in a separate PR)
##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -504,25 +506,27 @@ private void setQuery(ResponseBuilder rb, Elevation elevation) {
// Change the query to insert forced documents
SolrParams params = rb.req.getParams();
- if (params.getBool(QueryElevationParams.EXCLUSIVE, false)) {
- // We only want these elevated results
- rb.setQuery(new BoostQuery(elevation.includeQuery, 0f));
- } else {
- BooleanQuery.Builder queryBuilder = new BooleanQuery.Builder();
- queryBuilder.add(rb.getQuery(), BooleanClause.Occur.SHOULD);
- queryBuilder.add(new BoostQuery(elevation.includeQuery, 0f), BooleanClause.Occur.SHOULD);
- if (elevation.excludeQueries != null) {
- if (params.getBool(QueryElevationParams.MARK_EXCLUDES, false)) {
- // We are only going to mark items as excluded, not actually exclude them.
- // This works with the EditorialMarkerFactory.
- rb.req.getContext().put(EXCLUDED, elevation.excludedIds);
- } else {
- for (TermQuery tq : elevation.excludeQueries) {
- queryBuilder.add(tq, BooleanClause.Occur.MUST_NOT);
+ if (params.getBool(ELEVATE_DOCS_WITHOUT_MATCHING_Q, true)) {
Review comment:
I'm surprised to see that we skip completely the block if this param is set. I expected to see that this param changes the BooleanClause.Occur.SHOULD to a MUST at line 515 where rb.getQuery() is added. For example, with the current proposal, we skip any excluded ids. Shouldn't we change this if block?
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/QueryElevationParams.java
##########
@@ -55,4 +55,16 @@
* they be subject to whatever the sort criteria is? True by default.
*/
String USE_CONFIGURED_ELEVATED_ORDER = "useConfiguredElevatedOrder";
-}
\ No newline at end of file
+
+ /**
+ * By default, the component will also elevate docs that aren't part of the search result (matching the query).
+ * If you only want to elevate the docs that are part of the search result, set this to false. True by default.
+ */
+ String ELEVATE_DOCS_WITHOUT_MATCHING_Q = "elevateDocsWithoutMatchingQ";
+
+ /**
+ * If elevation is used in combination with the collapse query parser, we can define that we only want to return the
+ * representative and not all elevated docs.
+ */
+ String ONLY_ELEVATED_REPRESENTATIVE = "onlyElevatedRepresentative";
Review comment:
Since this is to drive the behavior of the CollapsingQParserPlugin, I think this parameter should be defined in the CollapsingQParserPlugin instead.
For example it could be named COLLECT_ELEVATED_DOCS_WHEN_COLLAPSING (default true).
##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -1241,14 +1264,16 @@ public void collect(int contextDoc) throws IOException {
// Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
if (boostedDocsCollector.collectIfBoosted(collapseKey, globalDoc)) return;
collapseStrategy.collapse(collapseKey, contextDoc, globalDoc);
-
+
} else { // Null Group...
-
- // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
- if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
- if (NullPolicy.IGNORE.getCode() != nullPolicy) {
- collapseStrategy.collapseNullGroup(contextDoc, globalDoc);
+ if(!onlyElevatedRepresentativeVisible){
+ // Check to see if we have documents boosted by the QueryElevationComponent (skip normal strategy based collection)
+ if (boostedDocsCollector.collectInNullGroupIfBoosted(globalDoc)) return;
+
+ if (NullPolicy.IGNORE.getCode() != nullPolicy) {
Review comment:
I'm not sure this block should be skipped if onlyElevatedRepresentativeVisible is true.
##########
File path: solr/core/src/java/org/apache/solr/search/CollapsingQParserPlugin.java
##########
@@ -88,6 +88,7 @@
import org.slf4j.LoggerFactory;
import static org.apache.solr.common.params.CommonParams.SORT;
+import static org.apache.solr.common.params.QueryElevationParams.ONLY_ELEVATED_REPRESENTATIVE;
Review comment:
Should not be a static import.
##########
File path: solr/solrj/src/java/org/apache/solr/common/params/QueryElevationParams.java
##########
@@ -55,4 +55,16 @@
* they be subject to whatever the sort criteria is? True by default.
*/
String USE_CONFIGURED_ELEVATED_ORDER = "useConfiguredElevatedOrder";
-}
\ No newline at end of file
+
+ /**
+ * By default, the component will also elevate docs that aren't part of the search result (matching the query).
+ * If you only want to elevate the docs that are part of the search result, set this to false. True by default.
+ */
+ String ELEVATE_DOCS_WITHOUT_MATCHING_Q = "elevateDocsWithoutMatchingQ";
Review comment:
I would prefer a name like ELEVATE_ONLY_DOCS_MATCHING_QUERY (default false) because the current behavior is to elevate docs not matching the query, so adding a new param default false sounds more intuitive.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org