andygrove opened a new issue, #5831:
URL: https://github.com/apache/datafusion-comet/issues/5831
### What is the problem the feature request solves?
Comet's native plan operators hand-write `equals` and `hashCode` so that
`nativeOp`, `originalPlan`
and `serializedPlanOpt` stay out of plan identity. Whenever one of those
overrides omits a field
that changes results, `ReuseExchangeAndSubquery` shares a shuffle between
plans that compute
different things and the query returns one branch's rows twice.
We have hit this three times in three separate passes over the same family
of overrides:
- #5470 fixed `CometHashAggregateExec`, which omitted `resultExpressions`.
- #5824 found `joinType` missing from all three join operators, and `outer`
never captured onto
`CometExplodeExec` at all.
- While fixing #5824, #5828 found a third instance:
`CometBroadcastHashJoinExec` did not carry
`isNullAwareAntiJoin`, so a `NOT IN` branch reused a `NOT EXISTS` shuffle
and dropped the NULL
probe row. `joinType` alone does not separate those two, because both are
`LeftAnti` with
`BuildRight`, no condition, and identical children.
Every one of these was found by a manual read of the operator family, and
every pass missed
something the next pass caught. They are silent wrong-answer bugs on stock
configuration, so the
cost of missing one is high and there is no reason to believe the manual
reads have converged.
### Describe the potential solution
A test that reflects over the `CometNativeExec` subclasses and asserts that
every primary
constructor parameter is either referenced by that class's `equals` or named
on an explicit
exclusion list. Adding a field to an operator would then fail until the
author consciously decides
whether it belongs in plan identity.
The exclusion list needs at least:
- `nativeOp` and `originalPlan`, which are excluded by design.
`CometNativeExec.canonicalizePlans`
nulls `originalPlan` out, and `nativeOp` is per-instance serialization
state.
- `outputOrdering` on `CometSortExec`, `CometBroadcastNestedLoopJoinExec`,
`CometHashJoinExec`,
`CometBroadcastHashJoinExec` and `CometSortMergeJoinExec`. It is derived
from parameters that are
already compared, so it belongs on the list rather than in `equals`.
One wrinkle for whoever picks this up: the operators in `operators.scala`
exclude `originalPlan`
from `equals`, but `CometBroadcastExchangeExec` and `CometNativeScanExec` do
the opposite. They
compare `originalPlan` in place of the individual fields and override
`doCanonicalize` to match.
Both conventions are sound, so the guard has to accommodate the second
rather than assume the
first, otherwise it will report false positives on the scans and the
broadcast exchange.
### Additional context
I swept the whole family by hand against #5828 and no live instance remains,
so this is preventive
rather than a fix for a known wrong-answer bug.
Filing this separately so #5828 can stay focused on the three fixes.
--
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]
