xiangfu0 commented on code in PR #19991:
URL: https://github.com/apache/pinot/pull/19991#discussion_r4366208976
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/AggregationFunctionType.java:
##########
@@ -428,6 +428,15 @@ public static AggregationFunctionType
getAggregationFunctionType(String function
}
}
+ /// Returns DOUBLE, or the type of the data argument for the explicit data
type form MODE(expr, reducer, dataType).
+ private static class ModeFinalReturnTypeInference implements
SqlReturnTypeInference {
+ @Override
+ public RelDataType inferReturnType(SqlOperatorBinding opBinding) {
+ return opBinding.getOperandCount() == 3 ?
ReturnTypes.ARG0.inferReturnType(opBinding)
+ : opBinding.getTypeFactory().createSqlType(SqlTypeName.DOUBLE);
+ }
Review Comment:
Small readability nit: this returns DOUBLE for the 1-arg form, which is what
the leaf intermediate stage uses, while the final client type for
BOOLEAN/TIMESTAMP comes from Calcite's ARG0 MODE at the FINAL stage. That
interplay is subtle — a one-line comment here pointing at where the real final
type is resolved would save the next reader a trace.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -116,20 +119,31 @@ public void onMatch(RelOptRuleCall call) {
aggRel.copy(aggRel.getTraitSet(), newInput, aggRel.getGroupSet(),
aggRel.getGroupSets(), aggCalls));
}
- /// Returns the data type argument to append, or `null` when the call
already has one or is not such a call. Throws
- /// for a data argument without an explicit form, such as an array from a
multi-value column.
+ /// Returns the literal arguments to append, or `null` when the call already
has them or is not such a call.
@Nullable
- private static String getDataTypeArgument(AggregateCall aggCall, RelNode
input) {
- if (aggCall.getArgList().size() != 2) {
- return null;
- }
+ private static List<String> getLiteralArguments(AggregateCall aggCall,
RelNode input) {
+ List<Integer> argList = aggCall.getArgList();
String name =
AggregationFunctionType.getNormalizedAggregationFunctionName(aggCall.getAggregation().getName());
- if (!name.equals(AggregationFunctionType.FIRSTWITHTIME.name())
- && !name.equals(AggregationFunctionType.LASTWITHTIME.name())) {
- return null;
+ if (name.equals(AggregationFunctionType.MODE.name())) {
+ // BOOLEAN and TIMESTAMP already return their own type, cast from the
DOUBLE result. The leaf stage may still
+ // rewrite them in the single-stage optimizer, which keeps the same
INT/LONG count maps.
+ return argList.size() == 1 &&
SqlTypeName.CHAR_TYPES.contains(getSqlTypeName(input, argList.get(0)))
+ ? List.of("MIN", "STRING") : null;
}
- SqlTypeName dataType =
-
input.getRowType().getFieldList().get(aggCall.getArgList().get(0)).getType().getSqlTypeName();
+ if (argList.size() == 2 &&
(name.equals(AggregationFunctionType.FIRSTWITHTIME.name())
+ || name.equals(AggregationFunctionType.LASTWITHTIME.name()))) {
+ return List.of(getDataTypeArgument(name, getSqlTypeName(input,
argList.get(0))));
Review Comment:
Nice that the single-stage path is covered by the optimizer. Worth noting
the known rolling-upgrade gap you called out in the description: a pre-patch
server rejects the rewritten 3-arg MODE. Same pattern as FIRST_WITH_TIME, so
just confirming it's an accepted trade-off here.
--
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]
