yashmayya commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4381245630
##########
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:
`ARRAY_AGG(DISTINCT x)` now returns wrong results with no error. The call
keeps the DISTINCT flag but gets no `isDistinct` argument. Then
`PinotAggregateExchangeNodeInsertRule.buildAggCall` (and
`AggregatePushdownRule` in v2) drops DISTINCT for every function except COUNT
and LISTAGG.
Example over {1, 2, 2, 3}: `SELECT CARDINALITY(ARRAY_AGG(DISTINCT int_col))
FROM tbl` returns 4, not 3. Before this PR the query failed on the server. SSE
rejects it (`Function 'ARRAY_AGG' on DISTINCT is not supported`).
Suggestion: map DISTINCT to the `isDistinct` argument, or reject it like
SSE. Please also add a test for the DISTINCT keyword.
```java
if (isArrayAgg(aggCall)) {
argList.add(1, literalIndex);
if (aggCall.isDistinct() && argList.size() == 2) {
projects.add(rexBuilder.makeLiteral(true));
argList.add(projects.size() - 1);
aggCall = aggCall.withDistinct(false);
}
}
```
The explicit form `ARRAY_AGG(DISTINCT x, 'INT')` drops DISTINCT on master
too, but this PR makes the bug much easier to hit.
##########
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:
For DECIMAL with scale > 0 this returns `BIG_DECIMAL`, but the plan types it
as DOUBLE_ARRAY (`resolveDecimal`), and the leaf computes DOUBLE.
`getBigDecimalValuesSV()` then calls `BigDecimal.valueOf(double)`, which throws
on NaN or Infinity. One such row fails the whole query.
Repro with rows `(num=1, den=1)` and `(num=2, den=0)`:
```sql
SELECT CARDINALITY(ARRAY_AGG(num / (den * 1.0))) FROM t
```
This fails with `Infinite or NaN`. GROUP BY, distinct and skip-leaf fail
too. `ARRAY_AGG(num / (den * 1.0), 'DOUBLE')` works.
It is also slow: each value becomes a boxed BigDecimal, and at the end the
list is converted back to `double[]`.
Suggestion: for ARRAY_AGG, return `"DOUBLE"` for every DECIMAL that is not
the scale-0 INT/LONG case. This matches the DOUBLE_ARRAY plan type.
##########
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:
The no-type `ARRAY_AGG(CASE WHEN c THEN x END)` fails at runtime, but the
explicit form works. `AGGREGATE_CASE_TO_FILTER` runs first. It only handles
1-arg calls, so it changes the call to `ARRAY_AGG(x) FILTER (WHERE c)`. The
element nullability changes, so Calcite adds a CAST to a NOT NULL array type
above the final aggregate. The runtime CAST rejects arrays.
Example (null handling off): `SELECT CARDINALITY(ARRAY_AGG(CASE WHEN int_col
> 1 THEN int_col END)) FROM tbl` fails with `class [I must not be an array
type`.
Suggestion: also add this rule once before the basic rules, and keep this
instance. In a local test this fixed the CASE queries, and the plan tests did
not change.
--
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]
