yashmayya commented on code in PR #19988:
URL: https://github.com/apache/pinot/pull/19988#discussion_r4407264560
##########
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java:
##########
@@ -1308,6 +1308,16 @@ private static Expression
compileFunctionExpression(SqlBasicCall functionNode) {
operands.add(toExpression(childNode));
}
}
+ if (functionKind == SqlKind.ARRAY_AGG &&
functionNode.getFunctionQuantifier() != null
+ && "DISTINCT".equals(functionNode.getFunctionQuantifier().toString()))
{
+ Expression distinct = RequestUtils.getLiteralExpression(true);
+ if (operands.size() == 1 || (operands.size() == 2 &&
operands.get(1).isSetLiteral()
+ && operands.get(1).getLiteral().isSetStringValue())) {
+ operands.add(distinct);
+ } else if (operands.size() == 2 || operands.size() == 3) {
+ operands.set(operands.size() - 1, distinct);
Review Comment:
Nit, not blocking: this treats any 2nd argument that is not a string literal
as the flag, and it overwrites any 3rd argument. Some effects:
```sql
SELECT ARRAY_AGG(DISTINCT intCol, UPPER('long')) FROM t -- master:
LONG_ARRAY, PR: INT_ARRAY (type dropped)
SELECT ARRAY_AGG(DISTINCT intCol, longCol) FROM t -- master: error,
PR: runs as (intCol, 'INT', true)
SELECT ARRAY_AGG(DISTINCT intCol, 'INT', strCol) FROM t -- master: error,
PR: 3rd argument dropped
SELECT ARRAY_AGG(DISTINCT intCol ORDER BY intCol) FROM t -- master: error,
PR: ORDER BY ignored
```
Suggestion: replace an operand only when it is a boolean literal. Otherwise
append `true` for 2 operands, and leave 3 operands as they are, so bad input
still fails as on master.
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotApproximateAggregateRewriteRule.java:
##########
@@ -101,29 +100,22 @@ public void onMatch(RelOptRuleCall call) {
// indices of every call stay valid. With no parameters configured the
plan keeps the shape it had before, with no
// extra project.
//
- // Invariant: this project has to stay directly beneath the aggregate.
Both PinotAggregateExchangeNodeInsertRule
- // and AggregatePushdownRule inline the literal only through
findImmediateProjects(input), so anything that puts a
- // node between them would leave the leaf with a column reference where
the function expects a parameter string.
- // Nothing in the later phases does: AggregateProjectMergeRule needs an
all-input-ref project and the literals
- // block it, and ProjectMergeRule collapses into a single project that
stays adjacent.
+ // Keep existing literal arguments visible to the aggregate split rules,
including those projected in union inputs.
Review Comment:
Nit: since `commonLiteral` is gone, literals projected in union inputs no
longer stay visible. The new project above a union holds input refs. Please
update this comment.
--
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]
