Skip to content

Fix MSE explainAskingServers segment grouping and remove all-or-nothing failure#18696

Merged
gortiz merged 2 commits into
apache:masterfrom
gortiz:fix/mse-explain-asking-servers-grouping
Jun 9, 2026
Merged

Fix MSE explainAskingServers segment grouping and remove all-or-nothing failure#18696
gortiz merged 2 commits into
apache:masterfrom
gortiz:fix/mse-explain-asking-servers-grouping

Conversation

@gortiz

@gortiz gortiz commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Problem

With explainAskingServers=true, the broker asks servers for their physical plans and ExplainNodeSimplifier groups the per-segment children of each combine node. The grouping was all-or-nothing: it folded every child into a single plan and, if any pair was not mergeable, hit assert false and bailed out keeping every child. This produced two symptoms:

  • Huge plans — with assertions disabled (prod default), it returned the fully un-grouped plan with one node per segment. This is much worse when prefetch is enabled, since each segment is wrapped in an AcquireReleaseColumnsSegment node.
  • Deterministic explain failures — with assertions enabled (-ea), the same condition threw AssertionError, surfaced to the user as a generic INTERNAL "Error while planning query". Failure vs success was deterministic per environment (does the broker run with -ea?) and per query (do all segment plans merge?).

Fix

Replace the all-or-nothing fold with a greedy partial merge — the same approach already used across servers in PlanNodeMerger.mergeCombineChildren:

  • Equivalent segment plans collapse into a single group; genuinely different plans (e.g. some segments use an index, others do a full scan) are kept as separate groups. The merge never fails.
  • When more than one distinct plan remains, each group is wrapped in an Alternative node annotated with a segments count, so the reader can tell the plans apart and see how many segments run each one. The count uses the default (additive) merge type, so per-server counts are summed as plans merge across servers (mirrors how alternatives across servers are represented in AskingServerStageExplainer).
  • Single-plan output is unchanged, so existing explain tests are unaffected.

Example divergent output:

StreamingCombineSelect
  Alternative(segments=[500])
    SelectStreaming(...)        // 500 segments doing a scan
  Alternative(segments=[3])
    FilterSortedIndex(...)      // 3 segments hitting a sorted index

Also fixes an inverted condition in PlanNodeMerger.visitExchange that reported exchanges over the same tables as not mergeable (and exchanges over different tables as mergeable).

Tests

Adds unit tests for ExplainNodeSimplifier and PlanNodeMerger (there were none): identical-collapse, summed attributes, partial grouping with counts, the prefetch AcquireReleaseColumnsSegment shape, no-throw-on-unmergeable, and cross-server count summing. The no-throw test fails against the previous code with AssertionError: Unmergeable inputs found, reproducing the deterministic failure.

🤖 Generated with Claude Code

…ng failure

ExplainNodeSimplifier merged the per-segment children of a combine explain
node in an all-or-nothing way: it folded every child into a single plan and,
if any pair was not mergeable, hit `assert false` and bailed out keeping all
children. This caused two symptoms with explainAskingServers=true:

- With assertions disabled (prod default) it returned the full un-grouped plan
  with one node per segment, producing huge plans (DATA-116), made worse when
  prefetch wraps each segment in AcquireReleaseColumnsSegment.
- With assertions enabled (-ea) the same condition threw AssertionError, which
  surfaced to the user as a generic INTERNAL "Error while planning query".
  This is the deterministic, env-dependent explain failure.

Replace the all-or-nothing fold with a greedy partial merge (the same approach
already used across servers in PlanNodeMerger.mergeCombineChildren): equivalent
segment plans collapse into a single group, genuinely different plans are kept
as separate groups, and the merge never fails. When more than one distinct plan
remains, each group is wrapped in an "Alternative" node annotated with a
"segments" count so the reader can see how many segments run each plan. The
count uses the default (additive) merge type, so per-server counts are summed
as plans merge across servers. Single-plan output is unchanged.

Also fix an inverted condition in PlanNodeMerger.visitExchange that reported
exchanges over the same tables as not mergeable (and vice versa).

Add unit tests for ExplainNodeSimplifier and PlanNodeMerger (none existed).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gortiz gortiz requested a review from yashmayya June 8, 2026 10:07
@gortiz gortiz added the multi-stage Related to the multi-stage query engine label Jun 8, 2026
@codecov-commenter

codecov-commenter commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.27273% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.58%. Comparing base (474efb1) to head (66e7609).
⚠️ Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
...t/query/planner/explain/ExplainNodeSimplifier.java 80.48% 3 Missing and 5 partials ⚠️
...ry/planner/explain/AskingServerStageExplainer.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #18696      +/-   ##
============================================
+ Coverage     64.48%   64.58%   +0.09%     
  Complexity     1291     1291              
============================================
  Files          3372     3372              
  Lines        208639   208666      +27     
  Branches      32590    32606      +16     
============================================
+ Hits         134550   134759     +209     
+ Misses        63287    63080     -207     
- Partials      10802    10827      +25     
Flag Coverage Δ
custom-integration1 100.00% <ø> (ø)
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-21 64.58% <77.27%> (+0.09%) ⬆️
temurin 64.58% <77.27%> (+0.09%) ⬆️
unittests 64.57% <77.27%> (+0.09%) ⬆️
unittests1 56.99% <77.27%> (+0.11%) ⬆️
unittests2 37.16% <0.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yashmayya yashmayya added the enhancement Improvement to existing functionality label Jun 8, 2026

@yashmayya yashmayya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Took a careful pass, mostly on the cross-server merge path since that's where the segment counts get summed.

  • The greedy partial merge looks correct. With verbose=false, "mergeable" is effectively an equivalence relation, so each child lands in exactly one group and the grouping is unambiguous/order-independent.
  • Confirmed the count summing: segments being a DEFAULT/long attribute means the values add when two servers' plans merge, and mergeCombineChildren pairs the Alternatives by their inner plan (HashSet matching), not by position — so it still holds even when the per-server sort order differs.
  • The visitExchange flip is clearly right; it now lines up with every other visit* method (!equals -> not mergeable).

Pulled the branch and ran both new test classes — all 12 pass.

One minor, non-blocking thing: when one server has divergent segment plans (so it emits Alternative wrappers) and another server's segments are all uniform (so the simplifier leaves a single bare child), merging them across servers leaves the uniform server's plan as a bare sibling next to the Alternatives, and its segments don't fold into the matching Alternative count. It's explain-only/cosmetic and still much better than the old behavior, so just flagging it as a possible follow-up.

LGTM 👍

Addresses review feedback: previously the per-segment Alternative wrapper was
only emitted when a server saw more than one distinct segment plan. When one
server had divergent segment plans and another's segments were all uniform,
merging across servers left the uniform server's plan as a bare sibling next to
the Alternatives, and its segment count did not fold into the matching group.

Always wrap each group in an Alternative(segments=N) during per-server
simplification, so every server contributes an Alternative that pairs and sums
counts across servers. After all servers are merged, removeRedundantAlternatives
strips any Alternative that is the sole child of a combine, so combine nodes
where all segments share a plan render exactly as before (no test changes).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@gortiz

gortiz commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review @yashmayya! Good catch on the uniform-vs-divergent case — pushed a follow-up that addresses it (66e7609).

Rather than conditionally emitting the Alternative wrapper only when a server has >1 distinct plan, the simplifier now always wraps each group in Alternative(segments=N) — including the single-group case. That way a uniform server also contributes an Alternative, so it pairs with and folds into the matching group of a divergent server (and the segments counts sum as before). A final removeRedundantAlternatives pass runs after all servers are merged and strips any Alternative that is the sole child of its combine, so the common all-segments-share-a-plan case renders exactly as before — no existing test output changes.

Added unit tests for the cross-server fold (uniformServerFoldsIntoDivergentServerAcrossServers) and the unwrap pass.

@gortiz gortiz merged commit d7258f8 into apache:master Jun 9, 2026
18 of 21 checks passed
@gortiz gortiz deleted the fix/mse-explain-asking-servers-grouping branch June 9, 2026 12:44
@xiangfu0

xiangfu0 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Opened the corresponding docs update PR: pinot-contrib/pinot-docs#863

xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Jun 9, 2026
xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Jun 9, 2026
xiangfu0 added a commit to pinot-contrib/pinot-docs that referenced this pull request Jun 9, 2026
…863)

## Summary
- explain how multi-stage brief segment plans group distinct per-segment
plans into `Alternative(segments=[n])` branches
- note that equivalent plans still collapse into one branch with
additive stats summed, while the single-plan case still renders without
the wrapper

## Source cross-check
-
`pinot-query-planner/src/main/java/org/apache/pinot/query/planner/explain/ExplainNodeSimplifier.java`
-
`pinot-query-planner/src/test/java/org/apache/pinot/query/planner/explain/ExplainNodeSimplifierTest.java`

## Validation
- `git diff --check`
cypherean pushed a commit to cypherean/pinot that referenced this pull request Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants