[CALCITE-7675] GROUP BY ALL and ORDER BY ALL should infer keys only from expressions that depend on the input - #5127
Conversation
|
I guess you did not read the comment I left on the issue |
tisyabhatia
changed the title
7b178b7 to
5e4cae7
Compare
…rom expressions that depend on the input
GROUP BY ALL made a grouping key of every non-aggregate SELECT item, and
ORDER BY ALL made a sort key of every SELECT item, including items that have the
same value in every row. Grouping by such an item does not
divide the input, and where the item is a bare integer literal the expansion
emits SQL that is read back as a select-list ordinal:
SELECT deptno, 42, count(*) FROM emp GROUP BY ALL
expanded to GROUP BY EMP.DEPTNO, 42, which fails validation with "Ordinal out of
range" wherever SqlConformance.isGroupByOrdinal() is true, and which silently
resolves to a different SELECT item where the value is within the select list's
size. The expanded form is what is stored as a view or materialized-table
definition, so the ambiguity outlives the original query.
ORDER BY ALL has the same defect, and in the default conformance, because
isSortByOrdinal() is true there: "SELECT sal, 42 FROM emp ORDER BY ALL" fails,
and "SELECT 2, deptno FROM emp ORDER BY ALL" silently sorts on deptno twice.
Infer keys only from expressions that depend on the input row. An expression
that references no column and all of whose operators are deterministic is
excluded: 42, 'x', 1 + 1, upper('x'), current_date, and a dynamic parameter. A
call to a non-deterministic operator such as RAND() does vary from row to row
and is retained, which is the rule [CALCITE-7697] applies to window keys one
phase later. An unresolved function is retained, since its determinism is not
yet known.
This follows BigQuery, which infers keys only from "expressions that ...
reference a name from the FROM clause", and which specifies that "if the set of
inferred grouping keys is empty after exclusions are applied, all input rows are
considered a single group for aggregation".
Behavior change: where a constant was previously the only grouping key, no key
now remains and the query becomes a single-group aggregation. So
SELECT 'x', count(*) FROM emps WHERE FALSE GROUP BY ALL
returns one row rather than none, and JdbcTest.testGroupByAllOverEmptyInput is
updated accordingly. A consequence is that GROUP BY ALL is not exactly
equivalent to writing the non-aggregate SELECT items out by hand.
Where no sort key remains, ORDER BY ALL now generates no ORDER BY clause; an
empty but non-null sort list would unparse with redundant parentheses.
Not included, and left for a separate issue: BigQuery also excludes an
expression that only references keys inferred from other SELECT items, so that
"SELECT x, x + 1 FROM t GROUP BY ALL" groups by x alone.
A sub-query is retained without being inspected. Its identifiers include table
names and aliases, which are not expressions in this sense; resolving them here
would be premature, and would wrongly reject a query whose alias happens to name
a niladic constant under a conformance that requires parentheses.
Two limits of the rule are worth stating. It inherits those of
SqlOperator.isDeterministic(), which reports a user-defined function as
deterministic unless it says otherwise, so a call to one with no column argument
is excluded. And the rewrite now calls makeNullaryCall, so a niladic constant
used without parentheses under a conformance that disallows it may report
handleUnresolvedFunction from the rewrite rather than from select-list
validation.
tisyabhatia
changed the title
5e4cae7 to
98d5119
Compare
|
your commit title and PR title should match the issue title |
| + "where false\n" | ||
| + "group by all") | ||
| .returnsCount(0); | ||
| .returns("EXPR$0=x; EXPR$1=0\n"); |
There was a problem hiding this comment.
one of these two tests must be wrong.
you wrote both of them.
There was a problem hiding this comment.
This PR changes the rule, which I should have led with. Under CALCITE-7594 'x' was a grouping key, so empty input produced no groups; here keys come only from expressions that depend on the input, so none remains and the query is a single-group aggregation. I'm following BigQuery / DuckDB, which infers keys only from expressions referencing a FROM name and treats an empty set like one group.
There was a problem hiding this comment.
I don't understand. We have to agree on the semantics of this query. It cannot be dictated by the implementation. On the contrary: the implementation has to produce the correct result.
There was a problem hiding this comment.
The expansion is what gets stored as a view definition, so:
SELECT deptno, count(*) as cnt, sum(sal) AS sm, 42 AS constant FROM emp GROUP BY ALL
unparses to GROUP BY 42. And that would fail with "ordinal out of range." The in-range case is also invalid behavior because SELECT ... 2 AS constant ... GROUP BY ALL resolves to GROUP BY deptno, 2, and 2 resolves to COUNT(*) which is an aggregate and we cannot group by aggregates.
On the empty-input semantics, Calcite's behavior:
SELECT count(), sum(sal) FROM emp GROUP BY ALL -- 1 row
SELECT count(), sum(sal), 42 as constant FROM emp GROUP BY ALL -- 0 rows
the first is what is already in CALCITE-7594. Adding a constant should not change the cardinality so that's why the inferred keyset should exclude it.
…expansion The expansion of GROUP BY ALL is what is stored as a view or materialized-table definition, so it is read back by a validator that cannot know which keys the user wrote and which the expansion synthesized. Assert that the expansion carries the same meaning on re-parse under a conformance where an integer in GROUP BY is a select-list ordinal. Covers both ways the previous behaviour broke: an out-of-range literal, which failed to validate, and an in-range one, which designated a different SELECT item.
4e24c84 to
e49c393
Compare
|
2 participants
GROUP BY ALLandORDER BY ALLinfer their keys from the SELECT list. This excludes from that inference any expression that does not depend on the input row:42,'x',1 + 1,upper('x'),current_date, a dynamic parameter. A non-deterministic call such asRAND()varies per row and is retained, as in CALCITE-7697. Where no key remains,GROUP BY ALLbecomesGROUP BY ()andORDER BY ALLgenerates no clause.Behavior change
SELECT 'x', count(*) FROM emps WHERE FALSE GROUP BY ALLreturns one row rather than none, soJdbcTest.testGroupByAllOverEmptyInputis updated. Both assertions were right for the rule in force when written; this PR changes the rule.GROUP BY ALLis consequently no longer exactly equivalent to writing the non-aggregate SELECT items by hand.Why
The expansion copies expressions out of the SELECT list, and those synthesized keys are then resolved as ordinals. Out of range it throws; in range it silently picks a different SELECT item:
Both
ORDER BYcases are the default conformance, whereisSortByOrdinal()is true. The expanded form is stored as a view or materialized-table definition, so the ambiguity outlives the query.This follows BigQuery, which infers keys only from expressions that reference a name in the
FROMclause and falls back toGROUP BY ()when none remain. DuckDB matches; Spark and Snowflake keep constants, so it is a real divergence.Alternative
The constant could instead be retained and synthesized keys marked so they are never resolved as ordinals. That preserves every result but leaves
GROUP BY 42in stored definitions. Working, and I can switch if preferred.Notes
Not included: BigQuery also excludes expressions referencing only other inferred keys (
SELECT x, x + 1 ... GROUP BY ALLgroups byxalone) — separate issue.Tests for both clauses in
SqlValidatorTest,agg.iqandsort.iq;reference.mdupdated. CI green.