yashmayya commented on code in PR #17109:
URL: https://github.com/apache/pinot/pull/17109#discussion_r2566567965
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -467,4 +481,83 @@ public boolean allowsFraming() {
return false;
}
}
+
+ /// Pinot's custom SUM aggregation function that can aggregate on SV or MV
numeric inputs. We can't simply override
+ /// the return type inference and operand type checker in {@link
AggregationFunctionType} like we do for other
+ /// functions because {@link org.apache.calcite.sql.fun.SqlSumAggFunction}
has some customizations that we need to
+ /// retain here to ensure that rules like {@link
org.apache.calcite.rel.rules.AggregateRemoveRule} work as expected.
+ /// TODO: Replace {@link SqlStdOperatorTable#SUM} with this instance after
the next release (there's a dependency
+ /// on AVG window function, see https://github.com/apache/pinot/pull/17109).
+ private static final class PinotSumFunction extends PinotSqlAggFunction {
Review Comment:
Yeah it'll be used when we make the broker side changes for `SUM` and `AVG`.
`MIN` / `MAX` are already being used though, and there shouldn't be any
backward compatible issues - we're not using the `SUM` right now due to the
chained side effect for `AVG` -
https://github.com/apache/pinot/pull/17109#discussion_r2558169830.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -140,6 +140,8 @@ public abstract class BaseSingleStageBrokerRequestHandler
extends BaseBrokerRequ
private static final Expression TRUE =
RequestUtils.getLiteralExpression(true);
private static final Expression STAR =
RequestUtils.getIdentifierExpression("*");
private static final int
MAX_UNAVAILABLE_SEGMENTS_TO_PRINT_IN_QUERY_EXCEPTION = 10;
+ // TODO: After the next release, remove overrides here for consolidated
aggregation functions
+ // (see https://github.com/apache/pinot/issues/17061)
Review Comment:
Yes, I think eventually we should consolidate all of these. I'll make more
changes in a subsequent PR, this one was just supposed to be the first version
with the most popular aggregations. I intentionally omitted `COUNT` here for
the same reason.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
