You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2020/08/13 07:24:17 UTC

[GitHub] [incubator-pinot] Jackie-Jiang opened a new pull request #5858: Change group key delimiter from '\t' to '\0'

Jackie-Jiang opened a new pull request #5858:
URL: https://github.com/apache/incubator-pinot/pull/5858


   ## Description
   Using '\t' as the group key delimiter will cause unexpected behavior when grouping on string value with '\t' inside. To solve it, we can use '\0' as the delimiter which is not allowed in the string values.
   
   - Add code to handle both '\0' and '\t' as limiter on broker side for backward-compatibility
   - Specialize case of grouping on single expression to save the redundant group key splits.
   
   ## Upgrade Notes
   In order to achieve zero down-time upgrade, Brokers must be upgraded before Servers.


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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5858:
URL: https://github.com/apache/incubator-pinot/pull/5858#discussion_r471835607



##########
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:
       Yes. For single group-by, we don't need to split the string for each group key




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

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

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on a change in pull request #5858:
URL: https://github.com/apache/incubator-pinot/pull/5858#discussion_r471838488



##########
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:
       I usually use `assert` to indicate that it is not possible to be null for readability, which will be ignored in production environment.
   `Preconditions` are used for catching the illegal user input and throw exceptions




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

[GitHub] [incubator-pinot] Jackie-Jiang merged pull request #5858: Change group key delimiter from '\t' to '\0'

Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged pull request #5858:
URL: https://github.com/apache/incubator-pinot/pull/5858


   


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