Support group_by_use_nulls in TOTALS#39574
Conversation
There was a problem hiding this comment.
Probably using ActionsDAG::makeConvertingActions would shorten the code, but the current version is more specific and better for showing intent.
|
? |
|
@alexey-milovidov currently distributed case is broken in this PR, I'll fix it. |
|
@novikd This PR learns how to walk :-) Do you plan to fix it or with Analyser everything will just work? |
|
Dear @vdimir, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself. |
|
@novikd, We need this. |
|
@groeneai Will you be cool enough to rebase this 4 year old change on top of current master and make it green? |
|
Dear @vdimir, you haven't been active on this PR for 30 days. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself. |
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer planner: * PlannerExpressionAnalysis: include WITH TOTALS in the group_by_use_nulls condition so the result type of the keys is analyzed as Nullable. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers both a pure constant key and a mixed real plus constant key. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers both a pure constant key and a mixed real plus constant key. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers the raw key value in the TOTALS row, a pure constant key, and a mixed real plus constant key. It also covers the expression level so the IdentifierResolveScope typing is protected: toTypeName(k) over the WITH TOTALS result, 'k IS NULL' which is 1 in the TOTALS row, and a wrapping subquery that selects the TOTALS row with WHERE k IS NULL. Without the IdentifierResolveScope change these report the non-nullable type and fold 'k IS NULL' to 0 while the raw value cases still pass, so they are the regression guard for that part of the fix. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers the raw key value in the TOTALS row, a pure constant key, and a mixed real plus constant key. It also covers the expression level so the IdentifierResolveScope typing is protected: toTypeName(k) over the WITH TOTALS result, 'k IS NULL' which is 1 in the TOTALS row, and a wrapping subquery that selects the TOTALS row with WHERE k IS NULL. Without the IdentifierResolveScope change these report the non-nullable type and fold 'k IS NULL' to 0 while the raw value cases still pass, so they are the regression guard for that part of the fix. Documentation: the group_by_use_nulls setting text in src/Core/Settings.cpp and the WITH TOTALS section in docs/.../select/group-by.md now state that with group_by_use_nulls = 1 the real GROUP BY keys in the WITH TOTALS row are Nullable and reported as NULL, and that a folded constant key keeps its value. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. GROUP BY ALL needs an extra step. Its key list is empty until expandGroupByAll collects the keys from the projection, and that normally runs near the end of resolveQuery, after resolveGroupByNode is the only place that registers keys in nullable_group_by_keys. So with the new WITH TOTALS gate, GROUP BY ALL took the delayed-resolution path with no registered keys: the projection was typed non-nullable while validateAggregates compared it against keys it had converted to Nullable, raising NOT_AN_AGGREGATE. resolveQuery now expands GROUP BY ALL before resolveGroupByNode when group_by_use_nulls is set. The expansion uses a throwaway clone of the projection so the real projection keeps its aliases and is not constant-folded against the pre-nullable types; the clone's resolved key nodes still match the real keys structurally, because nullable_group_by_keys ignores aliases and types. GROUP BY ALL WITH TOTALS then behaves exactly like the explicit GROUP BY case, and GROUP BY ALL WITH ROLLUP/CUBE (which had the same NOT_AN_AGGREGATE error before) works too. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers the raw key value in the TOTALS row, a pure constant key, and a mixed real plus constant key. It also covers the expression level so the IdentifierResolveScope typing is protected: toTypeName(k) over the WITH TOTALS result, 'k IS NULL' which is 1 in the TOTALS row, and a wrapping subquery that selects the TOTALS row with WHERE k IS NULL. Without the IdentifierResolveScope change these report the non-nullable type and fold 'k IS NULL' to 0 while the raw value cases still pass, so they are the regression guard for that part of the fix. The same expression-level cases are added for GROUP BY ALL WITH TOTALS as the regression guard for the expansion-order fix above. Documentation: the group_by_use_nulls setting text in src/Core/Settings.cpp and the WITH TOTALS section in docs/.../select/group-by.md now state that with group_by_use_nulls = 1 the real GROUP BY keys in the WITH TOTALS row are Nullable and reported as NULL, and that a folded constant key keeps its value. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. GROUP BY ALL needs an extra step. Its key list is empty until expandGroupByAll collects the keys from the projection, and that normally runs near the end of resolveQuery, after resolveGroupByNode is the only place that registers keys in nullable_group_by_keys. So with the new WITH TOTALS gate, GROUP BY ALL took the delayed-resolution path with no registered keys: the projection was typed non-nullable while validateAggregates compared it against keys it had converted to Nullable, raising NOT_AN_AGGREGATE. resolveQuery now expands GROUP BY ALL before resolveGroupByNode when group_by_use_nulls is set. The expansion uses a throwaway clone of the projection so the real projection keeps its aliases and is not constant-folded against the pre-nullable types; the clone's resolved key nodes still match the real keys structurally, because nullable_group_by_keys ignores aliases and types. GROUP BY ALL WITH TOTALS then behaves exactly like the explicit GROUP BY case, and GROUP BY ALL WITH ROLLUP/CUBE (which had the same NOT_AN_AGGREGATE error before) works too. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers the raw key value in the TOTALS row, a pure constant key, and a mixed real plus constant key. It also covers the expression level so the IdentifierResolveScope typing is protected: toTypeName(k) over the WITH TOTALS result, 'k IS NULL' which is 1 in the TOTALS row, and a wrapping subquery that selects the TOTALS row with WHERE k IS NULL. Without the IdentifierResolveScope change these report the non-nullable type and fold 'k IS NULL' to 0 while the raw value cases still pass, so they are the regression guard for that part of the fix. The same expression-level cases are added for GROUP BY ALL WITH TOTALS as the regression guard for the expansion-order fix above. Documentation: the group_by_use_nulls setting text in src/Core/Settings.cpp and the WITH TOTALS section in docs/.../select/group-by.md now state that with group_by_use_nulls = 1 the real GROUP BY keys in the WITH TOTALS row are Nullable and reported as NULL, and that a folded constant key keeps its value. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Documentation: the group_by_use_nulls setting text and the WITH TOTALS section now note that only keys whose types can be wrapped in Nullable become NULL in the totals row; Array and Map keys (which fail canBeInsideNullable) keep their default value, alongside the folded-constant exception.
Rebase and port of ClickHouse#39574 (original author @novikd) to the analyzer. With group_by_use_nulls enabled, GROUP BY keys are reported as Nullable for GROUPING SETS, ROLLUP and CUBE, but plain GROUP BY ... WITH TOTALS was left out: the keys stayed non-nullable and the TOTALS summary row showed 0 instead of NULL. The original PR fixed this in the old interpreter by converting the keys inside AggregatingStep. The analyzer is now the default and needs the same behavior, so this ports the fix to the analyzer in two places that gate the group_by_use_nulls handling on the GROUP BY modifier and previously listed only GROUPING SETS / ROLLUP / CUBE: * IdentifierResolveScope: include WITH TOTALS so the query tree resolves the GROUP BY keys (and expressions over them) as Nullable. Without this the planner-side conversion below produces a Nullable key column at runtime while query-tree analysis still treats the key as non-nullable, e.g. 'k IS NULL' over the TOTALS row folds to 0 and toTypeName(k) reports the non-nullable type, and a wrapping subquery sees the wrong key type. * PlannerExpressionAnalysis: include WITH TOTALS so the result type of the keys is analyzed as Nullable, consistent with the scope above. * Planner: insert a toNullable conversion for the GROUP BY keys right before the TotalsHaving step. The TOTALS row is built from default values, so a Nullable key column yields NULL there. This runs for CUBE/ROLLUP WITH TOTALS as well, because their own nullable conversion happens after TotalsHaving and would otherwise leave the TOTALS row non-NULL. Keys that cannot be made Nullable are skipped. GROUP BY ALL needs an extra step. Its key list is empty until expandGroupByAll collects the keys from the projection, and that normally runs near the end of resolveQuery, after resolveGroupByNode is the only place that registers keys in nullable_group_by_keys. So with the new WITH TOTALS gate, GROUP BY ALL took the delayed-resolution path with no registered keys: the projection was typed non-nullable while validateAggregates compared it against keys it had converted to Nullable, raising NOT_AN_AGGREGATE. resolveQuery now expands GROUP BY ALL before resolveGroupByNode when group_by_use_nulls is set. The expansion uses a throwaway clone of the projection so the real projection keeps its aliases and is not constant-folded against the pre-nullable types; the clone's resolved key nodes still match the real keys structurally, because nullable_group_by_keys ignores aliases and types. GROUP BY ALL WITH TOTALS then behaves exactly like the explicit GROUP BY case, and GROUP BY ALL WITH ROLLUP/CUBE (which had the same NOT_AN_AGGREGATE error before) works too. Constant GROUP BY keys are folded away before aggregation and are not real key columns, so they keep their constant value in the TOTALS row instead of becoming NULL. This is the same as CUBE/ROLLUP/GROUPING SETS WITH TOTALS, where a folded constant key likewise stays constant in the summary rows. 02373 covers the raw key value in the TOTALS row, a pure constant key, and a mixed real plus constant key. It also covers the expression level so the IdentifierResolveScope typing is protected: toTypeName(k) over the WITH TOTALS result, 'k IS NULL' which is 1 in the TOTALS row, and a wrapping subquery that selects the TOTALS row with WHERE k IS NULL. Without the IdentifierResolveScope change these report the non-nullable type and fold 'k IS NULL' to 0 while the raw value cases still pass, so they are the regression guard for that part of the fix. The same expression-level cases are added for GROUP BY ALL WITH TOTALS as the regression guard for the expansion-order fix above. Documentation: the group_by_use_nulls setting text in src/Core/Settings.cpp and the WITH TOTALS section in docs/en/sql-reference/statements/select/group-by.md now state that with group_by_use_nulls = 1 the real GROUP BY keys in the WITH TOTALS row are reported as NULL, qualified to only keys whose types can be wrapped in Nullable. Keys whose types cannot be made Nullable (for example Array and Map, which fail canBeInsideNullable) keep their default value, as does a folded constant key. The behavior is analyzer-only; the affected tests set enable_analyzer = 1. Co-authored-by: Dmitry Novik <novikd@clickhouse.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Support group_by_use_nulls setting for queries with WITH TOTALS modifier.