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_r269179855
########## 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<List<Command>, Map<String, SearchGroupsFieldCommandResult>> { +public abstract class SearchGroupsResultTransformer implements ShardResultTransformer<List<Command>, Map<String, SearchGroupsFieldCommandResult>> { Review comment: An existing public class becoming `abstract` here 'jumped out' at me a little since it could be considered to be breaking backwards compatibility? The https://github.com/cpoerschke/lucene-solr/commit/7e47e01ba27d3cd39daa288e60c61316b1738d67 commit explores removal/replacement of the `SearchGroupsResultTransformer.getInstance` wrapper and a next step could be for the proposed `SearchGroupsResultTransformer.SkipSecondStepSearchResultResultTransformer` inner class to become a `SkipSecondStepSearchResultResultTransformer.java` class of its own and the proposed `SearchGroupsResultTransformer.DefaultSearchResultResultTransformer` inner class to _not_ be created i.e. `SearchGroupsResultTransformer` here to not become abstract and its constructor to remain public. This of course assumes that `SearchGroupsResultTransformer` would require only simple changes to make it extensible and that then `SkipSecondStepSearchResultResultTransformer` could extend it. (Note that the https://github.com/cpoerschke/lucene-solr/commit/7e47e01ba27d3cd39daa288e60c61316b1738d67 commit is on https://github.com/cpoerschke/lucene-solr/tree/github-bloomberg-SOLR-11831-cpoerschke-5 branch and that branch includes https://github.com/cpoerschke/lucene-solr/tree/github-bloomberg-SOLR-11831-cpoerschke-4 changes for convenience only i.e. not out of necessity.) 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