Repository navigation
Conversation
xiangfu0
added
multi-stage
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19396 +/- ##
=============================================
+ Coverage 58.05% 68.30% +10.24%
- Complexity 7 1450 +1443
=============================================
Files 2708 3525 +817
Lines 167054 229272 +62218
Branches 27265 36389 +9124
=============================================
+ Hits 96987 156593 +59606
+ Misses 61910 60400 -1510
- Partials 8157 12279 +4122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
afe7a5d to
af8af6a
Compare
|
I've been working on my own fix for The idea I have is to stop generating SortedMailboxReceiveOperator and substitute that with a sort on the receiver side (with optional sort on the sender side when the limit is small). Once we have that, we can start thinking about recovering SortedMailboxReceiveOperator as a k-way merge when the senders guarantee data is sent in order, which is what you and #19121 are doing (although #19121 keeps both the current and the k-way merge). Also, I don't think |
cafa948 to
f151331
Compare
xiangfu0
changed the title
|
@gortiz I rebased this onto current
Validation on head |
fa82748 to
e968380
Compare
515927f to
f87d986
Compare
|
Rebased onto master after #19412 and kept the responsibilities separate: sender sorting owns ordering; #19396 k-way merges only confirmed-sorted local/gRPC streams and safely falls back for mixed/custom transports. Final coverage review added two targeted regressions for queued read-ahead preservation during fallback and buffered-cursor cleanup on sender error. The focused class passes 21/21, module formatting/checkstyle/license gates pass, and Codecov improved to 91.37% patch coverage. Exact head |
9642c66 to
7ecc01c
Compare
|
Rebased and simplified on top of Fresh matched JMH results on JDK 25, 400k rows, two servers, local + gRPC fan-in:
Both JMH-reported intervals are non-overlapping. Global is the stronger signal: every PR measurement was below every baseline measurement. The partitioned aggregate also favors the PR, but visible fork variance means 25.5% is this run's estimate, not a stable production-wide effect size. Local validation passed: 89 focused runtime tests, all 243 |
7ecc01c to
5aeb374
Compare
xiangfu0
changed the title
aa52944 to
10dd7e9
Compare
gortiz
left a comment
gortiz
left a comment
There was a problem hiding this comment.
Thanks for the thorough PR description and benchmark matrix; the design is careful about mixed-version compatibility.
Main requests:
- Drop the no-op Sort above the exchange when the exchange sorts on the receiver (inline at
PinotWindowExchangeNodeInsertRule). Every receiver already returns rows in order. On new servers the Sort is a pass-through, and on old servers it is a second full sort. With it gone,SortOperator.isInputSortedandSortedMultiStageOperatorare no longer needed. - Add a kill switch: a broker config plus a query option, in the style of
sortExchangeCopyThreshold, but as a boolean, because the window Sort has no fetch for a threshold to act on. - Rolling upgrade: with a new broker and old servers, the same rows can be sorted several times (old leaf V1
ORDER BY ... LIMIT 2147483647, old receiver full sort, old Sort above it). Results stay correct, but queries are slower until the upgrade finishes. - Tests: a randomized merge-vs-full-sort test (senders, block splits, tie density, starvation order) would protect the equal-head / lazy-heap logic.
Other comments are inline; several are nits or confirmations.
8eee817 to
2673b61
Compare
|
@gortiz Thanks for the detailed review. Final head |
9163e7e to
1feb422
Compare
|
Update to supersede my September 26 default-off summary: current head The PR description now includes the new A–B–B–A and same-cluster paired measurements, including the remaining small/uncertain fallback overhead. Focused tests, package build, formatting, checkstyle, license, and added-line warning checks passed locally. Exact-head CI is running; no unresolved review thread remains. I am not claiming a universal no-regression guarantee or requesting merge before CI/re-review. |
1feb422 to
6843676
Compare
xiangfu0
changed the title
6843676 to
555f55c
Compare
Make the optimization opt-in, remove the redundant receiver sort, and add randomized and plan-contract coverage.
Reproduce: run globalPresortedWindow with four senders and windowSortOnSender=true. Use a two-level tournament to reduce per-row comparisons, with a 10k-row boundary test.
Use one priority queue merge path and keep rollout fallback, bounded output and mailbox progress handling. Enable windowSortOnSender=true for global ordered windows; the default remains receiver sorting.
Compile queries before planning and explaining, and document the intentional legacy receiver assertion. This keeps the sender-sorted window tests free of new deprecation warnings.
555f55c to
6dff651
Compare
|
I'd like to use this PR to fix how MSE assigns the responsibility for ordering. Today, ordering should be a requirement that the logical plan guarantees. Instead, the physical plan and the runtime keep getting extra machinery to guarantee it: The runtime has to trust the plan. Re-checking ordering on every block is like a JIT adding checks for invariants that the front-end compiler already proved. It is also fragile: What I'd suggest instead is to express ordering with simple plan nodes, not flags:
On the Calcite side, this could be a new In practice, for this PR that means removing the mailbox metadata and the Compatibility gets simpler too. On a new server:
For a new broker with old servers, it's the broker's responsibility not to send the new node and to keep generating a plan that old servers understand, i.e. a plain receive with a Sort downstream. We already have the mechanism for this: With this, |
Expect the explicit MAX_VALUE fetch on receiver sorts below ordered windows. This preserves complete window input when a response cap is configured; SQL limits, window frames and ordering remain unchanged. Update only the 122 affected golden outputs (124 sort nodes). The full 593-case plan suite and existing complete-window regressions pass.
|
@gortiz updated at The broker gate requires matching, non-SNAPSHOT broker/server release versions and fails closed on missing/unreadable configs, registration failure or disconnect; multi-cluster queries disable it. Unsupported clusters use plain receive plus an explicit MAX-fetch Sort, preserving complete window input. Legacy Recent unchanged planner/window coverage passed 962 cases, including full-input following/RANGE regressions. This latest commit only corrects the existing SNAPSHOT assertion (+7/-3 lines); both broker gate tests, JDK25 warning-enabled compilation, formatting, checkstyle and license checks passed, with no warnings on added lines. All 17 reported checks were green at |
3 participants
PR flow
Opt-in sender-sorted merging for global ordered windows: version homogeneity check → planner rule → k-way merge exchange → merge receive operator.
AI-generated · Green: added · Yellow: modified · Red: removed · Gray: existing
Partial evidence: 0 file patches omitted; 2 truncated.
Diff evidence
Global ordered windows can keep an explicit Sort on each sender and use a logical k-way merge exchange with a distinct merge receive. Plain sending mailboxes transport the ordered input. The feature remains opt-in; matching immutable release versions are required, and missing, unknown, mixed, unreadable or SNAPSHOT versions and multi-cluster queries retain the compatible receiver-sort plan. The cached capability check is a point-in-time observation.
Receiver-side window Sorts explicitly retain complete input with MAX fetch and zero offset, including following frames and RANGE peers beyond response caps. Plan expectations were corrected at 124 internal Sort lines in 122 window queries; SQL, results, assertions, ignored flags, outer limits, frames and other plan fields remain unchanged. Independent review compared every corrected expected output against the hosted actual plan. The stalled-sender read-ahead follow-up remains #19395.
Validation at
b047dd479ea360f19cc8ea13595a874590ed66de: all 16 hosted PR checks passed, including both unit sets, both integration sets, all four compatibility lanes, quickstart, Java 11 client compatibility, linter and dependency/security checks. The hosted checkout was merge78aea7a9c868893b2a1b693566fc2eed9e2bb7cfwith base4e27013f9647e95e1ad2a2198d9c26eda411f867. These results certify that checkout and source head; they do not certify later master changes.Local warning/deprecation-enabled validation passes 962 cases: full ResourceBasedQueryPlansTest 593, QueryCompilationTest 246, WindowAggregateOperatorTest 122 and window rule 1, with zero failures/errors/skips. Scoped mandatory checks pass. The hosted planner suite also executed all 593 cases without failures/errors/skips, and current runtime/new-method execution is documented in the final audit. Counts overlap earlier validation and are not summed. Existing conditional or disabled hosted cases remain reported in their logs.
Maintainer architecture acceptance and independent review remain required. The four dependent PRs retain their agreed stack bases and feature-only diffs; full child CI remains subject to prerequisite landing and the agreed base update.