cpoerschke 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_r272643470
##########
File path:
solr/core/src/java/org/apache/solr/search/grouping/endresulttransformer/GroupedEndResultTransformer.java
##########
@@ -67,8 +67,14 @@ public void transform(Map<String, ?> result,
ResponseBuilder rb, SolrDocumentSou
for (GroupDocs<BytesRef> group : topGroups.groups) {
SimpleOrderedMap<Object> groupResult = new SimpleOrderedMap<>();
if (group.groupValue != null) {
+ final String groupValue;
+ if (rb.getGroupingSpec().isSkipSecondGroupingStep()) {
+ groupValue =
groupField.getType().indexedToReadable(group.groupValue.utf8ToString());
Review comment:
Q: So on this bit here I was wondering why the `indexedToReadable` is needed
in the `isSkipSecondGroupingStep` case.
A: The
[TopGroupsResultTransformer](https://github.com/apache/lucene-solr/blob/releases/lucene-solr/8.0.0/solr/core/src/java/org/apache/solr/search/grouping/distributed/shardresultserializer/TopGroupsResultTransformer.java#L235)
which would be called in the `not-isSkipSecondGroupingStep` case has it, just
the `indexedToReadable` that is. And the
`groupFieldType.toObject(groupField.createField(...))` wrap in this method is
needed to 'have' e.g. the name part of the group field i.e. not just the group
value stored in the `groupResult` map. Something along those lines.
Anyhow, then I wondered about whether or not the
`GroupedEndResultTransformer` "needs to know" about the
`GroupingSpecification.isSkipSecondGroupingStep` logic.
https://github.com/cpoerschke/lucene-solr/commit/ce5d9a99207e376cb7e09dd118c51bca5e90aee7
commit sketches how the "knowing" might be avoided, though I'm not entirely
convinced by the approach for three reasons:
* some difficulty in explaining (as evidenced by lack of javadocs) what the
new `GroupedEndResultTransformer.groupValueToString` method really does and why
it's beneficial,
* the realisation that `GroupedEndResultTransformer` already "knows" about
`GroupingSpecification` itself i.e. there are two
`docList.setStart(rb.getGroupingSpec().getWithinGroupOffset());` calls, and
* the observation that after the change all
`GroupingSpecification.isSkipSecondGroupingStep` callers are in
`QueryComponent` which raises a "is it a grouping parameter or a query
component parameter?" question.
Either way, an interesting question arose from the exercise:
* Making the `QueryComponent` change in
https://github.com/cpoerschke/lucene-solr/commit/ce5d9a99207e376cb7e09dd118c51bca5e90aee7
brought attention to the `group.main=true` and `group.format=simple` [Grouping
Parameters](https://lucene.apache.org/solr/guide/7_7/result-grouping.html#grouping-parameters)
code paths.
* The `EndResultTransformer` code for those parameters is apparently not
affected, and intuitively it would seem that it need not be (since the group
value is not(?) included in the output) whilst still benefitting from the
`isSkipSecondGroupingStep` logic. Though it might be nice to investigate that
further and perhaps add test coverage for it?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
