yashmayya commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4406545541
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotRuleUtils.java:
##########
@@ -85,6 +89,47 @@ public static RelNode unboxRel(RelNode rel) {
}
}
+ /// Appends literal arguments in one project, retaining a union column as a
literal only when every branch projects
+ /// that same literal. Branch-specific constants remain input references.
+ public static RelNode appendAggregateLiterals(RelNode input, List<RexNode>
literals) {
+ RelNode rel = unboxRel(input);
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ List<RexNode> projects = new ArrayList<>(project.getProjects());
+ projects.addAll(literals);
+ return LogicalProject.create(project.getInput(), project.getHints(),
projects, (List<String>) null, Set.of());
+ }
+ RexBuilder rexBuilder = input.getCluster().getRexBuilder();
+ List<RexNode> projects = new
ArrayList<>(input.getRowType().getFieldCount() + literals.size());
+ for (int i = 0; i < input.getRowType().getFieldCount(); i++) {
+ RexLiteral literal = rel instanceof Union ? commonLiteral(rel, i) : null;
Review Comment:
Regression (my last suggestion missed this case). If every union input
projects the same literal, this line writes that column as a literal.
`findImmediateProjects` then inlines it into the aggregate call. Some functions
read that argument as a column, and `AggregateOperator.getBlockValSetMap` maps
only identifiers, so they get a null value set. These work on master and fail
here in v1, v2 and lite, with null handling on and off:
```sql
-- NPE: "blockTimeSet" is null
SELECT FIRSTWITHTIME(x, c) FROM (SELECT int_col x, 5 c FROM t UNION ALL
SELECT int_col, 5 FROM t2)
-- NPE: "blockValSet" is null
SELECT COVARPOP(x, c), MODE(s) FROM (SELECT int_col x, 50 c, string_col s
FROM t
UNION ALL SELECT int_col, 50, string_col FROM t2)
```
The failure needs a rewrite in the query: no-type FIRST/LAST_WITH_TIME,
string MODE, ANY_VALUE with `inferAnyValueType`, or approx params. It also
shows with `GROUP BY c`, the skip-leaf, leaf-final and partitioned hints, and 3
or more branches.
Suggestion: remove the `commonLiteral` substitution and keep input refs
here, as on master. We tested this: 0 regressions against master. Then a
literal parameter over a union next to a rewritten call (for example `MODE(s),
PERCENTILE(x, 50)`) still fails, as on master. The new union tests that expect
the literal (`AggregatePlans.json`, `ApproximateAggregateRewriteTest`,
`UDFAggregates.json`) need an update.
The complete fix is a runtime change for a follow-up PR: `AggregateOperator`
gives a literal argument a constant value set. We tested it, and it also fixes
the same failure that master has without a rewrite. With that change, skip NULL
literals in `commonLiteral`, because they arrive with type UNKNOWN. The runtime
change alone is not enough for this PR: during a rolling upgrade (new broker,
old servers), the queries above still fail.
##########
pinot-query-runtime/src/test/resources/queries/UDFAggregates.json:
##########
@@ -45,6 +45,12 @@
}
},
"queries": [
+ {"sql": "SELECT CARDINALITY(ARRAY_AGG(x)), DISTINCTCOUNT(x), COVARPOP(x,
y) FROM (SELECT int_col x, 1.0 y FROM {tbl} UNION ALL SELECT int_col,
CAST(long_col AS DOUBLE) FROM {tbl})", "outputs": [[14, 6, 2339.918367346939]]},
+ {"sql": "SELECT MODE(CASE WHEN int_col > 3 THEN string_col END) FROM
{tbl}", "outputs": [["c"]]},
Review Comment:
Minor: these two tests pass even with the round-5 MODE bug put back, because
this harness turns null handling on by default. The bug shows only with null
handling off. Add `SET enableNullHandling=false;` to both queries. The expected
output stays `"c"`.
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -1242,6 +1242,10 @@ private static Expression toExpression(SqlNode node) {
private static Expression compileFunctionExpression(SqlBasicCall
functionNode) {
SqlKind functionKind = functionNode.getKind();
+ if (functionKind == SqlKind.ARRAY_AGG &&
functionNode.getFunctionQuantifier() != null
+ && "DISTINCT".equals(functionNode.getFunctionQuantifier().toString()))
{
+ throw new SqlCompilationException("Function 'ARRAY_AGG' on DISTINCT is
not supported.");
Review Comment:
Regression: this also rejects forms that give correct results on master,
because the explicit `true` flag already removes duplicates:
```sql
SELECT ARRAY_AGG(DISTINCT intCol, 'INT', true) FROM t -- master:
correct, PR: error
SELECT d, ARRAY_AGG(DISTINCT im, 'INT', true) FROM t GROUP BY d --
master: correct, uses the star-tree
EXPLAIN PLAN FOR SELECT ARRAY_AGG(DISTINCT intCol, 'INT') FROM t -- master:
works
```
About 20 forms in our SSE probes fail this way (all column types, FILTER,
GROUP BY, CARDINALITY, HAVING `arrayContainsInt`). My last comment offered
rejection as an option. That was too broad.
Suggestion: do not throw. Set the isDistinct argument to `true`, as MSE
`buildAggCall` does: `(x)` -> `(x, true)`, `(x, 'T')` -> `(x, 'T', true)`, and
replace `false` with `true`. Old servers accept all of these forms, and SSE
then gives the same results as MSE. Please add SSE tests for `(DISTINCT x)`,
`(DISTINCT x, 'INT')` and `(DISTINCT x, 'INT', true)`.
--
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]
