yashmayya commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4393063450
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java:
##########
@@ -85,6 +89,45 @@ public static RelNode unboxRel(RelNode rel) {
}
}
+ /// Appends aggregate literal arguments without hiding literals already
projected beneath a union. If a union has
+ /// no projected literals, keeps one project above it; otherwise recurses
through nested unions to their projects.
+ public static RelNode appendAggregateLiterals(RelNode input, List<RexNode>
literals) {
+ return appendAggregateLiterals(input, literals, false);
+ }
+
+ private static RelNode appendAggregateLiterals(RelNode input, List<RexNode>
literals, boolean insideUnion) {
+ RelNode rel = unboxRel(input);
+ if (rel instanceof Union && (insideUnion || hasProjectedLiterals(rel))) {
Review Comment:
`hasProjectedLiterals` is true for any literal in any union input, for
example a tag column `'a' AS src`. In that case the type and approx literals
move into the union inputs, and these queries that work on master now fail:
```sql
-- planning error: ANYVALUE expects a literal data type argument, got: null
SET inferAnyValueType=true;
SELECT src, ANY_VALUE(x) FROM (SELECT 'a' src, int_col x FROM t UNION ALL
SELECT 'b', int_col FROM t2) GROUP BY src
-- NPE "blockTimeSet is null": findImmediateProjects uses input 0's literal
for every input
SELECT LASTWITHTIME(x, t) FROM (SELECT int_col x, CAST('2019-01-01 00:00:00'
AS TIMESTAMP) t FROM t
UNION ALL SELECT int_col, ts_col FROM t2)
-- approx params on, NPE "blockValSet is null"
SELECT DISTINCTCOUNT(x), COVARPOP(x, y) FROM (SELECT int_col x, 1.0 y FROM t
UNION ALL SELECT int_col, CAST(long_col AS DOUBLE) FROM t2)
-- v1 skip-leaf: Invalid aggregation function: mode($1,$2,$3)
SELECT /*+ aggOptions(is_skip_leaf_stage_group_by='true') */ src, k, MODE(s)
FROM (
SELECT 'a' src, int_col k, string_col s FROM t UNION ALL SELECT 'b',
int_col, string_col FROM t2)
WHERE k = 2 GROUP BY src, k
```
Other tagged-union queries give the same results, but each row now carries
the constant columns through every union-input exchange.
Suggestion, either:
- push into the union inputs only when an aggregate call already has a
literal argument (position 1 or later) that `findImmediateProjects` reads
through the union, not for any projected literal; or
- never push into the inputs: keep one project above the union, and write a
column there as a literal when every input projects the same literal for it.
I tested both locally. Each gives 0 regressions against master and keeps all
your fixes. Please add these queries as tests.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/optimizer/statement/AggregateFunctionRewriteOptimizer.java:
##########
@@ -112,6 +113,10 @@ private static void rewrite(@Nullable Expression
expression, Schema schema, bool
}
return;
}
+ if (normalizedName.equals(AggregationFunctionType.ARRAYAGG.name()) &&
isArrayAggWithoutDataType(function)) {
Review Comment:
SSE wrong results: `SELECT ARRAY_AGG(DISTINCT intCol) FROM t` now runs but
returns duplicates. On master it fails with `expects 2 or 3 arguments`.
`CalciteSqlParser` drops the DISTINCT quantifier for `SqlKind.ARRAY_AGG` (it
handles DISTINCT only for OTHER_FUNCTION), so this rewrite makes it a plain
`arrayagg(intCol,'INT')`. The GROUP BY form and `ARRAY_AGG(DISTINCT intCol,
false)` have the same problem. MSE now returns distinct values for the same
SQL, so the two engines disagree.
Suggestion: in `CalciteSqlParser`, turn DISTINCT on ARRAY_AGG into
`isDistinct = true` (append the flag or replace it), as MSE does. That also
fixes `ARRAY_AGG(DISTINCT col, 'INT')`, which returns duplicates on master. To
keep the explicit form unchanged instead, reject DISTINCT on ARRAY_AGG in SSE.
Please add an SSE test.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -610,10 +610,14 @@ 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.WITHOUT_ANY_VALUE);
Review Comment:
Wrong results: this early instance also rewrites `MODE(stringExpr)` to 3
args before `AggregateCaseToFilter` runs. So `MODE(CASE WHEN c THEN s END)`
keeps its CASE and does not become `MODE(s) FILTER (WHERE c)`. With null
handling off (the default), the rows that do not match count as `''`:
```sql
SELECT MODE(CASE WHEN int_col > 3 THEN string_col END) FROM tbl -- master:
'c', PR: ''
```
The GROUP BY, skip-leaf, FILTER and JSON forms change the same way in v1, v2
and lite mode.
Suggestion: make the early instance rewrite only ARRAY_AGG, for example with
a separate rule instance. Then MODE and FIRST/LAST_WITH_TIME plan as on master.
--
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]
