[GitHub] [ignite] korlov42 commented on a change in pull request #9829: IGNITE-16396 Allow hash output distribution for aggregations and set ops

[GitBox <gi...@apache.org>]

korlov42 commented on a change in pull request #9829:
URL: https://github.com/apache/ignite/pull/9829#discussion_r817477211



##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/SortAggregateConverterRule.java
##########
@@ -51,10 +51,10 @@ private SortAggregateConverterRule() {
     }
 
     /** */
-    private static class SortSingleAggregateConverterRule extends AbstractIgniteConverterRule<LogicalAggregate> {
+    private static class SortColocatedAggregateConverterRule extends AbstractIgniteConverterRule<LogicalAggregate> {

Review comment:
       Currently we have:
   - IgniteColocatedHashAggregate
   - IgniteColocatedSortAggregate
   - IgniteMapHashAggregate
   - IgniteMapSortAggregate
   - IgniteReduceHashAggregate
   - IgniteReduceSortAggregate
   
   The pattern is `<some prefix><type of distribution><aggregation strategy (if I may say so)>Aggregate`. But parts `distribution` and `strategy` are switched in the convertors' names:
   - SortColocatedAggregateConverterRule
   - SortMapReduceAggregateConverterRule
   - HashColocatedAggregateConverterRule
   - HashMapReduceAggregateConverterRule
   
   Probably, it would be better to rename converters to match the relations' class names. WDYT?




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org