bkuang88 commented on a change in pull request #5832: URL: https://github.com/apache/incubator-pinot/pull/5832#discussion_r468138173
########## File path: pinot-core/src/main/java/org/apache/pinot/core/query/aggregation/function/DistinctCountThetaSketchAggregationFunction.java ########## @@ -526,28 +543,51 @@ private Predicate getPredicate(String predicateString) { * passed to this method to be used when evaluating the expression. * * @param postAggregationExpression Post-aggregation expression to evaluate (modeled as a filter) + * @param expressions list of aggregation function parameters * @param sketchMap Precomputed sketches for predicates that are part of the expression. * @return Overall evaluated sketch for the expression. */ - private Sketch evalPostAggregationExpression(FilterContext postAggregationExpression, - Map<Predicate, Sketch> sketchMap) { - switch (postAggregationExpression.getType()) { - case AND: - Intersection intersection = _setOperationBuilder.buildIntersection(); - for (FilterContext child : postAggregationExpression.getChildren()) { - intersection.update(evalPostAggregationExpression(child, sketchMap)); - } - return intersection.getResult(); - case OR: - Union union = _setOperationBuilder.buildUnion(); - for (FilterContext child : postAggregationExpression.getChildren()) { - union.update(evalPostAggregationExpression(child, sketchMap)); + private Sketch evalPostAggregationExpression( + final ExpressionContext postAggregationExpression, + final List<Predicate> expressions, + final Map<Predicate, Sketch> sketchMap) { + if (postAggregationExpression.getType() == ExpressionContext.Type.LITERAL) { + throw new IllegalArgumentException("Literal not supported in post-aggregation function"); + } + + if (postAggregationExpression.getType() == ExpressionContext.Type.IDENTIFIER) { + final Predicate exp = + expressions.get(extractSubstitutionPosition(postAggregationExpression.getLiteral()) - 1); + return sketchMap.get(exp); + } + + // shouldn't throw exception because of the validation in the constructor + final MergeFunction func = + MergeFunction.valueOf(postAggregationExpression.getFunction().getFunctionName().toUpperCase()); + + // handle functions recursively + switch(func) { + case SET_UNION: + final Union union = _setOperationBuilder.buildUnion(); + for (final ExpressionContext exp : postAggregationExpression.getFunction().getArguments()) { + union.update(evalPostAggregationExpression(exp, expressions, sketchMap)); } return union.getResult(); - case PREDICATE: - return sketchMap.get(postAggregationExpression.getPredicate()); + case SET_INTERSECT: + final Intersection intersection = _setOperationBuilder.buildIntersection(); + for (final ExpressionContext exp : postAggregationExpression.getFunction().getArguments()) { + intersection.update(evalPostAggregationExpression(exp, expressions, sketchMap)); + } + return intersection.getResult(); + case SET_DIFF: + final List<ExpressionContext> args = postAggregationExpression.getFunction().getArguments(); + final AnotB diff = _setOperationBuilder.buildANotB(); + final Sketch a = evalPostAggregationExpression(args.get(0), expressions, sketchMap); + final Sketch b = evalPostAggregationExpression(args.get(1), expressions, sketchMap); + diff.update(a, b); Review comment: Good call. Done - updated in the release notes in the PR. ---------------------------------------------------------------- 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 --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org