[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320456939
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
Review comment:
I removed `addSearchGroupToShard` and pushed here. Test are still
successful, thanks!
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320441007
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
Review comment:
(a) good catch, `addSearchGroupToShard` only sets `searchGroupToShards` that
seems to be used only here:
https://github.com/apache/lucene-solr/blob/1d85cd783863f75cea133fb9c452302214165a4d/solr/core/src/java/org/apache/solr/search/grouping/distributed/requestfactory/TopGroupsShardRequestFactory.java#L69
and if we enable las vegas we are going to skip topgroups so we should be
fine.
(b) um I would still skip it explicit instead of relying on implicit signal
that might change over the 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320422541
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
+group.topDocScore,
+new TotalHits(1, TotalHits.Relation.EQUAL_TO), /* we don't know
the actual number of hits in the group- we set it to 1 as we only keep track of
the top doc */
+new ShardDoc[] { sdoc }, /* only top doc */
+group.groupValue,
+group.sortValues);
+ }
+
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320422334
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
Review comment:
Ok, I double checked the code and it looks safe, merged - thank you
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
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320201290
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
+group.topDocScore,
+new TotalHits(1, TotalHits.Relation.EQUAL_TO), /* we don't know
the actual number of hits in the group- we set it to 1 as we only keep track of
the top doc */
+new ShardDoc[] { sdoc }, /* only top doc */
+group.groupValue,
+group.sortValues);
+ }
+
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320201261
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
Review comment:
Ok, after reviewing the code I agree that is OK and better to use `NaN`
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
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320021306
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
Review comment:
I'm a bit concerned about using `NaN` because *NaN cannot be compared with
any floating type value* I'll have to double check that we are not sorting on
that.
This is an automated message from the Apache Git Service.
To respond to the message, please log
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320021306
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
Review comment:
I'm a bit concerned about using `NaN` because *NaN cannot be compared with
any floating type value* I'll have to double check that we not sorting on that.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320020253
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
+group.topDocScore,
+new TotalHits(1, TotalHits.Relation.EQUAL_TO), /* we don't know
the actual number of hits in the group- we set it to 1 as we only keep track of
the top doc */
+new ShardDoc[] { sdoc }, /* only top doc */
+group.groupValue,
+group.sortValues);
+ }
+
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320020253
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,116 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardRequest;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ @Override
+ public void process(ResponseBuilder rb, ShardRequest shardRequest) {
+super.process(rb, shardRequest);
+TopGroupsShardResponseProcessor.fillResultIds(rb);
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort =
groupingSpecification.getGroupSortSpec().getSort();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+final ShardDoc sdoc = new ShardDoc();
+sdoc.score = group.topDocScore;
+sdoc.id = group.topDocSolrId;
+sdoc.shard = shard;
+
+groups[index++] = new GroupDocs<>(group.topDocScore,
+group.topDocScore,
+new TotalHits(1, TotalHits.Relation.EQUAL_TO), /* we don't know
the actual number of hits in the group- we set it to 1 as we only keep track of
the top doc */
+new ShardDoc[] { sdoc }, /* only top doc */
+group.groupValue,
+group.sortValues);
+ }
+
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320017507
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/GroupedEndResultTransformer.java
##
@@ -75,7 +75,13 @@ public void transform(Map result,
ResponseBuilder rb, SolrDocumentSou
SimpleOrderedMap groupResult = new SimpleOrderedMap<>();
if (group.groupValue != null) {
// use createFields so that fields having doc values are also
supported
-List fields =
groupField.createFields(group.groupValue.utf8ToString());
+final String groupValue;
+if (rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ groupValue =
groupField.getType().indexedToReadable(group.groupValue.utf8ToString());
Review comment:
Looks good, merged
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r320017542
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -142,4 +150,58 @@ private NamedList
serializeSearchGroup(Collection> data, S
return result;
}
+ public static class SkipSecondStepSearchResultResultTransformer extends
SearchGroupsResultTransformer {
+
+private static final String TOP_DOC_SOLR_ID_KEY = "topDocSolrId";
+private static final String TOP_DOC_SCORE_KEY = "topDocScore";
+private static final String SORTVALUES_KEY = "sortValues";
+
+private final SchemaField uniqueField;
+
+public SkipSecondStepSearchResultResultTransformer(SolrIndexSearcher
searcher) {
+ super(searcher);
+ this.uniqueField = searcher.getSchema().getUniqueKeyField();
+}
+
+@Override
+protected Object[] getSortValues(Object groupDocs) {
+ NamedList groupInfo = (NamedList) groupDocs;
+ final ArrayList sortValues = (ArrayList)
groupInfo.get(SORTVALUES_KEY);
+ return sortValues.toArray(new Comparable[sortValues.size()]);
+}
+
+@Override
+protected SearchGroup deserializeOneSearchGroup(SchemaField
groupField, String groupValue,
+ SortField[]
groupSortField, Object rawSearchGroupData) {
+ SearchGroup searchGroup =
super.deserializeOneSearchGroup(groupField, groupValue, groupSortField,
rawSearchGroupData);
+ NamedList groupInfo = (NamedList) rawSearchGroupData;
+ searchGroup.topDocLuceneId = DocIdSetIterator.NO_MORE_DOCS;
+ searchGroup.topDocScore = (float) groupInfo.get(TOP_DOC_SCORE_KEY);
+ searchGroup.topDocSolrId = groupInfo.get(TOP_DOC_SOLR_ID_KEY);
+ return searchGroup;
+}
+
+@Override
+protected Object serializeOneSearchGroup(SortField[] groupSortField,
SearchGroup searchGroup) {
+ Document luceneDoc = null;
+ /** Use the lucene id to get the unique solr id so that it can be sent
to the federator.
+ * The lucene id of a document is not unique across all shards i.e.
different documents
+ * in different shards could have the same lucene id, whereas the solr
id is guaranteed
+ * to be unique so this is what we need to return to the federator
+ **/
+ try {
+luceneDoc = searcher.doc(searchGroup.topDocLuceneId,
Collections.singleton(uniqueField.getName()));
Review comment:
Looks good, merged
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r303058934
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -34,17 +41,37 @@
/**
* Implementation for transforming {@link SearchGroup} into a {@link
NamedList} structure and visa versa.
*/
-public class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
+public abstract class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
Review comment:
Thanks @cpoerschke https://issues.apache.org/jira/browse/SOLR-13585 Looks
good to me, I would merge it and then rebase this code on top of 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r295265871
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,47 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ // check if prefix is a prefix of the array
+ private static boolean isPrefix(SortField[] prefix, SortField[] array){
Review comment:
Done in 0563e66
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r295265453
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -80,50 +106,120 @@ public NamedList transform(List data) throws
IOException {
final NamedList> rawSearchGroups =
(NamedList>) topGroupsAndGroupCount.get(TOP_GROUPS);
if (rawSearchGroups != null) {
for (Map.Entry> rawSearchGroup :
rawSearchGroups){
- SearchGroup searchGroup = new SearchGroup<>();
- SchemaField groupField = rawSearchGroup.getKey() != null?
searcher.getSchema().getFieldOrNull(command.getKey()) : null;
- searchGroup.groupValue = null;
- if (rawSearchGroup.getKey() != null) {
-if (groupField != null) {
- BytesRefBuilder builder = new BytesRefBuilder();
- groupField.getType().readableToIndexed(rawSearchGroup.getKey(),
builder);
- searchGroup.groupValue = builder.get();
-} else {
- searchGroup.groupValue = new BytesRef(rawSearchGroup.getKey());
-}
- }
- searchGroup.sortValues = rawSearchGroup.getValue().toArray(new
Comparable[rawSearchGroup.getValue().size()]);
- for (int i = 0; i < searchGroup.sortValues.length; i++) {
-SchemaField field = groupSort.getSort()[i].getField() != null ?
searcher.getSchema().getFieldOrNull(groupSort.getSort()[i].getField()) : null;
-searchGroup.sortValues[i] =
ShardResultTransformerUtils.unmarshalSortValue(searchGroup.sortValues[i],
field);
- }
- searchGroups.add(searchGroup);
+ searchGroups.add(deserializeOneSearchGroup(command.getKey(),
groupSort, rawSearchGroup.getKey(), rawSearchGroup.getValue()));
}
}
-
final Integer groupCount = (Integer)
topGroupsAndGroupCount.get(GROUP_COUNT);
result.put(command.getKey(), new
SearchGroupsFieldCommandResult(groupCount, searchGroups));
}
return result;
}
- private NamedList serializeSearchGroup(Collection>
data, SearchGroupsFieldCommand command) {
-final NamedList result = new NamedList<>(data.size());
+ public static class DefaultSearchResultResultTransformer extends
SearchGroupsResultTransformer {
-for (SearchGroup searchGroup : data) {
+public DefaultSearchResultResultTransformer(SolrIndexSearcher searcher) {
+ super(searcher);
+}
+
+// given a raw search group will return its array sort values.
+protected Object[] getSortValues(Object groupDocs){
+ List docs = (List)groupDocs;
+ return docs.toArray(new Comparable[docs.size()]);
+}
+
+@Override
+protected SearchGroup deserializeOneSearchGroup(String
groupField, Sort groupSort, String groupKey, Object rawSearchGroup){
+ SearchGroup searchGroup = new SearchGroup<>();
+ SchemaField schemaGroupField = groupKey != null?
searcher.getSchema().getFieldOrNull(groupField) : null;
+ searchGroup.groupValue = null;
+ if (groupKey != null) {
+if (schemaGroupField != null) {
+ BytesRefBuilder builder = new BytesRefBuilder();
+ schemaGroupField.getType().readableToIndexed(groupKey, builder);
+ searchGroup.groupValue = builder.get();
+} else {
+ searchGroup.groupValue = new BytesRef(groupKey);
+}
+ }
+ searchGroup.sortValues = getSortValues(rawSearchGroup);
+ for (int i = 0; i < searchGroup.sortValues.length; i++) {
+final String sortField = groupSort.getSort()[i].getField();
+SchemaField field = sortField != null ?
searcher.getSchema().getFieldOrNull(sortField) : null;
+searchGroup.sortValues[i] =
ShardResultTransformerUtils.unmarshalSortValue(searchGroup.sortValues[i],
field);
+ }
+ return searchGroup;
+}
+
+@Override
+protected Object serializeOneSearchGroup(SearchGroup
searchGroup, SearchGroupsFieldCommand command) {
Object[] convertedSortValues = new Object[searchGroup.sortValues.length];
for (int i = 0; i < searchGroup.sortValues.length; i++) {
Object sortValue = searchGroup.sortValues[i];
SchemaField field = command.getGroupSort().getSort()[i].getField() !=
null ?
searcher.getSchema().getFieldOrNull(command.getGroupSort().getSort()[i].getField())
: null;
convertedSortValues[i] =
ShardResultTransformerUtils.marshalSortValue(sortValue, field);
}
- SchemaField field =
searcher.getSchema().getFieldOrNull(command.getKey());
- String groupValue = searchGroup.groupValue != null ?
field.getType().indexedToReadable(searchGroup.groupValue, new
CharsRefBuilder()).toString() : null;
- result.add(groupValue, convertedSortValues);
+ return convertedSortValues;
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r295253612
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
> edit: a little further below in the class there's a
cmd.setSegmentTerminateEarly(false); // not supported, silently ignore any
segmentTerminateEarly flag for example
Checking for this and failing if it is true will modify the current
behavior - do you think we should fail?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r295252069
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
Done in 0563e6618a3c5f049388583cd558fc08af684db8
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r293902076
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,47 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ // check if prefix is a prefix of the array
+ private static boolean isPrefix(SortField[] prefix, SortField[] array){
Review comment:
Ah! I think so, let me try!
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r293828038
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
I like it, but what about `validate` being `void` and throwing a
`SolrException` in case of problems? in this case we can return the description
of the particular problem in the exception and we just call `validate` at the
end of the setup. what do you 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r293825303
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,47 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ // check if prefix is a prefix of the array
+ private static boolean isPrefix(SortField[] prefix, SortField[] array){
+if (prefix == null || array == null) return false;
+if (prefix.length > array.length) return false;
+// if we are here, prefix.length <= array.length
+for (int i = 0; i < prefix.length; i++){
+ // we check if the element at position i matches
+ if (! prefix[i].equals(array[i])){
+// if not is not a prefix
+return false;
+ }
+}
+//.. other it is
+return true;
+ }
+
+ private boolean allowSkipSecondGroupingStep(final SortSpec
withinGroupSpecification, final SortSpec groupSort, final boolean isReranking) {
+// Only possible if we only want one doc per group
+final int limit = withinGroupSpecification.getCount();
+final int offset = withinGroupSpecification.getOffset();
+if ( limit != 1 || offset != 0 ) {
+return false;
+}
+
+final SortField[] withinGroupSortFields =
withinGroupSpecification.getSort().getSort();
+final SortField[] groupSortFields = groupSort.getSort().getSort();
+
+// Within group sort must be the same as group sort because if we skip
second step no sorting within group will be done.
+if (! isPrefix(withinGroupSortFields, groupSortFields)) {
+System.out.println("Not prefix" +
Arrays.toString(withinGroupSortFields)+ " , "+Arrays.toString(groupSortFields)
);
Review comment:
Removed (95ed12a), thanks. I would just throw the exception from the caller.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r293823386
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,47 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ // check if prefix is a prefix of the array
+ private static boolean isPrefix(SortField[] prefix, SortField[] array){
Review comment:
Given the fact that it is used only here I would keep it private - and we
might put it into `ArrayUtil` when another class needs 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r293823441
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,47 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ // check if prefix is a prefix of the array
+ private static boolean isPrefix(SortField[] prefix, SortField[] array){
Review comment:
what do you 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290352433
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/GroupedEndResultTransformer.java
##
@@ -67,8 +67,14 @@ public void transform(Map result,
ResponseBuilder rb, SolrDocumentSou
for (GroupDocs group : topGroups.groups) {
SimpleOrderedMap groupResult = new SimpleOrderedMap<>();
if (group.groupValue != null) {
+final String groupValue;
+if (rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ groupValue =
groupField.getType().indexedToReadable(group.groupValue.utf8ToString());
Review comment:
added regression test for `group.main=true` and `group.format=simple`
in 60d60526af3
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290367448
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/GroupedEndResultTransformer.java
##
@@ -67,8 +67,14 @@ public void transform(Map result,
ResponseBuilder rb, SolrDocumentSou
for (GroupDocs group : topGroups.groups) {
SimpleOrderedMap groupResult = new SimpleOrderedMap<>();
if (group.groupValue != null) {
+final String groupValue;
+if (rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ groupValue =
groupField.getType().indexedToReadable(group.groupValue.utf8ToString());
Review comment:
wrt cpoerschke@ce5d9a9 , I wouldn't do that change - the check
`rb.getGroupingSpec().isSkipSecondGroupingStep()` is just moved into
`QueryComponent`, but it affects only the behavior in
`GroupedEndResultTransformer.transform` so I would keep it there..
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290356580
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -233,6 +233,41 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ private boolean allowSkipSecondGroupingStep(final GroupingSpecification
groupingSpec, final boolean isReranking ) {
+// Only possible if we only want one doc per group
+if (groupingSpec.getGroupLimit() != 1) {
Review comment:
Thanks, Applied in eca5af716517e
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290352433
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/GroupedEndResultTransformer.java
##
@@ -67,8 +67,14 @@ public void transform(Map result,
ResponseBuilder rb, SolrDocumentSou
for (GroupDocs group : topGroups.groups) {
SimpleOrderedMap groupResult = new SimpleOrderedMap<>();
if (group.groupValue != null) {
+final String groupValue;
+if (rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ groupValue =
groupField.getType().indexedToReadable(group.groupValue.utf8ToString());
Review comment:
add regression test for `group.main=true` and `group.format=simple`
in 60d60526af3
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290346022
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,118 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
Review comment:
I agree, I applied your change (removing `public` from
`TopGroupsShardResponseProcessor.fillResultIds` , package visibility is enough)
commit a7df0c0d8
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290245119
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SkipSecondStepSearchGroupShardResponseProcessor.java
##
@@ -0,0 +1,118 @@
+/*
+ * 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.grouping.distributed.responseprocessor;
+
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.lucene.search.Sort;
+import org.apache.lucene.search.TotalHits;
+import org.apache.lucene.search.grouping.GroupDocs;
+import org.apache.lucene.search.grouping.SearchGroup;
+import org.apache.lucene.search.grouping.TopGroups;
+import org.apache.lucene.util.BytesRef;
+import org.apache.solr.handler.component.ResponseBuilder;
+import org.apache.solr.handler.component.ShardDoc;
+import org.apache.solr.handler.component.ShardResponse;
+import org.apache.solr.search.SolrIndexSearcher;
+import org.apache.solr.search.grouping.GroupingSpecification;
+import
org.apache.solr.search.grouping.distributed.shardresultserializer.SearchGroupsResultTransformer;
+
+public class SkipSecondStepSearchGroupShardResponseProcessor extends
SearchGroupShardResponseProcessor {
+
+ @Override
+ protected SearchGroupsResultTransformer
newSearchGroupsResultTransformer(SolrIndexSearcher solrIndexSearcher) {
+return new
SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer(solrIndexSearcher);
+ }
+
+ @Override
+ protected SearchGroupsContainer newSearchGroupsContainer(ResponseBuilder rb)
{
+return new
SkipSecondStepSearchGroupsContainer(rb.getGroupingSpec().getFields());
+ }
+
+ protected static class SkipSecondStepSearchGroupsContainer extends
SearchGroupsContainer {
+
+private final Map docIdToShard = new HashMap<>();
+
+public SkipSecondStepSearchGroupsContainer(String[] fields) {
+ super(fields);
+}
+
+@Override
+public void addSearchGroups(ShardResponse srsp, String field,
Collection> searchGroups) {
+ super.addSearchGroups(srsp, field, searchGroups);
+ for (SearchGroup searchGroup : searchGroups) {
+assert(srsp.getShard() != null);
+docIdToShard.put(searchGroup.topDocSolrId, srsp.getShard());
+ }
+}
+
+@Override
+public void addMergedSearchGroups(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups ) {
+ // TODO: add comment or javadoc re: why this method is overridden as a
no-op
+}
+
+@Override
+public void addSearchGroupToShards(ResponseBuilder rb, String groupField,
Collection> mergedTopGroups) {
+ super.addSearchGroupToShards(rb, groupField, mergedTopGroups);
+
+ final GroupingSpecification groupingSpecification = rb.getGroupingSpec();
+ final Sort groupSort = groupingSpecification.getGroupSort();
+ final String[] fields = groupingSpecification.getFields();
+
+ GroupDocs[] groups = new GroupDocs[mergedTopGroups.size()];
+ Map resultsId = new HashMap<>(mergedTopGroups.size());
+
+ // This is the max score found in any document on any group
+ float maxScore = 0;
+ int index = 0;
+
+ for (SearchGroup group : mergedTopGroups) {
+maxScore = Math.max(maxScore, group.topDocScore);
+final String shard = docIdToShard.get(group.topDocSolrId);
+assert(shard != null);
+ShardDoc sdoc = new ShardDoc(group.topDocScore,
+fields,
Review comment:
Thanks, added in 72f559a0
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r290224198
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -232,6 +233,45 @@ public void prepare(ResponseBuilder rb) throws IOException
}
}
+ private boolean allowSkipSecondGroupingStep(final SortSpec
withinGroupSpecification, final SortSpec groupSort, final boolean isReranking) {
+// Only possible if we only want one doc per group
+final int limit = withinGroupSpecification.getCount();
+final int offset = withinGroupSpecification.getOffset();
+if ( limit != 1 || offset != 0 ) {
+log.error("group.skip.second.step=true is not compatible with
group.limit (limit={}) ", limit);
+return false;
+}
+
+// Within group sort must be the same as group sort because if we skip
second step no sorting within group will be done.
+if (withinGroupSpecification.getSort() != groupSort.getSort()) {
Review comment:
I agree, done in c5024d0
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r289842271
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
done in 725bfc
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r289842271
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
done in 725bfc8
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r289842271
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -288,6 +328,13 @@ protected void prepareGrouping(ResponseBuilder rb) throws
IOException {
groupingSpec.setMain(params.getBool(GroupParams.GROUP_MAIN, false));
groupingSpec.setNeedScore((rb.getFieldFlags() &
SolrIndexSearcher.GET_SCORES) != 0);
groupingSpec.setTruncateGroups(params.getBool(GroupParams.GROUP_TRUNCATE,
false));
+
+
groupingSpec.setSkipSecondGroupingStep(params.getBool(GroupParams.GROUP_SKIP_DISTRIBUTED_SECOND,
false));
+boolean isReranking = (rb.getRankQuery() != null);
+if (groupingSpec.isSkipSecondGroupingStep() &
!allowSkipSecondGroupingStep(groupingSpec.getWithinGroupSortSpec(),
groupingSpec.getGroupSortSpec(), isReranking)){
Review comment:
done in fc8c9c7
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch) URL: https://github.com/apache/lucene-solr/pull/300#discussion_r289836805 ## File path: solr/solrj/src/java/org/apache/solr/common/params/GroupParams.java ## @@ -66,5 +66,10 @@ public static final String GROUP_DISTRIBUTED_SECOND = GROUP + ".distributed.second"; public static final String GROUP_DISTRIBUTED_TOPGROUPS_PREFIX = GROUP + ".topgroups."; + + /** activates optimization in case only one document per group. + * Setting this to true is only compatible with group.limit = 1 + */ + public static final String GROUP_SKIP_DISTRIBUTED_SECOND = GROUP + ".skip.second.step"; Review comment: Done in c5553ed 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch) URL: https://github.com/apache/lucene-solr/pull/300#discussion_r289836805 ## File path: solr/solrj/src/java/org/apache/solr/common/params/GroupParams.java ## @@ -66,5 +66,10 @@ public static final String GROUP_DISTRIBUTED_SECOND = GROUP + ".distributed.second"; public static final String GROUP_DISTRIBUTED_TOPGROUPS_PREFIX = GROUP + ".topgroups."; + + /** activates optimization in case only one document per group. + * Setting this to true is only compatible with group.limit = 1 + */ + public static final String GROUP_SKIP_DISTRIBUTED_SECOND = GROUP + ".skip.second.step"; Review comment: Done in c5553ed2ee5 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: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r288569478
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -34,17 +41,37 @@
/**
* Implementation for transforming {@link SearchGroup} into a {@link
NamedList} structure and visa versa.
*/
-public class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
+public abstract class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
Review comment:
Thanks @cpoerschke I applied you commit (with minor changes) in
9480482166b05d04eb2bb55227baf4f3c6549ab9 and attempted a similar approach for
`transformToNative` in 0eb206bf6f635ba2e7514efa369e09137911b825. Now there is
no duplication, but I think we still need to revisit
`[Default|SkipSecondStep]SearchResultResultTransformer`... I'm not convinced
about SkipSecondStep inheriting and overriding `Default`
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r273634722
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -34,17 +41,37 @@
/**
* Implementation for transforming {@link SearchGroup} into a {@link
NamedList} structure and visa versa.
*/
-public class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
+public abstract class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
Review comment:
https://github.com/bloomberg/lucene-solr/pull/231 sketches the idea, what do
you 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r273444073
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -34,17 +41,37 @@
/**
* Implementation for transforming {@link SearchGroup} into a {@link
NamedList} structure and visa versa.
*/
-public class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
+public abstract class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
Review comment:
`SearchGroupsResultsTransformer` shouldn't change, it would just delegate
the behaviour to a different class according to the `skip` flag.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r271031492
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/SearchGroupsResultTransformer.java
##
@@ -34,17 +41,37 @@
/**
* Implementation for transforming {@link SearchGroup} into a {@link
NamedList} structure and visa versa.
*/
-public class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
+public abstract class SearchGroupsResultTransformer implements
ShardResultTransformer, Map> {
Review comment:
It looks a lot better, I merged it with the previous commit.
One comment: instead of having `SearchGroupsResultTransformer` implementing
the default behavior and SkipSecondStepSearchResultResultTransformer, I would
compose the behavior: One class for the `default` behaviur`, and one for the
`skipsStep` behavior. I'll try to sketch it in a PR to this branch.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r271027782
##
File path:
solr/core/src/java/org/apache/solr/search/grouping/distributed/responseprocessor/SearchGroupShardResponseProcessor.java
##
@@ -143,12 +151,61 @@ public void process(ResponseBuilder rb, ShardRequest
shardRequest) {
if (mergedTopGroups == null) {
continue;
}
-
- rb.mergedSearchGroups.put(groupField, mergedTopGroups);
- for (SearchGroup mergedTopGroup : mergedTopGroups) {
-rb.searchGroupToShards.get(groupField).put(mergedTopGroup,
tempSearchGroupToShards.get(groupField).get(mergedTopGroup));
+ if (rb.getGroupingSpec().isSkipSecondGroupingStep()){
+ /* If we are skipping the second grouping step we want to translate
the response of the
+ * first step in the response of the second step and send it to the
get_fields step.
+ */
+ processSkipSecondGroupingStep(rb, mergedTopGroups,
tempSearchGroupToShards, docIdToShard, groupSort, fields, groupField);
Review comment:
Thanks @cpoerschke for the analysis. Please find the answers:
(1) I agree, allocation can be avoided.
(2) I agree, I'll see if it is possible to reduce or wrap them in a private
object
(3) `rb.mergedSearchGroups`
It is used only to generate the second group request
https://github.com/apache/lucene-solr/blob/1d85cd783863f75cea133fb9c452302214165a4d/solr/core/src/java/org/apache/solr/search/grouping/distributed/requestfactory/TopGroupsShardRequestFactory.java#L114
... That we are going to skip if we apply the skip second step:
https://github.com/bloomberg/lucene-solr/blob/SOLR-11831/solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java#L573
(4-7b) Yes
(10, 11) I Merged your changes, and I'll check that the tests are still
fine, and that precommit is still happy
Thanks
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip second grouping step if group.limit is 1 (aka Las Vegas Patch)
diegoceccarelli commented on a change in pull request #300: SOLR-11831: Skip
second grouping step if group.limit is 1 (aka Las Vegas Patch)
URL: https://github.com/apache/lucene-solr/pull/300#discussion_r267291396
##
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryComponent.java
##
@@ -532,6 +578,12 @@ protected int groupedDistributedProcess(ResponseBuilder
rb) {
nextStage = ResponseBuilder.STAGE_DONE;
}
+if (rb.stage == ResponseBuilder.STAGE_EXECUTE_QUERY &&
rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ shardRequestFactory = new StoredFieldsShardRequestFactory();
+ nextStage = ResponseBuilder.STAGE_DONE;
+ rb.stage = ResponseBuilder.STAGE_GET_FIELDS;
Review comment:
Thanks @cpoerschke, I agree that we should skip stages in the middle, I
merged your draft and later and I'm going to rerun the tests to see if it still
works as intended or not.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org
