[GitHub] [incubator-pinot] bkuang88 commented on a change in pull request #5832: Added set-diff operators and changed distinctCountThetaSketch syntax

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

bkuang88 commented on a change in pull request #5832:
URL: https://github.com/apache/incubator-pinot/pull/5832#discussion_r467343288



##########
File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java
##########
@@ -108,35 +136,24 @@ public DistinctCountThetaSketchAggregationFunction(List<ExpressionContext> argum
     Preconditions.checkArgument(paramsExpression.getType() == ExpressionContext.Type.LITERAL,
         "Last argument of DistinctCountThetaSketch must be literal (post-aggregation expression)");
     _postAggregationExpression = QueryContextConverterUtils
-        .getFilter(CalciteSqlParser.compileToExpression(postAggregationExpression.getLiteral()));
+        .getExpression(CalciteSqlParser.compileToExpression(postAggregationExpression.getLiteral()));
 
     // Initialize the predicate map
     _predicateInfoMap = new HashMap<>();
-    if (numArguments > 3) {
-      // Predicates are explicitly specified
-      for (int i = 2; i < numArguments - 1; i++) {
-        ExpressionContext predicateExpression = arguments.get(i);
-        Preconditions.checkArgument(predicateExpression.getType() == ExpressionContext.Type.LITERAL,
-            "Third to second last argument of DistinctCountThetaSketch must be literal (predicate expression)");
-        Predicate predicate = getPredicate(predicateExpression.getLiteral());
-        _inputExpressions.add(predicate.getLhs());
-        _predicateInfoMap.put(predicate, new PredicateInfo(predicate));
-      }
-    } else {
-      // Auto-derive predicates from the post-aggregation expression
-      Stack<FilterContext> stack = new Stack<>();
-      stack.push(_postAggregationExpression);
-      while (!stack.isEmpty()) {
-        FilterContext filter = stack.pop();
-        if (filter.getType() == FilterContext.Type.PREDICATE) {
-          Predicate predicate = filter.getPredicate();
-          _inputExpressions.add(predicate.getLhs());
-          _predicateInfoMap.put(predicate, new PredicateInfo(predicate));
-        } else {
-          stack.addAll(filter.getChildren());
-        }
-      }
+
+    // Predicates are explicitly specified

Review comment:
       Hmm I've thought about it, but if we auto-derive like before, we'd only have a subset of features. We won't be able to support SET_DIFF at all if go with that approach right?
   
   Usually, I would think that if there are 2 equivalent ways of writing something, the feature set would be the same. But it seems like only a subset of features would be available if we go with that route.
   
   I'm personally okay with both approach but wanted to point that out.




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