yashmayya opened a new pull request, #19414: URL: https://github.com/apache/pinot/pull/19414
Follow-up to #19219, implementing the reflective guard test @gortiz asked for there. ## The problem it guards `EquivalentStagesFinder.NodeEquivalence` decides whether two stages are interchangeable for `useSpools=true` by comparing node fields **by hand**. That list is maintained separately from `PlanNode.equals`, nothing connects them, and a missing comparison only makes the check *more permissive* — so the failure mode is silent wrong results, never an error. Five fields have now reached a plan node without reaching the equivalence check: | Field | Landed in | |---|---| | `ignoreNulls` | #14264 | | `matchCondition` | #15630 | | `exclude` | #18482 | | `groupingSets` | #18817 | | `passthroughInputIndexes` / `prunedPassthrough` | #18782 | Four were fixed one at a time in #19219. Each fix was correct, but the mechanism guarantees a sixth. `NodeEquivalenceFieldCoverageTest` walks the declared fields of every type the decision reads and requires each one to be listed. A new field fails the test until someone either makes the check compare it, or records it in `NOT_COMPARED` with a reason. The failure message names the file to edit. `NOT_COMPARED` also replaces the commented-out fields that used to carry this reasoning inside the check. Those could never fail once the reasoning stopped holding — and one of them had already rotted: it referenced `getReceiverStageId()`, which no longer exists (the field is now a `BitSet _receiverStages`). ## Why A (classification) and not a fully generic B (mutation) The stronger design is behavioural: for each field, build two nodes differing only in that field and assert the check reports them as not equivalent. That proves the comparison *works*, where this test only proves a decision was *made*. I did not do it generically, for three reasons. **1. A generic mutation cannot pick a valid value.** The fields are coupled. `groupingSets` holds indexes into `groupKeys`; `elementIndexes`, `ordinalityIndex` and `passthroughInputIndexes` hold indexes into the schema; `DataSchema` must line up with the inputs. Generic values for these produce invalid nodes, so the "value that differs" has to be chosen per field by someone who knows the field — which is a hand-written test. **2. It would have to construct, not mutate.** Java 25 blocks reflective writes to final fields, and every one of these fields is final. So B needs a per-class builder for each node type — i.e. `StagesTestBase` — at which point the reflective part is only telling you *that* a field exists, which is exactly what this test does. **3. The false green is a real trap, not a theoretical one.** If the varied field also changes the `DataSchema`, `areBaseNodesEquivalent` rejects the pair first: the test passes without ever reaching the field's own comparison. This bit me in #19219 — my first `Spool.json` case passed *without* the fix, because an extra join condition widened one side's exchange keys and the pair was rejected on `getKeys()` instead of on `exclude`. A generic harness would hit that silently and at scale, and a green suite that proves nothing is worse than no suite. So the split is: reflection does completeness, which is cheap and has no false failures; `EquivalentStagesFinderTest` keeps doing behaviour case by case, where the value can be chosen correctly and the discriminator verified. Every negative test there has been checked to fail when its comparison is deleted. **What this test cannot do:** it cannot tell a correct comparison from a missing one. Someone in a hurry can add a field name to `FIELDS` and write no comparison. That is strictly better than today, where adding the field triggers nothing at all — the diff now shows a reviewer that a comparison decision was made — but it is not proof. ## The fields the guard surfaced @gortiz predicted the guard would flag two things, and it did. Both are resolved here rather than left as known gaps: - **`visitUnnest` unrolled three of `TableFunctionContext`'s five fields**, missing `passthroughInputIndexes` and `prunedPassthrough`. Now it compares the context as a whole, so it stays in step with the fields it holds. This is the only behavioural change in the PR, and it comes with paired tests (verified to fail when reverted). - **`MailboxSendNode.isSort()` / `getHashFunction()`** are recorded as opt-outs with verified reasons: sending-side sort is unimplemented (`MailboxSendOperator` still has `// TODO: Support sort on sender`) and a receiver-visible difference is already compared in `visitMailboxReceive`; and one hash function is threaded through an entire v1 plan, so two send nodes in the same plan always agree on it. The guard also surfaced a third, `EnrichedJoinNode.getJoinResultSchema()`, likewise recorded as an opt-out: only `PlanNodeDeserializer` builds an `EnrichedJoinNode`, so the broker-side planner that runs this check never sees one, and the class is deprecated for removal in 1.6.0. I originally added comparisons for all three. I reverted them because none can be tested in isolation — the exchange builder threads one `sort` flag into both the send and the receive node, so a test would pass on the receive-side comparison that already exists; and adding an untested comparison to a PR whose thesis is "don't let comparisons drift untested" is self-defeating. A documented opt-out that names the claim it rests on is the honest version, and the guard now fails if any of these fields is renamed or removed. ## Scope of the registry The boundary is *every type this module owns that the decision reads* — the plan nodes, plus the value types they hold that are compared through `equals` (`RexExpression.InputRef` / `Literal` / `FunctionCall`, `PlanNode.NodeHint`, `UnnestNode.TableFunctionContext`, `EnrichedJoinNode.FilterProjectRex` and its nested schema holder). It deliberately stops at the module edge, so `DataSchema` and `RelFieldCollation` are out. Drawing the line at "types where we already got burned" would have been too narrow: the motivating regression, `_ignoreNulls` (#14264), was itself a field on a nested value type. ## Verification - Guard bites, both ways: adding a field to `WindowNode` fails `registeredFieldsMatchDeclaredFields`; dropping a registry entry fails `everyVisitedNodeTypeIsRegistered`. Both with actionable messages. - The two new `visitUnnest` tests fail when that comparison is reverted. - Passes under `-Pcodecoverage`. The field walk skips synthetic and static fields on purpose — Jacoco adds `$jacocoData` to instrumented classes, and `RexExpression.Literal` holds `public static final TRUE`/`FALSE`, so without the filter this would fail only in the coverage build. - `pinot-query-planner` (1544 tests) and `ResourceBasedQueriesTest` (3685) green. spotless / checkstyle / license clean. ## Not in scope `PlanNodeMerger` has the same three hand-written field lists and the same drift, including the identical `visitUnnest` unrolling. It only affects `EXPLAIN` rendering, not query results, so I left it out to keep this focused — happy to extend the guard to cover both comparators 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]
