ClickHouse / ClickHouse/ClickHouse
Trino dialect omits `group_by_use_nulls`: ROLLUP/CUBE rows return 0, not NULL
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 21h 32m
- Merged PRs (30d)
- 515
Description
### Describe what's wrong
**Under `dialect = 'trino'`, the super-aggregate rows of `GROUP BY ROLLUP`, `CUBE` and `GROUPING SETS` carry the column type default (`0`, `''`) instead of `NULL`. `SELECT x, count(*) FROM (VALUES 1, 1, 2) AS t(x) GROUP BY ROLLUP (x)` returns a `0 3` total row; Trino returns `NULL 3`.**
- **Root cause:** [`src/Interpreters/executeQuery.cpp:2383-2385`](https://github.com/ClickHouse/ClickHouse/blob/c108e273b608778/src/Interpreters/executeQuery.cpp#L2383-L2385) sets `join_use_nulls`, `use_variant_as_common_type` and `enable_analyzer` but not `group_by_use_nulls`. The translator cannot fix this at the text level either - `TrinoSyntaxTranslator.cpp:132` explicitly defers all such semantics to this block.
Analysis details (evidence, affected locations, impact)
**Why we believe this is a bug:** `executeQueryImpl` ([`src/Interpreters/executeQuery.cpp:2357`](https://github.com/ClickHouse/ClickHouse/blob/c108e273b608778/src/Interpreters/executeQuery.cpp#L2357)) enters the Trino branch, parses with `ParserTrinoQuery`, then forces the settings that encode Trino NULL semantics onto the query context at :2383-2385. `group_by_use_nulls` is not among them, so the aggregator keeps ClickHouse's default of filling grouped-out keys with the type default.
**Affected locations:**
- [`src/Interpreters/executeQuery.cpp:2383`](https://github.com/ClickHouse/ClickHouse/blob/c108e273b608778/src/Interpreters/executeQuery.cpp#L2383) — Trino dialect context settings; `group_by_use_nulls` missing from the list
- [`src/Parsers/Trino/TrinoSyntaxTranslator.cpp:132`](https://github.com/ClickHouse/ClickHouse/blob/c108e273b608778/src/Parsers/Trino/TrinoSyntaxTranslator.cpp#L132) — comment delegating all query-semantics settings to executeQuery.cpp
**Impact:** Silently wrong results for every Trino `ROLLUP`, `CUBE` and `GROUPING SETS` query - the class of query the modifier exists for. A subtotal row is indistinguishable from a genuine `x = 0` / `x = ''` group, so a BI tool migrating dashboards from Trino gets plausible but wrong numbers with no error. The PR's user documentation lists only integer division and banker's rounding as remaining ClickHouse-side deviations, so users are told this case is covered.
### Does it reproduce on most recent release?
Yes — confirmed on current `master` (commit `c108e273b608778`).
### How to reproduce
[▶ Run on ClickHouse Fiddle](https://fiddle.clickhouse.com/3111c059-a98d-47a7-9a8b-3bac11f4500a)
Reproducer
```sql
-- Test: in the Trino dialect the super-aggregate rows of ROLLUP/CUBE/GROUPING SETS hold NULL, not the column type default.
SET allow_experimental_trino_dialect = 1;
SET dialect = 'trino';
SELECT x, count(*) FROM (VALUES 1, 1, 2) AS t(x) GROUP BY ROLLUP (x) ORDER BY x NULLS LAST;
SELECT s, count(*) FROM (VALUES 'a', 'b') AS t(s) GROUP BY GROUPING SETS ((s), ()) ORDER BY s NULLS LAST;
SELECT x, y, count(*) FROM (VALUES (1, 'a')) AS t(x, y) GROUP BY CUBE (x, y) ORDER BY x NULLS LAST, y NULLS LAST;
SET dialect = 'clickhouse';
```
### Expected behavior
Expected output of the reproducer above:
```
1 2
2 1
\N 3
a 1
b 1
\N 2
1 a 1
1 \N 1
\N a 1
\N \N 1
```
### Error message and/or stacktrace
Actual output of the reproducer above on `master` (`c108e273b608778`):
```
0 3
1 2
2 1
2
a 1
b 1
0 1
0 a 1
1 1
1 a 1
```
Suggested fix
Add `context->setSetting("group_by_use_nulls", true);` to the same block at [`src/Interpreters/executeQuery.cpp:2383`](https://github.com/ClickHouse/ClickHouse/blob/c108e273b608778/src/Interpreters/executeQuery.cpp#L2383). Trade-off: with `group_by_use_nulls = 1` the grouping-key columns of a `ROLLUP`/`CUBE`/`GROUPING SETS` query become `Nullable`, which changes `toTypeName` output for those queries - but that is exactly the Trino type, and the same trade-off was already accepted for `join_use_nulls`. If the type change is unwanted, the alternative is to document `GROUPING SETS` alongside `/` and `round` in the known-deviations list.
Additional context
**Open risks:**
- `group_by_use_nulls` also affects `WITH TOTALS`, which has no Trino counterpart; turning it on for the dialect changes the totals row of a hand-written ClickHouse `WITH TOTALS` query executed under `dialect = 'trino'`.
Found during automated review of [PR #115383](https://github.com/ClickHouse/ClickHouse/pull/115383). Severity P2 · Finding `h_pr115383_101`
cc @alexey-milovidov (from #115383)
Contributor guide
Assessment
This issue has not been assessed yet.