ClickHouse / ClickHouse/ClickHouse

Trino dialect omits `group_by_use_nulls`: ROLLUP/CUBE rows return 0, not NULL

Open
#119,396 0 comments 0 reactions 1 assignee Claimed by @alexey-milovidov View on GitHub
bug-unreleased comp-query-execution comp-settings
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.