Skip to content

Disable parallelize query processing right after reading FROM when distributed_aggregation_memory_efficient enabled#78500

Merged
nickitat merged 4 commits intoClickHouse:masterfrom
ucasfl:fix-read-from-resize
Apr 8, 2025
Merged

Disable parallelize query processing right after reading FROM when distributed_aggregation_memory_efficient enabled#78500
nickitat merged 4 commits intoClickHouse:masterfrom
ucasfl:fix-read-from-resize

Conversation

@ucasfl
Copy link
Collaborator

@ucasfl ucasfl commented Apr 1, 2025

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):

Disable parallelize query processing right after reading FROM when distributed_aggregation_memory_efficient enabled, it may lead to logical error. Closes #76934.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@ucasfl
Disable parallelize query processing right after reading FROM when di…
d5b4dbc 
…stributed_aggregation_memory_efficient enabled
@clickhouse-gh
Copy link
Contributor

clickhouse-gh bot commented Apr 1, 2025 •
edited
Loading

Workflow [PR], commit [3ea45f8]

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Apr 1, 2025
@nickitat nickitat self-assigned this Apr 1, 2025
@nickitat nickitat added the can be tested Allows running workflows for external contributors label Apr 1, 2025
nickitat
nickitat reviewed Apr 1, 2025
View reviewed changes
Copy link
Member

@nickitat nickitat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls add a test. apart from that - 👍🏼

@ucasfl
Copy link
Collaborator Author

ucasfl commented Apr 2, 2025 •
edited
Loading

pls add a test. apart from that - 👍🏼

Look likes the problem be fixed in master branch accident by refactor IStorageCluster here https://github.com/ClickHouse/ClickHouse/pull/58353/files#diff-326364e371a6f9dcd6722035bd77220c0c021c73cfd423b896b62cc6036c9a48R102-R112, so the modified IStorage::read method does not called anymore in this case.

Should we still keep the fix?

@nickitat
Copy link
Member

nickitat commented Apr 2, 2025

pls add a test. apart from that - 👍🏼

Look likes the problem be fixed in master branch accident by refactor IStorageCluster here https://github.com/ClickHouse/ClickHouse/pull/58353/files#diff-326364e371a6f9dcd6722035bd77220c0c021c73cfd423b896b62cc6036c9a48R102-R112, so the modified IStorage::read method does not called anymore in this case.

Should we still keep the fix?

I'm not sure about the fix but we better to add a test, otherwise it will be broken by the next refactoring

@ucasfl
Copy link
Collaborator Author

ucasfl commented Apr 2, 2025

pls add a test. apart from that - 👍🏼

Look likes the problem be fixed in master branch accident by refactor IStorageCluster here https://github.com/ClickHouse/ClickHouse/pull/58353/files#diff-326364e371a6f9dcd6722035bd77220c0c021c73cfd423b896b62cc6036c9a48R102-R112, so the modified IStorage::read method does not called anymore in this case.
Should we still keep the fix?

I'm not sure about the fix but we better to add a test, otherwise it will be broken by the next refactoring

I have add a simple test, but due to it has been fixed by above code refactor, so it can not be validated.

@ucasfl ucasfl requested a review from nickitat April 7, 2025 08:09
@nickitat nickitat added this pull request to the merge queue Apr 8, 2025
Merged via the queue into ClickHouse:master with commit a724d31 Apr 8, 2025
117 of 120 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Apr 8, 2025
@ucasfl ucasfl deleted the fix-read-from-resize branch April 9, 2025 02:34
@ucasfl ucasfl mentioned this pull request Apr 10, 2025
Closed
@MikhailBurdukov
Copy link
Contributor

MikhailBurdukov commented Sep 9, 2025

@nickitat
Hi! Can we try to make a backport of this bugfix?

@nickitat
Copy link
Member

nickitat commented Sep 9, 2025

@nickitat Hi! Can we try to make a backport of this bugfix?

Hi, to what version?

@MikhailBurdukov
Copy link
Contributor

MikhailBurdukov commented Sep 9, 2025

25.3

@robot-clickhouse-ci-2 robot-clickhouse-ci-2 mentioned this pull request Sep 9, 2025
Merged
robot-clickhouse-ci-2 added a commit that referenced this pull request Sep 9, 2025
@robot-clickhouse-ci-2
Merge pull request #86904 from ClickHouse/cherrypick/25.3/78500
65971bf 
Cherry pick #78500 to 25.3: Disable parallelize query processing right after reading FROM when distributed_aggregation_memory_efficient enabled
robot-clickhouse added a commit that referenced this pull request Sep 9, 2025
@robot-clickhouse
Backport #78500 to 25.3: Disable parallelize query processing right a…
71b76c1 
…fter reading FROM when distributed_aggregation_memory_efficient enabled
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 mentioned this pull request Sep 9, 2025
Merged
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Sep 9, 2025
nickitat added a commit that referenced this pull request Sep 12, 2025
@nickitat
Merge pull request #86905 from ClickHouse/backport/25.3/78500
a03f7be 
Backport #78500 to 25.3: Disable parallelize query processing right after reading FROM when distributed_aggregation_memory_efficient enabled
@nickitat nickitat mentioned this pull request Oct 3, 2025
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickitat nickitat nickitat approved these changes

Assignees

@nickitat nickitat

Labels

can be tested Allows running workflows for external contributors pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

A simple GROUP BY query failed with distributed_aggregation_memory_efficient enabled and group by key items greater than 65535

5 participants

@ucasfl @nickitat @MikhailBurdukov @robot-ch-test-poll @robot-clickhouse-ci-2