Skip to content

PR: fix inconsistent reading mode with read-in-order optimizations#80652

Merged
devcrafter merged 40 commits intomasterfrom
pr-reading-mode-conflict
Jun 18, 2025
Merged

PR: fix inconsistent reading mode with read-in-order optimizations#80652
devcrafter merged 40 commits intomasterfrom
pr-reading-mode-conflict

Conversation

@devcrafter
Copy link
Copy Markdown
Member

@devcrafter devcrafter commented May 21, 2025

Fixes #75961

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

For some queries executed with parallel replicas, reading in order optimization(s) could be applied on initiator while can't be applied on remote nodes. It leads to different reading modes used by parallel replicas coordinator (on initiator) and on remoted nodes, which is a logical error

@devcrafter devcrafter marked this pull request as draft May 21, 2025 19:38
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented May 21, 2025

Workflow [PR], commit [5586628]

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label May 21, 2025
@devcrafter devcrafter marked this pull request as ready for review June 5, 2025 10:35
@KochetovNicolai KochetovNicolai self-assigned this Jun 10, 2025
Comment on lines +712 to +719
if (!blocksHaveEqualStructure(header, plan_header))
{
auto converting_dag = ActionsDAG::makeConvertingActions(
plan_header.getColumnsWithTypeAndName(), header.getColumnsWithTypeAndName(), ActionsDAG::MatchColumnsMode::Name);

auto expression = std::make_unique<ExpressionStep>(plan_header, std::move(converting_dag));
plan->addStep(std::move(expression));
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks a bit specific to ReadFromLocalParallelReplicaStep.
Another option is to add ExpressionStep to query plan (if needed), and only then call replaceNode

Copy link
Copy Markdown
Member

@KochetovNicolai KochetovNicolai left a comment

Choose a reason for hiding this comment

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

Overall looks good.

@devcrafter devcrafter enabled auto-merge June 12, 2025 14:48
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 17, 2025

Workflow [PR], commit [ce58617]

Summary:

job_name test_name status info comment
Stateless tests (amd_debug, distributed plan, s3 storage) failure
02443_detach_attach_partition FAIL
Integration tests (release, 1/4) failure
Job Timeout Expired FAIL

@devcrafter devcrafter added this pull request to the merge queue Jun 18, 2025
Merged via the queue into master with commit 1b4641d Jun 18, 2025
118 of 123 checks passed
@devcrafter devcrafter deleted the pr-reading-mode-conflict branch June 18, 2025 14:08
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Parallel replicas: logical error: Replica 0 decided to read in Default mode, not in WithOrder. This is a bug

3 participants