8000 Antalya 26.6 Backport of #128865 - Fix wrong `ORDER BY` results and `pushed bucket twice` with offset based parallel replicas by zvonand · Pull Request #2802 · Altinity/ClickHouse · GitHub
[go: up one dir, main page]
Skip to content

Antalya 26.6 Backport of #128865 - Fix wrong ORDER BY results and pushed bucket twice with offset based parallel replicas - #2802

Merged
zvonand merged 2 commits into
antalya-26.6from
backports/antalya-26.6/128865
Nov 6, 2026
Merged

Antalya 26.6 Backport of #128865 - Fix wrong ORDER BY results and pushed bucket twice with offset based parallel replicas#2802
zvonand merged 2 commits into
antalya-26.6from
backports/antalya-26.6/128865

Conversation

@zvonand
@zvonand zvonand commented Nov 5, 2026
Copy link
Copy Markdown
Member

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

Fixed wrong results of ORDER BY (unsorted rows, wrong rows under LIMIT) and the GroupingAggregatedTransform pushed bucket ... twice error (duplicated groups in older versions) of queries with parallel_replicas_mode = 'sampling_key', or a custom key mode on a cluster with more than one shard. Such a query now reads a shard from one replica unless the initiator only aggregates or fully sorts the parts returned by the replicas. (ClickHouse#128865 by @groeneai)

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

alexey-milovidov and others added 2 commits November 5, 2026 20:33
@alexey-milovidov @zvonand
Merge pull request ClickHouse#128865 from groeneai/offset-parallel-re…
c3b7597 
…plicas-ordered-merges

Fix wrong `ORDER BY` results and `pushed bucket twice` with offset based parallel replicas

Conflicts committed as-is; the next commit resolves them:
- src/Processors/QueryPlan/Optimizations/Optimizations.h (both modified)
- src/Processors/QueryPlan/Optimizations/optimizeTree.cpp (both modified)
- src/Processors/QueryPlan/ReadFromRemote.cpp (both modified)
- src/Processors/QueryPlan/ReadFromRemote.h (both modified)
@zvonand
Resolve conflicts in backport of ClickHouse#128865
5f93e27 
Added the PR's one-replica condition to antalya-26.6's existing GET_ONE/GET_MANY branch in `ReadFromRemote::addPipe`, and dropped unrelated upstream code that came in through the conflict hunks.
@zvonand
zvonand commented Nov 5, 2026
Copy link
Copy Markdown
Member Author

Backporter: cherry-pick conflicts resolved by AI

Conflicted files:

  • src/Processors/QueryPlan/Optimizations/Optimizations.h (both modified)
  • src/Processors/QueryPlan/Optimizations/optimizeTree.cpp (both modified)
  • src/Processors/QueryPlan/ReadFromRemote.cpp (both modified)
  • src/Processors/QueryPlan/ReadFromRemote.h (both modified)

Commits:

  • c3b7597: cherry-pick with the conflicts committed as-is

  • 5f93e27: resolution

  • Optimizations.h: kept the branch's std::optional<String> optimizeUseAggregateProjections and added only the allowReadingShardsFromSeveralReplicas declaration. Upstream's applyStreamDisjointness and UseProjectionsResult came from other PRs and were dropped.

  • optimizeTree.cpp: added only the final allowReadingShardsFromSeveralReplicas(root) call after considerEnablingParallelReplicas. Upstream's group-by top-k and HAVING prefilter passes in the same hunk aren't part of this PR and don't exist on 26.6, so they were dropped.

  • ReadFromRemote.cpp: kept the branch's canUseTaskBasedParallelReplicas() || parallel_replicas_disabled check and added || !allow_reading_shard_from_several_replicas to force GET_ONE. Added the PR's LOG_DEBUG in initializePipeline, but kept the branch's addPipes(shards, output_header) call and left out upstream's unavailable-shard-tracker unit counting, which isn't on 26.6.

  • ReadFromRemote.h: kept both is_remote_function and the new allow_reading_shard_from_several_replicas member.

@github-actions
github-actions Bot commented Nov 5, 2026
Copy link
Copy Markdown

Workflow [PR], commit [5f93e27]

@zvonand
zvonand commented Nov 6, 2026
Copy link
Copy Markdown
Member Author

@blau-ai

@blau-ai
blau-ai commented Nov 6, 2026
Copy link
Copy Markdown
Collaborator

CI triage for 5f93e27

Summary: 4 failing checks. 0 are caused by this PR, 1 is a process/metadata issue (DCO), and 3 are flaky or infra. The backport's own tests did not fail in any suite: 05483_offset_parallel_replicas_ordered_merges and test_parallel_replicas_custom_key don't appear among the failures in result_pr.json.

Check Classification PR-caused?
DCO Commits missing Signed-off-by Not a code issue, but the fix is on this PR
Stateless tests (amd_asan_ubsan, cas s3 storage, parallel, 1/2) Flaky: stray S3 404 in stderr No
Integration tests (amd_asan_ubsan, db disk, old analyzer, 4/8) Infra: pytest session timeout, 0 test failures No
RegressionTestsRelease / S3Export (part) Infra: GitHub API 5xx while setting commit status, all scenarios passed No

1. DCO: not a code problem, fix on this PR

Neither commit has a sign-off:

Fix: if the repo requires DCO, add sign-offs. One way is git rebase --signoff origin/antalya-26.6 plus a force-push, which only the branch owner should do. Otherwise, other antalya-26.6 PRs (#2806, #2801, #2749) pass DCO with signed commits, so a maintainer can set this check aside for backports.

2. Stateless (amd_asan_ubsan, cas s3 storage, parallel, 1/2): 04812_merge_alias_in_prepared_set, flaky

1 failure out of 5586. Query output was correct. The test failed only because a line showed up in stderr:

Reason: having stderror:
<Error> AWSClient: Response status: 404, Not Found
  • The test is about MergeTree ALIAS columns in prepared sets. It doesn't use parallel replicas, OFFSET, or ReadFromRemote, so this PR's code doesn't run in it.
  • The 404 log line comes from the CAS-on-S3 storage layer in this job variant, not from the query.
  • praktika's own rerun diagnosis: "Runs: 7, Failed: 0, Passed: 7 … The failure is not reproducible (likely a transient issue)."
  • The same check passed on Synchronize CAS test clock and retry callbacks #2801 (Failed: 0, Passed: 5586).

Next step: safe to re-run.

3. Integration (amd_asan_ubsan, db disk, old analyzer, 4/8): session timeout, infra

!!!!!! xdist.dsession.Interrupted: session-timeout: 10800.0 sec exceeded !!!!!!!
========= 1158 passed, 44 skipped, 50 warnings in 10950.62s (3:02:30) ==========
  • No test failed. The batch was at 96% when the 3 h --session-timeout cut it off. It ran with -n 2, and most of the time went to test_storage_iceberg_with_spark/* and test_export_partition_to_iceberg/*.
  • The last tests reported (test_attach_without_fetching, test_cas_gc_bulk_delete, …) all passed, so nothing was hung.
  • test_parallel_replicas_custom_key, the only integration test this PR changes, is not in this batch.
  • The same shard passed on Fix alias error in distributed calls, simplification for nanoseconds in Iceberg #2749 (Failures: 0/1248), which means the slowdown depends on the runner.

Next step: re-run the job.

4. RegressionTestsRelease / S3Export (part) / s3_export_part: infra

The TestFlows summary (nice-new-fails) is clean:

17 features (17 ok)
90 scenarios (90 ok)
169 examples (168 ok, 1 skipped)

The job went red only because of the step that posts the commit status:

❌ Failed to set commit status: {
  "message": "Server Error"
##[error]Process completed with exit code 1.

Next step: re-run, or ignore. The tests themselves passed.

Bottom line: I found nothing that needs a code change in this backport. Re-run the stateless 1/2, integration 4/8 and S3Export (part) jobs, and add DCO sign-offs (or have the check overridden). I did not rebuild or re-run anything locally; this is based on the CI reports and logs alone.

@zvonand
zvonand merged commit d21bf1d into antalya-26.6 Nov 6, 2026
309 of 316 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

antalya antalya-26.6 antalya-26.6.8.20003 backport Backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zvonand @blau-ai @svb-alt @alexey-milovidov
0