diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 52dc0321c8f..4ca5f013903 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -583,7 +583,7 @@ Bug Fixes * SOLR-6631: DistributedQueue spinning on calling zookeeper getChildren() (Jessica Cheng Mallet, Mark Miller, Timothy Potter) -* SOLR-6579:SnapPuller Replication blocks clean shutdown of tomcat +* SOLR-6579: SnapPuller Replication blocks clean shutdown of tomcat (Philip Black-Knight via Noble Paul) * SOLR-6721: ZkController.ensureReplicaInLeaderInitiatedRecovery puts replica diff --git a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java index b2f9a84e3d5..8dbfa81867d 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java @@ -882,34 +882,40 @@ public class QueryComponent extends SearchComponent sreq.params.set(ResponseBuilder.FIELD_SORT_VALUES,"true"); - // TODO: should this really sendGlobalDfs if just includeScore? boolean shardQueryIncludeScore = (rb.getFieldFlags() & SolrIndexSearcher.GET_SCORES) != 0 || rb.getSortSpec().includesScore(); - if (shardQueryIncludeScore) { - sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName() + ",score"); - StatsCache statsCache = rb.req.getCore().getStatsCache(); - statsCache.sendGlobalStats(rb, sreq); - } else { - // reset so that only unique key is requested in shard requests - sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); - } - - if (distribSinglePass) { + StringBuilder additionalFL = new StringBuilder(); + boolean additionalAdded = false; + if (distribSinglePass) { String[] fls = rb.req.getParams().getParams(CommonParams.FL); if (fls != null && fls.length > 0 && (fls.length != 1 || !fls[0].isEmpty())) { // If the outer request contains actual FL's use them... sreq.params.set(CommonParams.FL, fls); + if (!fields.wantsField(keyFieldName)) { + additionalAdded = addFL(additionalFL, keyFieldName, additionalAdded); + } } else { // ... else we need to explicitly ask for all fields, because we are going to add // additional fields below sreq.params.set(CommonParams.FL, "*"); } + if (!fields.wantsScore() && shardQueryIncludeScore) { + additionalAdded = addFL(additionalFL, "score", additionalAdded); + } + } else { + // reset so that only unique key is requested in shard requests + sreq.params.set(CommonParams.FL, rb.req.getSchema().getUniqueKeyField().getName()); + if (shardQueryIncludeScore) { + additionalAdded = addFL(additionalFL, "score", additionalAdded); + } } - StringBuilder additionalFL = new StringBuilder(); - boolean additionalAdded = false; - if (!distribSinglePass || !fields.wantsField(keyFieldName)) - additionalAdded = addFL(additionalFL, keyFieldName, additionalAdded); - if ((!distribSinglePass || !fields.wantsScore()) && shardQueryIncludeScore) - additionalAdded = addFL(additionalFL, "score", additionalAdded); + + // TODO: should this really sendGlobalDfs if just includeScore? + + if (shardQueryIncludeScore) { + StatsCache statsCache = rb.req.getCore().getStatsCache(); + statsCache.sendGlobalStats(rb, sreq); + } + if (additionalAdded) sreq.params.add(CommonParams.FL, additionalFL.toString()); rb.addRequest(this, sreq); diff --git a/solr/core/src/test/org/apache/solr/handler/component/DistributedQueryComponentOptimizationTest.java b/solr/core/src/test/org/apache/solr/handler/component/DistributedQueryComponentOptimizationTest.java index d13b82ba2f9..6887ef8b551 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/DistributedQueryComponentOptimizationTest.java +++ b/solr/core/src/test/org/apache/solr/handler/component/DistributedQueryComponentOptimizationTest.java @@ -132,6 +132,18 @@ public class DistributedQueryComponentOptimizationTest extends AbstractFullDistr // fix for a bug where not all fields are returned if using multiple fl parameters, see SOLR-6796 queryWithAsserts("q", "*:*", "fl", "id", "fl", "dynamic", "sort", "payload desc", ShardParams.DISTRIB_SINGLE_PASS, "true"); + + // missing fl with sort + queryWithAsserts("q", "*:*", "sort", "payload desc", ShardParams.DISTRIB_SINGLE_PASS, "true"); + queryWithAsserts("q", "*:*", "sort", "payload desc"); + + // fl=* + queryWithAsserts("q", "*:*", "fl", "*", "sort", "payload desc", ShardParams.DISTRIB_SINGLE_PASS, "true"); + queryWithAsserts("q", "*:*", "fl", "*", "sort", "payload desc"); + + // fl=*,score + queryWithAsserts("q", "*:*", "fl", "*,score", "sort", "payload desc", ShardParams.DISTRIB_SINGLE_PASS, "true"); + queryWithAsserts("q", "*:*", "fl", "*,score", "sort", "payload desc"); } /** @@ -196,7 +208,7 @@ public class DistributedQueryComponentOptimizationTest extends AbstractFullDistr // score is optional, requested only if sorted by score if (fls.contains("score") || sortFields.contains("score")) idScoreFields.add("score"); - if (idScoreFields.containsAll(fls)) { + if (idScoreFields.containsAll(fls) && !fls.isEmpty()) { // if id and/or score are the only fields being requested then we implicitly turn on distribSinglePass=true distribSinglePass = true; } @@ -259,13 +271,23 @@ public class DistributedQueryComponentOptimizationTest extends AbstractFullDistr fail("Expected non-zero number of '" + paramName + "' parameters in request"); } Set requestedFields = new HashSet<>(); - for (String p : params) { - requestedFields.addAll(StrUtils.splitSmart(p, ',')); + if (params != null) { + for (String p : params) { + List list = StrUtils.splitSmart(p, ','); + for (String s : list) { + // make sure field names aren't duplicated in the parameters + assertTrue("Field name " + s + " was requested multiple times: params = " + requestAndParams.params, + requestedFields.add(s)); + } + } } - assertEquals("Number of requested fields do not match with expectations", expectedCount, requestedFields.size()); - for (String field : values) { - if (!requestedFields.contains(field)) { - fail("Field " + field + " not found in param: " + paramName + " request had " + paramName + "=" + requestedFields); + // if a wildcard ALL field is requested then we don't need to match exact number of params + if (!requestedFields.contains("*")) { + assertEquals("Number of requested fields do not match with expectations", expectedCount, requestedFields.size()); + for (String field : values) { + if (!requestedFields.contains(field)) { + fail("Field " + field + " not found in param: " + paramName + " request had " + paramName + "=" + requestedFields); + } } } }