You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by mu...@apache.org on 2019/07/02 03:08:40 UTC
[lucene-solr] branch branch_8x updated: SOLR-13404: support
group.query in multishard env with group.main=true
This is an automated email from the ASF dual-hosted git repository.
munendrasn pushed a commit to branch branch_8x
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/branch_8x by this push:
new d811f86 SOLR-13404: support group.query in multishard env with group.main=true
d811f86 is described below
commit d811f8634252c9567966dc64482ff3bd2abb69ad
Author: Munendra S N <mu...@apache.org>
AuthorDate: Mon Jul 1 12:11:58 2019 +0530
SOLR-13404: support group.query in multishard env with group.main=true
group.query after execution forms QueryCommandResult. In case of
group.main=true or group.format=simple, QueryCommandResult was not
consumed in EndResultTransformer. Also, MainEndResultTransformer assumed
that always group.field would be specified. When group.field not specified
it failed with AIOOBE. After adding suppport for QueryCommandResult in
EndResultTransformers and handling AIOOBE, group.query started giving results
Working on tests exposed few other issues. Results differed b/w standalone
& distributed mode.
* One of the reason is that TopGroupShardResponseProcessor doesn't consider correct
limit and offset when group format is simple. In case of simple, start and rows should be used
as limit and offset instead of group.limit and group.offset.
* Secondly, In distributed second phase grouping, computing docsToCollect didn't consider
group response format. This issue is again similar to above issue
* offset(group.offset or start) not being considered during TopDocs#merge caused
different results. The fix was to use to offset in merge process
* group.offset doesn't support negative values but there is no checks on the value.
In case of negative values AIOOBE. Now, checks are added for negative values and
returns proper error message(this change is for both standalone and distrbuted).
Validation is done only in case of group.format=grouped as that is only case when
group.offset is consumed.
Fixing above issues resolved the differences b/w standalone and distributed mode.
---
solr/CHANGES.txt | 2 +
.../solr/handler/component/QueryComponent.java | 18 ++++++++-
.../TopGroupsShardResponseProcessor.java | 22 +++++++++--
.../MainEndResultTransformer.java | 45 +++++++++++++++++++---
.../SimpleEndResultTransformer.java | 35 +++++++++++++++--
.../org/apache/solr/TestDistributedGrouping.java | 42 +++++++++++++++++---
.../test/org/apache/solr/TestGroupingSearch.java | 15 +++++++-
7 files changed, 159 insertions(+), 20 deletions(-)
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index 80b949f..f529b4c 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -159,6 +159,8 @@ Bug Fixes
* SOLR-9409: Improve error message for unsupported field types and return 400 error code on
unsupported values in collapsing (hossman, Munendra S N)
+* SOLR-13404: Support group.query in multi-shard environment when group.main=true or group.format=simple (Munendra S N)
+
Other Changes
----------------------
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 868b5c9..5f7b218 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
@@ -301,6 +301,14 @@ public class QueryComponent extends SearchComponent
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() & SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE, false));
+
+ // when group.format=grouped then, validate group.offset
+ // for group.main=true and group.format=simple, start value is used instead of group.offset
+ // and start is already validate above for negative values
+ if (!(groupingSpec.isMain() || groupingSpec.getResponseFormat() == Grouping.Format.simple) &&
+ groupingSpec.getWithinGroupSortSpec().getOffset() < 0) {
+ throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "'group.offset' parameter cannot be negative");
+ }
}
@@ -730,6 +738,7 @@ public class QueryComponent extends SearchComponent
// if the client set shards.rows set this explicity
sreq.params.set(CommonParams.ROWS,rb.shards_rows);
} else {
+ // what if rows<0 as it is allowed for grouped request??
sreq.params.set(CommonParams.ROWS, rb.getSortSpec().getOffset() + rb.getSortSpec().getCount());
}
@@ -1365,10 +1374,17 @@ public class QueryComponent extends SearchComponent
);
}
+ SortSpec groupSortSpec = groupingSpec.getGroupSortSpec();
+ // use start and rows for group.format=simple and group.main=true
+ if (rb.getGroupingSpec().getResponseFormat() == Grouping.Format.simple || rb.getGroupingSpec().isMain()) {
+ // would this ever be negative, as shardRequest sets rows to offset+limit
+ int limit = groupSortSpec.getCount();
+ docsToCollect = limit >= 0? limit + groupSortSpec.getOffset() : Integer.MAX_VALUE;
+ }
for (String query : groupingSpec.getQueries()) {
secondPhaseBuilder.addCommandField(new Builder()
.setDocsToCollect(docsToCollect)
- .setSort(groupingSpec.getGroupSort())
+ .setSort(groupSortSpec.getSort())
.setQuery(query, rb.req)
.setDocSet(searcher)
.build()
diff --git a/solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java b/solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java
index 3ccecd1..92589d6 100644
--- a/solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java
+++ b/solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/TopGroupsShardResponseProcessor.java
@@ -58,9 +58,12 @@ public class TopGroupsShardResponseProcessor implements ShardResponseProcessor {
Sort withinGroupSort = rb.getGroupingSpec().getSortWithinGroup();
assert withinGroupSort != null;
+ boolean simpleOrMain = rb.getGroupingSpec().getResponseFormat() == Grouping.Format.simple ||
+ rb.getGroupingSpec().isMain();
+
// If group.format=simple group.offset doesn't make sense
int groupOffsetDefault;
- if (rb.getGroupingSpec().getResponseFormat() == Grouping.Format.simple || rb.getGroupingSpec().isMain()) {
+ if (simpleOrMain) {
groupOffsetDefault = 0;
} else {
groupOffsetDefault = rb.getGroupingSpec().getWithinGroupOffset();
@@ -162,6 +165,16 @@ public class TopGroupsShardResponseProcessor implements ShardResponseProcessor {
rb.mergedTopGroups.put(groupField, TopGroups.merge(topGroups.toArray(topGroupsArr), groupSort, withinGroupSort, groupOffsetDefault, docsPerGroup, TopGroups.ScoreMergeMode.None));
}
+ // calculate topN and start for group.query
+ int topN = docsPerGroupDefault >= 0? docsPerGroupDefault: Integer.MAX_VALUE;
+ int start = groupOffsetDefault;
+ if (simpleOrMain) {
+ // use start and rows here
+ start = rb.getGroupingSpec().getGroupSortSpec().getOffset();
+ int limit = rb.getGroupingSpec().getGroupSortSpec().getCount();
+ topN = limit >= 0? limit: Integer.MAX_VALUE;
+ }
+
for (String query : commandTopDocs.keySet()) {
List<QueryCommandResult> queryCommandResults = commandTopDocs.get(query);
List<TopDocs> topDocs = new ArrayList<>(queryCommandResults.size());
@@ -179,12 +192,13 @@ public class TopGroupsShardResponseProcessor implements ShardResponseProcessor {
}
}
- int topN = rb.getGroupingSpec().getOffset() + rb.getGroupingSpec().getLimit();
final TopDocs mergedTopDocs;
if (withinGroupSort.equals(Sort.RELEVANCE)) {
- mergedTopDocs = TopDocs.merge(topN, topDocs.toArray(new TopDocs[topDocs.size()]));
+ mergedTopDocs = TopDocs.merge(
+ start, topN, topDocs.toArray(new TopDocs[topDocs.size()]), true);
} else {
- mergedTopDocs = TopDocs.merge(withinGroupSort, topN, topDocs.toArray(new TopFieldDocs[topDocs.size()]));
+ mergedTopDocs = TopDocs.merge(
+ withinGroupSort, start, topN, topDocs.toArray(new TopFieldDocs[topDocs.size()]), true);
}
rb.mergedQueryCommandResults.put(query, new QueryCommandResult(mergedTopDocs, mergedMatches, maxScore));
}
diff --git a/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/MainEndResultTransformer.java b/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/MainEndResultTransformer.java
index 3e11abc..0e03e4e 100644
--- a/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/MainEndResultTransformer.java
+++ b/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/MainEndResultTransformer.java
@@ -16,14 +16,19 @@
*/
package org.apache.solr.search.grouping.endresulttransformer;
+import java.util.Map;
+
+import org.apache.commons.lang3.ArrayUtils;
import org.apache.lucene.search.ScoreDoc;
+import org.apache.lucene.search.TopDocs;
import org.apache.lucene.search.grouping.GroupDocs;
import org.apache.lucene.search.grouping.TopGroups;
import org.apache.lucene.util.BytesRef;
+import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.handler.component.ResponseBuilder;
-
-import java.util.Map;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import org.apache.solr.search.grouping.distributed.command.QueryCommandResult;
/**
* Implementation of {@link EndResultTransformer} that transforms the grouped result into the main result list in the
@@ -33,7 +38,17 @@ public class MainEndResultTransformer implements EndResultTransformer {
@Override
public void transform(Map<String, ?> result, ResponseBuilder rb, SolrDocumentSource solrDocumentSource) {
- Object value = result.get(rb.getGroupingSpec().getFields()[0]);
+ GroupingSpecification groupingSpec = rb.getGroupingSpec();
+ String[] entries = groupingSpec.getFields();
+ if (ArrayUtils.isEmpty(entries)) {
+ entries = groupingSpec.getQueries();
+ if (ArrayUtils.isEmpty(entries)) {
+ // group.func is not supported in distributed mode
+ // so when group.field or group.query is not specified return
+ return;
+ }
+ }
+ Object value = result.get(entries[0]);
if (TopGroups.class.isInstance(value)) {
@SuppressWarnings("unchecked")
TopGroups<BytesRef> topGroups = (TopGroups<BytesRef>) value;
@@ -41,19 +56,39 @@ public class MainEndResultTransformer implements EndResultTransformer {
docList.setStart(rb.getGroupingSpec().getOffset());
docList.setNumFound(rb.totalHitCount);
- Float maxScore = Float.NEGATIVE_INFINITY;
+ float maxScore = Float.NEGATIVE_INFINITY;
for (GroupDocs<BytesRef> group : topGroups.groups) {
for (ScoreDoc scoreDoc : group.scoreDocs) {
if (maxScore < scoreDoc.score) {
maxScore = scoreDoc.score;
}
- docList.add(solrDocumentSource.retrieve(scoreDoc));
+ SolrDocument solrDocument = solrDocumentSource.retrieve(scoreDoc);
+ if (solrDocument != null) {
+ docList.add(solrDocument);
+ }
}
}
if (maxScore != Float.NEGATIVE_INFINITY) {
docList.setMaxScore(maxScore);
}
rb.rsp.addResponse(docList);
+ } else if (value instanceof QueryCommandResult) {
+ QueryCommandResult queryCommandResult = (QueryCommandResult) value;
+ SolrDocumentList docList = new SolrDocumentList();
+ TopDocs topDocs = queryCommandResult.getTopDocs();
+
+ docList.setStart(rb.getGroupingSpec().getOffset());
+ docList.setNumFound(topDocs.totalHits.value);
+ if (!Float.isNaN(queryCommandResult.getMaxScore())) {
+ docList.setMaxScore(queryCommandResult.getMaxScore());
+ }
+ for (ScoreDoc scoreDoc : topDocs.scoreDocs) {
+ SolrDocument solrDocument = solrDocumentSource.retrieve(scoreDoc);
+ if (solrDocument != null) {
+ docList.add(solrDocument);
+ }
+ }
+ rb.rsp.addResponse(docList);
}
}
}
diff --git a/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/SimpleEndResultTransformer.java b/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/SimpleEndResultTransformer.java
index 593f84f..01adb45 100644
--- a/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/SimpleEndResultTransformer.java
+++ b/solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/SimpleEndResultTransformer.java
@@ -16,16 +16,19 @@
*/
package org.apache.solr.search.grouping.endresulttransformer;
+import java.util.Map;
+
import org.apache.lucene.search.ScoreDoc;
+import org.apache.lucene.search.TopDocs;
import org.apache.lucene.search.grouping.GroupDocs;
import org.apache.lucene.search.grouping.TopGroups;
import org.apache.lucene.util.BytesRef;
+import org.apache.solr.common.SolrDocument;
import org.apache.solr.common.SolrDocumentList;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.handler.component.ResponseBuilder;
-
-import java.util.Map;
+import org.apache.solr.search.grouping.distributed.command.QueryCommandResult;
/**
* Implementation of {@link EndResultTransformer} that transforms the grouped result into a single flat list.
@@ -49,13 +52,16 @@ public class SimpleEndResultTransformer implements EndResultTransformer {
docList.setStart(rb.getGroupingSpec().getOffset());
docList.setNumFound(topGroups.totalHitCount);
- Float maxScore = Float.NEGATIVE_INFINITY;
+ float maxScore = Float.NEGATIVE_INFINITY;
for (GroupDocs<BytesRef> group : topGroups.groups) {
for (ScoreDoc scoreDoc : group.scoreDocs) {
if (maxScore < scoreDoc.score) {
maxScore = scoreDoc.score;
}
- docList.add(solrDocumentSource.retrieve(scoreDoc));
+ SolrDocument solrDocument = solrDocumentSource.retrieve(scoreDoc);
+ if (solrDocument != null) {
+ docList.add(solrDocument);
+ }
}
}
if (maxScore != Float.NEGATIVE_INFINITY) {
@@ -63,6 +69,27 @@ public class SimpleEndResultTransformer implements EndResultTransformer {
}
command.add("doclist", docList);
commands.add(entry.getKey(), command);
+ } else if (value instanceof QueryCommandResult) {
+ QueryCommandResult queryCommandResult = (QueryCommandResult) value;
+ NamedList<Object> command = new SimpleOrderedMap<>();
+ command.add("matches", queryCommandResult.getMatches());
+
+ TopDocs topDocs = queryCommandResult.getTopDocs();
+ SolrDocumentList docList = new SolrDocumentList();
+ docList.setStart(rb.getGroupingSpec().getOffset());
+ docList.setNumFound(topDocs.totalHits.value);
+
+ if (!Float.isNaN(queryCommandResult.getMaxScore())) {
+ docList.setMaxScore(queryCommandResult.getMaxScore());
+ }
+ for (ScoreDoc scoreDoc : topDocs.scoreDocs) {
+ SolrDocument solrDocument = solrDocumentSource.retrieve(scoreDoc);
+ if (solrDocument != null) {
+ docList.add(solrDocument);
+ }
+ }
+ command.add("doclist", docList);
+ commands.add(entry.getKey(), command);
}
}
diff --git a/solr/core/src/test/org/apache/solr/TestDistributedGrouping.java b/solr/core/src/test/org/apache/solr/TestDistributedGrouping.java
index de55b44..73315f5 100644
--- a/solr/core/src/test/org/apache/solr/TestDistributedGrouping.java
+++ b/solr/core/src/test/org/apache/solr/TestDistributedGrouping.java
@@ -24,6 +24,7 @@ import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrDocumentList;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
@@ -187,13 +188,12 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
// SOLR-4150: what if group.query has no matches,
// or only matches on one shard
- query("q", "*:*", "rows", 100, "fl", "id," + i1, "group", "true",
- "group.query", t1 + ":kings OR " + t1 + ":eggs",
+ query("q", "*:*", "rows", 100, "fl", "id," + i1, "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs",
"group.query", "id:5", // single doc, so only one shard will have it
"group.limit", -1, "sort", i1 + " asc, id asc");
- handle.put(t1 + ":this_will_never_match", SKIP); // :TODO: SOLR-4181
- query("q", "*:*", "rows", 100, "fl", "id," + i1, "group", "true",
- "group.query", t1 + ":kings OR " + t1 + ":eggs",
+ query("q", "*:*", "rows", 100, "fl", "id," + i1, "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs",
"group.query", t1 + ":this_will_never_match",
"group.limit", 10, "sort", i1 + " asc, id asc");
@@ -209,6 +209,38 @@ public class TestDistributedGrouping extends BaseDistributedSearchTestCase {
"group.field", i1,
"group.limit", 10, "sort", i1 + " asc, id asc");
+ // SOLR-13404
+ query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs",
+ "fl", "id", "group.format", "grouped", "group.limit", "2", "group.offset", "2",
+ "sort", i1 + " asc, id asc");
+ query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs",
+ "fl", "id", "group.format", "grouped", "group.limit", "-12",
+ "sort", i1 + " asc, id asc");
+
+ SolrException exception = expectThrows(SolrException.class, () -> query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs", "group.offset", "-1")
+ );
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code());
+ assertTrue(exception.getMessage().contains("'group.offset' parameter cannot be negative"));
+
+ query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs", "group.limit", "3",
+ "fl", "id", "group.format", "simple", "sort", i1 + " asc, id asc");
+ query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs",
+ "fl", "id", "group.main", "true", "sort", i1 + " asc, id asc");
+ query("q", "*:*",
+ "group", "true",
+ "group.query", t1 + ":kings OR " + t1 + ":eggs", "rows", "13", "start", "2",
+ "fl", "id", "group.main", "true", "sort", i1 + " asc, id asc");
+
// SOLR-3109
query("q", t1 + ":eggs", "rows", 100, "fl", "id," + i1, "group", "true", "group.field", i1, "group.limit", 10, "sort", tlong + " asc, id asc");
query("q", i1 + ":232", "rows", 100, "fl", "id," + i1, "group", "true", "group.field", i1, "group.limit", 10, "sort", tlong + " asc, id asc");
diff --git a/solr/core/src/test/org/apache/solr/TestGroupingSearch.java b/solr/core/src/test/org/apache/solr/TestGroupingSearch.java
index fe5894a..b6460f6 100644
--- a/solr/core/src/test/org/apache/solr/TestGroupingSearch.java
+++ b/solr/core/src/test/org/apache/solr/TestGroupingSearch.java
@@ -31,6 +31,7 @@ import java.util.Map;
import java.util.TreeMap;
import org.apache.solr.client.solrj.impl.BinaryResponseParser;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.CommonParams;
import org.apache.solr.common.params.GroupParams;
import org.apache.solr.common.util.Utils;
@@ -99,7 +100,7 @@ public class TestGroupingSearch extends SolrTestCaseJ4 {
,"//arr[@name='groups']/lst[1]/str[@name='groupValue'][.='author3']"
,"//arr[@name='groups']/lst[1]/result[@numFound='1']"
,"//arr[@name='groups']/lst[1]/result/doc/*[@name='id'][.='5']"
-
+
,"//arr[@name='groups']/lst[2]/str[@name='groupValue'][.='author2']"
,"//arr[@name='groups']/lst[2]/result[@numFound='2']"
,"//arr[@name='groups']/lst[2]/result/doc/*[@name='id'][.='4']"
@@ -122,6 +123,18 @@ public class TestGroupingSearch extends SolrTestCaseJ4 {
, "//arr[@name='groups']/lst[2]/result[@numFound='3']"
, "//arr[@name='groups']/lst[2]/result/doc/*[@name='id'][.='5']"
);
+
+ SolrException exception = expectThrows(SolrException.class, () -> {
+ h.query(req("q", "title:title", "group", "true", "group.field", "group_i", "group.offset", "-1"));
+ });
+ assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, exception.code());
+ assertEquals("'group.offset' parameter cannot be negative", exception.getMessage());
+
+ // for group.main=true and group.format=simple, group.offset is not consumed
+ assertQ(req("q", "title:title", "group", "true", "group.field", "group_i",
+ "group.offset", "-1", "group.format", "simple"));
+ assertQ(req("q", "title:title", "group", "true", "group.field", "group_i",
+ "group.offset", "-1", "group.main", "true"));
}
@Test