gortiz opened a new pull request, #18696:
URL: https://github.com/apache/pinot/pull/18696
## 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](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
