yashmayya opened a new pull request, #19219:
URL: https://github.com/apache/pinot/pull/19219
## What
`EquivalentStagesFinder` decides whether two stages are interchangeable for
the spool optimizer (`useSpools=true`) by comparing plan-node fields. Several
semantically significant fields were missing from those comparisons, so stages
that compute *different* values compared as equivalent.
`EquivalentStagesReplacer` then drops one stage and serves the survivor's rows
to both consumers — silent wrong results, no error.
Three of these are reachable and produce wrong query results:
| Field | Why the other checks don't catch it |
|---|---|
| `WindowNode.getExclude()` | The frame exclusion changes which rows feed
the window function. `SUM(x) OVER (... EXCLUDE CURRENT ROW)` and the same
window with `EXCLUDE NO OTHERS` agree on every other compared field. |
| `AggregateNode.getGroupingSets()` | `getGroupKeys()` only holds the
*union* of the grouping columns, so `ROLLUP(a,b)` and `CUBE(a,b)` agree on
group keys, agg calls **and** data schema. |
| `JoinNode.getMatchCondition()` | ASOF joins require `nonEquiConditions` to
be empty and carry the whole comparison in `matchCondition`, so two ASOF joins
differing only on `>` vs `>=` compared as equivalent. |
All three are compared by the corresponding `PlanNode.equals()`; only the
equivalence check was missing them.
This also adds `_ignoreNulls` to
`RexExpression.FunctionCall#equals`/`#hashCode`, where it has been missing
since IGNORE NULLS was introduced in #14264. See the note below — unlike the
three above, this one is **not** currently reachable from SQL on its own.
## Repro
`Spool.json` gains an end-to-end case (H2-compared). Before the fix:
```
Mismatched value at row id: 0, column id: 3.
Expected Row: [1, null, 1, 1], Actual Row: [1, null, 1, null]
```
The `EXCLUDE NO OTHERS` window returned the `EXCLUDE CURRENT ROW` value
because the two stages were spooled together. Note the two windows must
partition on the same key — adding a join condition that widens one side's
exchange keys makes the stages differ on `getKeys()` and masks the bug.
## Tests
- `EquivalentStagesFinderTest`: negative cases for all four fields, each
verified to fail without its fix, plus matching positive cases (both `IGNORE
NULLS`, both `EXCLUDE CURRENT ROW`, same grouping sets, same match condition)
so the checks can't regress into blanket rejection.
- `Spool.json`: end-to-end value comparison, verified to fail without the
`getExclude()` fix. This suite previously had no window coverage.
## Note on IGNORE NULLS
The `FunctionCall#equals` change is a correctness-contract fix, not a live
bug fix, and I want to be explicit about why.
Window agg calls are the only ones that set `ignoreNulls` today
(`RexExpressionUtils.fromAggregateCall` hardcodes `false`). Two window stages
differing only in IGNORE NULLS never reach the equivalence check, because
Calcite collapses them earlier: `Window.Group#equals`/`hashCode` compare only
`digest`, and `computeString()` renders agg calls via `RexCall.toString()`,
which does not print IGNORE NULLS. So the two `LogicalWindow` nodes have
identical digests and the planner dedupes them.
That upstream behaviour is itself a wrong-results bug, and it is
**independent of `useSpools`**. On master:
```sql
SELECT w1.v, w2.v, b.col1 FROM
(SELECT col1, LAST_VALUE(col3) IGNORE NULLS OVER (PARTITION BY col2 ORDER
BY col1) AS v FROM a) w1
JOIN b ON w1.col1 = b.col1
JOIN (SELECT col1, LAST_VALUE(col3) RESPECT NULLS OVER (PARTITION BY col2
ORDER BY col1) AS v FROM a) w2
ON w2.col1 = b.col1
```
plans **both** windows with `ignoreNulls=true` with `useSpools=false`, and
whichever variant appears first wins (swapping the two subqueries flips both to
`ignoreNulls=false`). `Window.Group#exclude` *is* in the digest, which is
exactly why the EXCLUDE case survives Calcite and then gets mis-spooled instead.
I have not tried to work around that here — it needs a fix in Calcite's
`Window.Group` (or a Pinot-side digest override) and is a separate concern from
stage equivalence. Filing it separately. This PR makes the Pinot-side equality
contract correct so the spool path does not become a second way to hit the same
class of bug.
## Notes
- Not backward-incompatible: no wire, config or plan-format change. Both
fields already round-trip through `expressions.proto` and `plan.proto`. The
change is strictly *fewer* merges, so a mixed-version cluster executes either
plan shape correctly.
- `FunctionCall#equals` is shared beyond the spool path
(`AggregateNode`/`ProjectNode`/`FilterNode`/`WindowNode.equals`, and
`PlanNodeMerger`), so multi-stage `EXPLAIN` output also stops merging nodes
that differ only on IGNORE NULLS, regardless of `useSpools`.
- Exposure: `exclude` since #18482, `groupingSets` since #18817,
`matchCondition` since #15630, all gated on `useSpools` (off by default).
- `PlanNodeMerger.visitWindow`/`visitJoin`/`visitAggregate` have the same
omissions. Those only affect `EXPLAIN` rendering, not query results, so I left
them out to keep this focused — happy to fold them in if reviewers prefer.
--
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]
