yashmayya commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4382803400
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateTypeArgumentRewriteRule.java:
##########
@@ -62,10 +69,11 @@ private
PinotAggregateTypeArgumentRewriteRule(RelBuilderFactory factory) {
@Override
public boolean matches(RelOptRuleCall call) {
- // Requiring a call without its data type is also what makes the rule
terminate.
+ // Requiring a call without its data type or with the DISTINCT flag is
also what makes the rule terminate.
Aggregate aggRel = call.rel(0);
RelNode input = aggRel.getInput();
- return aggRel.getAggCallList().stream().anyMatch(aggCall ->
getDataTypeArgument(aggCall, input) != null);
+ return aggRel.getAggCallList().stream()
+ .anyMatch(aggCall -> getDataTypeArgument(aggCall, input) != null ||
isDistinctArrayAgg(aggCall));
Review Comment:
Regression: an explicit DISTINCT ARRAY_AGG directly over UNION ALL works on
master and fails here.
```sql
SELECT ARRAY_AGG(DISTINCT x, 'INT', true)
FROM (SELECT int_col x FROM t UNION ALL SELECT int_col FROM t2)
```
This fails with `ARRAY_AGG expects the 2nd argument to be literal, got:
IDENTIFIER` in v1, v2 and lite mode. `ARRAY_AGG(DISTINCT x, 'INT')` fails the
same way, and so do other literal-argument calls in the same aggregate, for
example `PERCENTILE(x, 50)`.
Cause: `ProjectSetOpTransposeRule` pushes the `'INT'` literal into the union
branches, where `findImmediateProjects` can still find it through the `Union`.
On master this rule does not fire for explicit-type calls. Now it fires for
DISTINCT, and it puts an identity project over the `Union`, which hides the
literal (the plan shows `ARRAYAGG($0, $1, true)`).
Suggestion: move the "DISTINCT -> isDistinct = true" step into
`buildAggCall` in `PinotAggregateExchangeNodeInsertRule` and
`AggregatePushdownRule`, next to the LISTAGG DISTINCT case, and drop the
DISTINCT trigger from `matches()`. Then this rule does not touch explicit-type
calls, as on master. Please add this query as a test.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -603,9 +603,15 @@ private static HepProgram getOptProgram(PinotRuleSet
ruleSet, Set<String> skipRu
// Run the Calcite CORE rules using 1 HepInstruction per rule. We use 1
HepInstruction per rule for simplicity:
// the rules used here can rest assured that they are the only ones
evaluated in a dedicated graph-traversal.
for (RelOptRule relOptRule : basicRules) {
+ // Supplies the data type argument before AggregateCaseToFilter, which
rewrites only one-argument calls: its
+ // ARRAY_AGG(expression) FILTER (WHERE condition) form of ARRAY_AGG(CASE
WHEN ...) fails at runtime.
+ if
(relOptRule.toString().equals(CommonConstants.Broker.PlannerRuleNames.AGGREGATE_CASE_TO_FILTER))
{
+
hepProgramBuilder.addRuleInstance(PinotAggregateTypeArgumentRewriteRule.INSTANCE);
Review Comment:
Minor, non-default config only: with approximate-function params set on the
broker and `skipPlannerRules='ProjectMerge'`, a no-type FIRST_WITH_TIME next to
`DISTINCTCOUNT` or `PERCENTILE` now plans as `FIRSTWITHTIME($1, $2, $4)`, with
the type literal not inlined. On master it was `'INT'`. Here
`PinotApproximateAggregateRewriteRule` runs after the early instance and stacks
its own project on top, and only `ProjectMerge` flattens the two. A fix is to
let the approximate rule append to an existing immediate project, like this
rule does. I checked this at plan level only.
--
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]
