apache / apache/datafusion-comet

Add a guard test that every CometNativeExec constructor parameter participates in equals

Open
#5,831 0 comments 0 reactions 0 assignees View on GitHub
enhancement requires-triage test
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

### 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. `CometNativeScanExec` overrides
`doCanonicalize` to match, keeping a canonicalized `originalPlan`, so that convention is sound and
the guard has to accommodate it rather than assume the first one, otherwise it will report a false
positive on the scan.

`CometBroadcastExchangeExec` is a different story, and I had this wrong when I first filed. Its
`doCanonicalize` nulls `originalPlan` out while deliberately keeping `mode`, so after
canonicalization `equals` reduces to `child` alone and `mode` never participates in identity. That
is a live omission rather than a second sound convention, and the guard should be expected to flag
it.

It is reachable. Two broadcast hash joins over the same scan with build keys `r._1` and `r._2`
produce different `HashedRelationBroadcastMode`s over an identical child. For that query Comet
emits one `CometBroadcastExchangeExec` plus a `ReusedExchangeExec`, under AQE both on and off,
where the same query with Comet disabled reuses nothing. Results are correct in every arm, because
Comet broadcasts serialized Arrow batches and the native join builds its own hash table from the
serialized join keys, so the payload really is mode independent. So this one is a divergence from
Spark's reuse behavior rather than a wrong answer. It is still exactly the shape of omission this
guard exists to surface, and a guard written against the original wording of this issue would have
been told to skip the one class that has a gap.

### Additional context

I swept the whole family by hand against #5828 and no live wrong-answer instance remains, so this
is preventive rather than a fix for a known incorrect-results bug. The `mode` omission on
`CometBroadcastExchangeExec` described above is the one real gap and it does not produce wrong
results today.

Filing this separately so #5828 can stay focused on the three fixes.

Contributor guide

Open the contributing guide

Research direction

Start in operators.scala and inspect CometNativeExec subclasses, their primary constructor parameters, equals implementations, and doCanonicalize methods. Trace the existing canonicalization conventions for CometNativeScanExec and CometBroadcastExchangeExec before defining the guard’s exclusions. Done means the test covers the operator family, accepts deliberate exclusions, and flags the CometBroadcastExchangeExec mode omission.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala, spark
Domain
distributed-systems, testing-qa
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.