Skip to content

Analyzer: Fix wrong results for grouping sets with ColumnConst#79743

Merged
yakov-olkhovskiy merged 5 commits intoClickHouse:masterfrom
zvonand:fix-group-by-const-cols
Jun 11, 2025
Merged

Analyzer: Fix wrong results for grouping sets with ColumnConst#79743
yakov-olkhovskiy merged 5 commits intoClickHouse:masterfrom
zvonand:fix-group-by-const-cols

Conversation

@zvonand
Copy link
Contributor

@zvonand zvonand commented Apr 30, 2025

Closes #70655.

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Fix wrong results for grouping sets with ColumnConst and Analyzer

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@zvonand zvonand force-pushed the fix-group-by-const-cols branch from 0717296 to aef039d Compare May 1, 2025 08:33
@Algunenano Algunenano added the can be tested Allows running workflows for external contributors label May 1, 2025
@clickhouse-gh
Copy link
Contributor

clickhouse-gh bot commented May 1, 2025

Workflow [PR], commit [fc773bf]

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label May 1, 2025
@zvonand
Copy link
Contributor Author

zvonand commented May 1, 2025

I do not think this test 00757_enum_defaults_const_analyzer is actually correct:
(Introduced in #59986)

Firstly, for Nullable data type default value is Null:

Field DataTypeNullable::getDefault() const

And the value in output of the test is not null only because original column (ColumnConst in our case) is propagated all the way to output, and the result we see in this test is the default value of the ColumnConst itself (Default value == column value in this case), not its underlying type. So, it is a default value for ColumnConst in this particular case, not for the Enum default value or even a string default value.

It used to behave differently before analyzer was introduced, which is correct IMO: https://fiddle.clickhouse.com/a3af412c-582d-42e5-baf1-8f9a7e46b4aa

This is also counter-intuitive when comparing it to what docs say: key columns containing default values (zeros or empty lines), which is very hard to expect to be the default value of ColumnConst, not its underlying type default value. If we change the ColumnConst, the TOTALS will also change: https://fiddle.clickhouse.com/6a370979-6f6f-49d7-ac67-e17b31fedb21

@azat what do you think?

https://fiddle.clickhouse.com/654a66cb-ff0a-44ef-9a2e-3925769c4a23

@zvonand zvonand force-pushed the fix-group-by-const-cols branch 3 times, most recently from b4f8939 to b64d1b3 Compare May 2, 2025 13:08
@zvonand zvonand changed the title Fix wrong results for grouping sets with ColumnConst and Analyzer Analyzer: Fix wrong results for grouping sets with ColumnConst May 6, 2025
@zvonand zvonand force-pushed the fix-group-by-const-cols branch from baa7932 to 8fe4ba4 Compare May 6, 2025 13:00
@yakov-olkhovskiy yakov-olkhovskiy self-assigned this May 7, 2025
@zvonand zvonand force-pushed the fix-group-by-const-cols branch from 17ec743 to 48f6c26 Compare May 12, 2025 08:42
@zvonand zvonand force-pushed the fix-group-by-const-cols branch from 5ba34c2 to 50d31a6 Compare May 19, 2025 20:42
@zvonand zvonand force-pushed the fix-group-by-const-cols branch from 47d9893 to fc773bf Compare June 3, 2025 07:55
@zvonand
Copy link
Contributor Author

zvonand commented Jun 3, 2025

@yakov-olkhovskiy the CI is green, PR ready for review

@zvonand
Copy link
Contributor Author

zvonand commented Jun 3, 2025

The idea behind this PR is described in the comment above.

In a couple of words it is like this:
The original column (which is ColumnConst) is kind of duplicated to avoid losing its original type unexpectedly. In most cases this is a good idea, but not in this particular case: it is OK if column loses its constness when processing aggregation. Otherwise, the result would be incorrect. So, for this case we add an option not to preserve the original column when processing aggregation.

Copy link
Contributor

@ianton-ru ianton-ru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yakov-olkhovskiy yakov-olkhovskiy added this pull request to the merge queue Jun 11, 2025
Merged via the queue into ClickHouse:master with commit 712d4af Jun 11, 2025
120 checks passed
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 11, 2025
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Jun 16, 2025
Analyzer: Fix wrong results for grouping sets with ColumnConst
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Jun 17, 2025
24.3.14 Backport of ClickHouse#79743 - Analyzer: Fix wrong results for grouping sets with ColumnConst
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 10, 2025
Analyzer: Fix wrong results for grouping sets with ColumnConst
Enmk added a commit to Altinity/ClickHouse that referenced this pull request Jul 11, 2025
25.3.5 Backport of ClickHouse#79743: Fix wrong results for grouping sets with ColumnConst
zvonand pushed a commit to Altinity/ClickHouse that referenced this pull request Jul 16, 2025
Analyzer: Fix wrong results for grouping sets with ColumnConst
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Jul 18, 2025
25.3.6 Backport of ClickHouse#79743: Fix wrong results for grouping sets with ColumnConst
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Feb 18, 2026
25.3.6 Backport of ClickHouse#79743: Fix wrong results for grouping sets with ColumnConst
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Analyzer GROUPING SETS return uncorrected key column value if it constant

6 participants