xiangfu0 commented on code in PR #19991:
URL: https://github.com/apache/pinot/pull/19991#discussion_r4383698595
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/AggregateFunctionRewriteOptimizer.java:
##########
@@ -134,6 +139,31 @@ private static void addDataTypeArgument(Function function,
String functionName,
functionName, fieldSpec.isSingleValueField() ? "" : "multi-value ",
fieldSpec.getDataType()));
}
+ /// Appends the default tie reducer when missing, then the data column's
logical type when it is non-numeric.
+ private static void addModeDataTypeArgument(Function function, FieldSpec
fieldSpec) {
+ if (!fieldSpec.isSingleValueField()) {
+ return;
+ }
+ String dataType;
+ switch (fieldSpec.getDataType()) {
+ case STRING:
+ case BOOLEAN:
Review Comment:
Restricted automatic MODE rewriting to STRING/JSON; BOOLEAN/TIMESTAMP retain
their legacy DOUBLE path, AVG support and expression semantics unless the
explicit third type argument is supplied. Added optimizer and end-to-end HAVING
regression coverage.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Removed the rolling-upgrade break for existing BOOLEAN/TIMESTAMP calls: only
STRING/JSON are automatically rewritten, while typed BOOLEAN/TIMESTAMP require
an explicit third argument.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
