Jackie-Jiang commented on code in PR #17109:
URL: https://github.com/apache/pinot/pull/17109#discussion_r2566517289
##########
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:
Is this used?
Should we introduce this change (also MIN and MAX) along with other broker
side changes?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/aggregator/AvgValueAggregator.java:
##########
@@ -40,23 +40,13 @@ public DataType getAggregatedValueType() {
@Override
public AvgPair getInitialAggregatedValue(@Nullable Object rawValue) {
- if (rawValue == null) {
- return new AvgPair();
- }
- if (rawValue instanceof byte[]) {
- return deserializeAggregatedValue((byte[]) rawValue);
- } else {
- return new AvgPair(ValueAggregatorUtils.toDouble(rawValue), 1L);
- }
+ return processRawValue(rawValue);
Review Comment:
(minor) Keep the `null` check here, as we don't need `null` check in
`applyRawValue()`. Same for other places
##########
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:
Should we support MV for all following functions?
For `COUNT`, I'm not sure if always rewriting it to `COUNT_MV` is desired.
`COUNT` could be used to find only the non-null values
--
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]
