Skip to content

Fix analyzer - preserve column in group by aggregation.#60046

Closed
yakov-olkhovskiy wants to merge 2 commits intomasterfrom
fix-analyzer-group-dag
Closed

Fix analyzer - preserve column in group by aggregation.#60046
yakov-olkhovskiy wants to merge 2 commits intomasterfrom
fix-analyzer-group-dag

Conversation

@yakov-olkhovskiy
Copy link
Member

@yakov-olkhovskiy yakov-olkhovskiy commented Feb 15, 2024

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

found by fuzzer:

set allow_experimental_analyzer=1;
SELECT cityHash64('limit', _CAST(materialize('World'), 'LowCardinality(String)')) FROM system.one GROUP BY GROUPING SETS ('limit');

Received exception from server (version 24.1.5):
Code: 49. DB::Exception: Received from localhost:9000. DB::Exception: Argument column 'String' size 2 doesn't match result column size 1 of function cityHash64: while executing 'FUNCTION cityHash64('limit'_String :: 0, _CAST(materialize('World'_String), 'LowCardinality(String)'_String) :: 1) -> cityHash64('limit'_String, _CAST(materialize('World'_String), 'LowCardinality(String)'_String)) LowCardinality(UInt64) : 2'. (LOGICAL_ERROR)
(query: SELECT cityHash64('limit', _CAST(materialize('World'), 'LowCardinality(String)')) FROM system.one GROUP BY GROUPING SETS ('limit'))

@yakov-olkhovskiy yakov-olkhovskiy changed the title Fiix analyzer - preserve column in group by aggregation. Fix analyzer - preserve column in group by aggregation. Feb 15, 2024
@yakov-olkhovskiy yakov-olkhovskiy added the analyzer Issues and pull-requests related to new analyzer label Feb 15, 2024
@robot-clickhouse-ci-1 robot-clickhouse-ci-1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Feb 15, 2024
@robot-clickhouse-ci-1
Copy link
Contributor

robot-clickhouse-ci-1 commented Feb 15, 2024 •
edited by robot-clickhouse-ci-2
Loading

This is an automated comment for commit 05adefb with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Successful checks
Check nameDescriptionStatus
Docs checkBuilds and tests the documentation✅ success
Style checkRuns a set of checks to keep the code style clean. If some of tests failed, see the related log from the report✅ success
Check nameDescriptionStatus
A SyncThere's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS❌ failure
CI runningA meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR⏳ pending
ClickHouse build checkBuilds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process⏳ pending
Fast testNormally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here❌ failure
Mergeable CheckChecks if all other necessary checks are successful❌ failure

alexey-milovidov
alexey-milovidov approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@alexey-milovidov alexey-milovidov left a comment

Choose a reason for hiding this comment

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

Also, commit the exact found case as a test.
(it makes sense, because the fuzzer takes time to find it)

@alexey-milovidov alexey-milovidov self-assigned this Feb 15, 2024
@Algunenano
Copy link
Member

Algunenano commented Feb 16, 2024

Is #59684 related to this bug? There are multiple queries crashing with analyzer + group by there

@yakov-olkhovskiy
Copy link
Member Author

yakov-olkhovskiy commented Feb 16, 2024

@Algunenano it is possible - seems like I inadvertently fixed some significant bug...

@yakov-olkhovskiy yakov-olkhovskiy mentioned this pull request Feb 17, 2024
Merged
azat
azat reviewed Feb 18, 2024
View reviewed changes
src/Planner/PlannerExpressionAnalysis.cpp
continue;

auto expression_type_after_aggregation = group_by_use_nulls ? makeNullableSafe(expression_dag_node->result_type) : expression_dag_node->result_type;
available_columns_after_aggregation.emplace_back(nullptr, expression_type_after_aggregation, expression_dag_node->result_name);
Copy link
Member

@azat azat Feb 18, 2024

Choose a reason for hiding this comment

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

I'm not sure that sharing const column for aggregator is a good idea, since there are some manipulations to it, so I think it is better to avoid sharing this column at all, like here https://github.com/ClickHouse/ClickHouse/pull/59986/files#diff-06a4eb7b985c6d6bcb43e37d94ada33f85a846a26674641e6648395f704bcc23R96

Copy link
Member Author

@yakov-olkhovskiy yakov-olkhovskiy Feb 19, 2024

Choose a reason for hiding this comment

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

I compared to current version's DAG for the case in this PR and seems it is also sharing this column...

@Algunenano Algunenano mentioned this pull request Feb 20, 2024
Closed
1 task
@yakov-olkhovskiy
Copy link
Member Author

yakov-olkhovskiy commented Feb 23, 2024

closing in favor of #59986

azat added a commit to azat/ClickHouse that referenced this pull request Mar 14, 2024
@azat
Add a test from ClickHouse#60046
b11b732 
Signed-off-by: Azat Khuzhin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

analyzer Issues and pull-requests related to new analyzer pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@yakov-olkhovskiy @robot-clickhouse-ci-1 @Algunenano @azat @alexey-milovidov