xiangfu0 commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4381353039
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -103,11 +106,16 @@ public void onMatch(RelOptRuleCall call) {
aggCalls.add(aggCall);
continue;
}
- List<Integer> argList = new ArrayList<>(aggCall.getArgList());
- argList.add(literalIndexByDataType.computeIfAbsent(dataType, type -> {
+ int literalIndex = literalIndexByDataType.computeIfAbsent(dataType, type
-> {
projects.add(rexBuilder.makeLiteral(type));
return projects.size() - 1;
- }));
+ });
+ List<Integer> argList = new ArrayList<>(aggCall.getArgList());
+ if (isArrayAgg(aggCall)) {
+ argList.add(1, literalIndex);
Review Comment:
Done in c01facda12: `ARRAY_AGG(DISTINCT x)` now becomes `ARRAY_AGG(x,
'TYPE', true)` with the DISTINCT flag cleared. Plan and runtime tests cover it,
including a multi-value column; the explicit `ARRAY_AGG(DISTINCT x, 'INT')` is
unchanged from master.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -143,6 +171,12 @@ private static String getDataTypeArgument(AggregateCall
aggCall, RelNode input)
case DOUBLE:
return "DOUBLE";
case DECIMAL:
+ // The planner types an array of DECIMAL with scale 0 as INT_ARRAY or
LONG_ARRAY, see
+ // RelToPlanNodeConverter.resolveDecimal
+ if (name.equals(AggregationFunctionType.ARRAYAGG.name()) &&
dataType.getScale() == 0
Review Comment:
Done in c01facda12: ARRAY_AGG now maps every DECIMAL outside the scale-0
INT/LONG cases to `'DOUBLE'`, matching the DOUBLE_ARRAY plan type, and your
divide-by-zero case is in `UDFAggregates.json`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -605,7 +605,8 @@ private static HepProgram getOptProgram(PinotRuleSet
ruleSet, Set<String> skipRu
for (RelOptRule relOptRule : basicRules) {
hepProgramBuilder.addRuleInstance(relOptRule);
}
- // Required to plan FIRST_WITH_TIME / LAST_WITH_TIME without a data type,
so the rule options do not filter it.
+ // Required to plan FIRST_WITH_TIME, LAST_WITH_TIME and ARRAY_AGG without
a data type, so the rule options do not
+ // filter it.
hepProgramBuilder.addRuleInstance(PinotAggregateTypeArgumentRewriteRule.INSTANCE);
Review Comment:
Done in c01facda12: the rule also runs once before the basic rules, so
`AggregateCaseToFilter` leaves `ARRAY_AGG(CASE WHEN ...)` alone. A plan test
and a runtime test comparing the no-type and explicit forms cover it, and no
other plan changed.
_🤖 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]
