Backport #114523 to 26.7: Fix wrong results of the partial aggregation strategy in distributed query plans - #118802
Merged
Merged
Conversation
…n strategy in distributed query plans
robot-clickhouse
added
the
pr-backport
Contributor
|
Workflow [BackportPR], commit [708c4c9] Summary: ✅ |
Member
|
@copilot pls fix the build |
…_bucket_number Co-authored-by: nickitat <5218483+nickitat@users.noreply.github.com>
Contributor
Fixed in ecd... (removed the stray
Fixed in 5cbd46b: removed the stray |
|
|
…ettings applyParallelReplicas.cpp:85:55: error: use of undeclared identifier 'optimization_settings' On master `ApplyParallelReplicasVisitor` holds a `QueryPlanOptimizationSettings` reference; on 26.7 it does not, and `applyParallelReplicas` ignores its settings argument. The memory-efficient merge is correspondingly hard-coded to `false` there, so the cherry-pick only had to move that constant up, not master's expression. #114523's real fix for this branch is in `makeDistributed.cpp`, which does have the settings in scope and is unchanged from the original. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The read-rows estimator on this branch does not reach the per-column NDV hint for the aggregation key, so the group estimate falls back to the hinted row count of 50000, that exceeds `distributed_plan_max_rows_to_broadcast`, and the planner picks Shuffle instead of the partial aggregation the test is about. Lowering the hint keeps the reader-bucket split intact: it is gated on the real row count from range analysis, not on the hint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`04869_distributed_plan_partial_aggregation_bucket_order` fails
reproducibly on this backport: the planner picks `Shuffle` instead of
partial aggregation plus a memory-efficient merge, so the `EXPLAIN`
prints `GatherExchange / Aggregating / ShuffleExchange` where the
reference has `MergingAggregated (merge) / GatherExchange /
Aggregating (partial)` - and the test never reaches the code this
pull request fixes.
The distributed rewrites run post-order, so `tryMakeDistributedRead`
has already inserted a `GatherExchangeStep` above the read by the time
`tryMakeDistributedAggregation` calls `estimateReadRowsCount`. With
the estimation unable to look through an exchange it returns no stats
at all, and the strategy falls back to `Shuffle` "to be safe". The
row-count hint never reaches the read step, which is why lowering
`cardinality` did not help.
`0fae364ef698` ("Make row count estimation look through exchange steps
in non-cloud builds") is a commit of this very pull request and it
removed those two `CLICKHOUSE_CLOUD` guards. It was lost from the
backport: PR #86353 (Cascades) removed the same guards on `master`
three days before #114523 merged, so by merge time the hunk was a
no-op in the pull request's net diff, and the backport carries only
that net diff. `master` and `26.8` have no `CLICKHOUSE_CLOUD` left in
this file; this branch still had both guards.
The guarded include is dropped rather than unguarded, because the
same header is already included unguarded further down, which leaves
this file exactly as `master` has it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Original pull-request #114523
Cherry-pick pull-request #118799
This pull-request is a last step of an automated backporting.
Treat it as a standard pull-request: look at the checks and resolve conflicts.
Merge it only if you intend to backport changes to the target branch, otherwise just close it.
The PR source
The PR is created in the CI job
Version info
26.7.8.11