[GitHub] [incubator-pinot] fx19880617 commented on a change in pull request #5858: Change group key delimiter from '\t' to '\0'

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

fx19880617 commented on a change in pull request #5858:
URL: https://github.com/apache/incubator-pinot/pull/5858#discussion_r471831666



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/operator/combine/GroupByOrderByCombineOperator.java
##########
@@ -155,33 +155,53 @@ public void runJob() {
             // Merge aggregation group-by result.
             AggregationGroupByResult aggregationGroupByResult = intermediateResultsBlock.getAggregationGroupByResult();
             if (aggregationGroupByResult != null) {
-              // Get converter functions
-              Function[] converterFunctions = new Function[numGroupByExpressions];
-              for (int i = 0; i < numGroupByExpressions; i++) {
-                converterFunctions[i] = getConverterFunction(_dataSchema.getColumnDataType(i));
-              }
+              if (numGroupByExpressions == 1) {

Review comment:
       why we separate single groupby ? for performance ?

##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/groupby/AggregationGroupByTrimmingService.java
##########
@@ -29,34 +28,39 @@
 import java.util.Map;
 import java.util.PriorityQueue;
 import java.util.TreeMap;
-import javax.annotation.Nonnull;
 import org.apache.commons.collections.comparators.ComparableComparator;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.tuple.ImmutablePair;
 import org.apache.pinot.common.response.broker.GroupByResult;
 import org.apache.pinot.core.query.aggregation.function.AggregationFunction;
 import org.apache.pinot.core.query.aggregation.function.AggregationFunctionUtils;
 import org.apache.pinot.core.query.aggregation.function.MinAggregationFunction;
+import org.apache.pinot.core.query.request.context.ExpressionContext;
+import org.apache.pinot.core.query.request.context.QueryContext;
 import org.apache.pinot.core.util.GroupByUtils;
 
 
 /**
  * The <code>AggregationGroupByTrimmingService</code> class provides trimming service for aggregation group-by queries.
  */
+@SuppressWarnings({"rawtypes", "unchecked"})
 public class AggregationGroupByTrimmingService {
-
   private final AggregationFunction[] _aggregationFunctions;
-  private final int _groupByTopN;
+  private final int _numGroupByExpressions;
+  private final int _limit;
   private final int _trimSize;
   private final int _trimThreshold;
 
-  public AggregationGroupByTrimmingService(@Nonnull AggregationFunction[] aggregationFunctions, int groupByTopN) {
-    Preconditions.checkArgument(groupByTopN > 0);
-
-    _aggregationFunctions = aggregationFunctions;
-    _groupByTopN = groupByTopN;
+  public AggregationGroupByTrimmingService(QueryContext queryContext) {
+    _aggregationFunctions = queryContext.getAggregationFunctions();
+    List<ExpressionContext> groupByExpressions = queryContext.getGroupByExpressions();
+    assert groupByExpressions != null;

Review comment:
       shall we stick to `Preconditions.checkArgument`?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org