[CALCITE-7744] ORDER BY agg(col) on a query where an alias shadows col produces an invalid plan containing an aggregate call in a Project

[1fanwang]

Jira Link

CALCITE-7744

Changes Proposed

SELECT max(sal) AS sal, deptno, job FROM emp GROUP BY deptno, job ORDER BY max(sal) passes validation but fails during execution with Unable to implement EnumerableCalc.

The ORDER BY alias expansion turns the expression into max(max(sal)) after the original expression has already been validated. The inner aggregate then leaks into a Project. Validate expanded non-measure ORDER BY expressions before SQL-to-rel conversion. The query now returns Calcite's existing Aggregate expressions cannot be nested validation error instead of building an illegal plan. Measure aliases retain their existing scope and conversion path.

Verification

JAVA_HOME=$(/usr/libexec/java_home -v 21) ./gradlew :core:test \
  --tests org.apache.calcite.test.SqlValidatorTest \
  --tests org.apache.calcite.test.JdbcTest --no-daemon

The JDBC regression uses CalciteAssert.Config.SCOTT and the query above. The same test ran against production source from upstream/main and from this branch.

Raw logs

Before:
java.sql.SQLException: Unable to implement EnumerableCalc(...)
  EnumerableAggregate(group=[{0, 1}], SAL=[MAX($2)], agg#1=[MAX($3)])
    EnumerableCalc(... expr#8=[MAX($t5)] ...)
Suppressed: java.lang.RuntimeException: cannot translate call MAX($t5)
1 test completed, 1 failed

After:
SqlValidatorTest: 593 completed, 0 failed, 7 skipped
JdbcTest: 409 completed, 0 failed, 17 skipped
Gradle Test Run :core:test: 1002 completed, 0 failed, 24 skipped
BUILD SUCCESSFUL

[iwanttobepowerful]

SELECT max(sal) AS sal, deptno, job FROM emp GROUP BY deptno, job ORDER BY max(sal);

postgresql can return result, instead of throw error

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[1fanwang]

SELECT max(sal) AS sal, deptno, job FROM emp GROUP BY deptno, job ORDER BY max(sal);

postgresql can return result, instead of throw error

Thanks for raising this @iwanttobepowerful

PostgreSQL and Calcite resolve this query differently.

PostgreSQL only recognizes an output alias when it appears by itself in ORDER BY. In ORDER BY max(sal), sal therefore refers to the input column, so the query succeeds.

Calcite allows SELECT aliases inside ORDER BY expressions. Since the query defines max(sal) AS sal, Calcite expands the expression to max(max(sal)), which is invalid.

Right now this PR proposes to keep Calcite's current resolution rules. It replaces the later planner failure with a clear validation error.

I think matching PostgreSQL would require a separate conformance change that's beyond the scope of this ticket/PR

[mihaibudiu]

This error message is not very clear; I know you didn't create it. Does the error at least point to the offending aggregate?

[1fanwang]

Not precisely: the error spans columns 12 to 83, while the aggregate itself is at columns 76 to 83.

[mihaibudiu]

at least it's on the same line.

[vlsi]

Requesting changes for the error position, where the check runs, and missing validator-level tests. Details are in the inline comments. Two points concern the issue rather than a line of the diff:

• As @iwanttobepowerful noted, PostgreSQL runs this query: it resolves sal inside max(...) to the input column. This PR rejects the query instead, and JdbcTest makes the rejection the expected result, so a later change that adopts PostgreSQL's resolution would have to invert the test. Should OrderExpressionExpander fall back to the input column when an alias that stands for an aggregate appears inside an aggregate argument and a column with that name exists? If that is out of scope, please say so in CALCITE-7744 so the rejection is not taken as the intended semantics.
• select deptno, max(sal) as sal from emp group by deptno order by max(sal) over () has the same cause and still throws AssertionError: type mismatch, both before and after this change. A windowed aggregate over an aggregate is legal, so this query should run. Please record it in the JIRA, even if you fix it separately.

The description says the change validates "before SQL-to-rel conversion", but SqlToRelConverter calls expandOrderExpr too (see the inline comment on SqlValidatorImpl). Please update the description once the placement is settled.

[vlsi]

expandOrderExpr is not only a validator method. SqlToRelConverter calls it twice per ORDER BY item, from convertOrderItem and from the extra projections in convertSelectList, and SqlAbstractGroupFunction.validateCall calls it as well. With this line, every ORDER BY expression is validated again during SQL-to-rel conversion. The Javadoc of SqlValidator#expandOrderExpr describes an expansion only and does not mention that the method can throw a validation error. Could the check move to the validation phase, for example validateOrderItem or OrderByScope.validateExpr?

Wherever the check ends up, it needs a comment on two choices a reader cannot infer from the code: why it validates in getSelectScope(select) rather than the ORDER BY scope, and why it skips the measure branch. I checked the second one: with validate before the isMeasure() check, select deptno + 1 as d1, d1 + 2 as measure d3 from emp order by d3 fails with Column 'D1' not found in any table.

[vlsi]

This is the only negative test, and it runs end to end, but the fix is in the validator. Please add the rejection to SqlValidatorTest with a ^...^ position and ERR_NESTED_AGG (see my comment there). If this JDBC test stays, please give it the /** Test case for <a href="https://issues.apache.org/jira/browse/CALCITE-7744">[CALCITE-7744]</a> ... */ comment that the neighboring regression tests carry, and a name that states the outcome as well as the scenario.

[vlsi]

Following up on the position question: at the validator level the error is reported at line 1 col 12 thru line 2 col 38, which is max(^sal) as sal, ... order by max(sal)^. The span starts inside the SELECT-list aggregate and runs across FROM and GROUP BY. The ORDER BY item as written, max(sal), contains no nested aggregate, so Aggregate expressions cannot be nested does not tell the user what to fix. Please point the error at the ORDER BY item and say that SAL resolved to the select-list alias of an aggregate. A new resource message would do it.

[vlsi]

This line guards the measure exclusion in expandOrderExpr, but nothing marks it as a guard, and inside testAsMeasure it looks like one more measure example. Please add a comment that says what it guards, or move it to a test whose name says so.

Please also add validator-level tests for the rejection itself, with positions:

• the query from the issue: select max(sal) as sal, deptno, job from emp group by deptno, job order by ^max(sal)^
• order by max(sal) desc
• order by sal + max(sal)
• order by count(*) filter (where sal > 1000), which now fails with FILTER must not contain aggregate expression

On main, the last two also fail in Enumerable code generation, so this change affects them too. Please also add a case that must still pass: the same aggregate in ORDER BY without the shadowing alias, for example select max(sal) as m, deptno, job from emp group by deptno, job order by max(sal).

[vlsi]

Correction to my review: the OVER () case is unrelated to this PR. The failure does not need an alias at all: select deptno, max(sal) as m, max(max(sal)) over () as w from emp group by deptno throws the same AssertionError: type mismatch on main from SqlToRelConverter.createAggImpl, with ref: SMALLINT NOT NULL and input: TINYINT. Please ignore that point and do not add it to CALCITE-7744.

It is now filed as CALCITE-7822: even select deptno, count(*) over () from emp group by deptno fails there, so it has nothing to do with nested aggregates either.