apache / apache/datafusion-comet
Adding a New Operator guide omits the equals/hashCode requirement for Comet plan operators
- 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?
`docs/source/contributor-guide/adding_a_new_operator.md` never mentions `equals`, `hashCode`,
`stringArgs`, canonicalization or exchange reuse. A contributor who follows it end to end has no
reason to know the override exists, let alone that omitting a field from it produces silent wrong
answers.
The worked examples make this worse rather than neutral. The guide's `CometFilterExec` and
`CometProjectExec` case classes in Step 2 show only `outputPartitioning`, `outputOrdering`,
`producedAttributes` and `withNewChildInternal`. The real classes in
`spark/src/main/scala/org/apache/spark/sql/comet/operators.scala` also override `stringArgs`,
`equals` and `hashCode`. Someone copying either example verbatim gets the default case-class
`equals`, which pulls `nativeOp`, `originalPlan` and `serializedPlanOpt` into plan identity. The
guide gives no signal that anything is missing.
We have shipped the opposite defect three times: #5470 (`resultExpressions` omitted from
`CometHashAggregateExec`), #5824 (`joinType` omitted from all three join operators, `outer` never
captured onto `CometExplodeExec`), and a third instance found while fixing that in #5828
(`isNullAwareAntiJoin` on `CometBroadcastHashJoinExec`). All three returned wrong results on stock
configuration.
### Describe the potential solution
Two changes to `adding_a_new_operator.md`:
1. Complete the `CometFilterExec` example in Step 2 with its real `stringArgs`, `equals` and
`hashCode` overrides, so the copy-paste path produces a correct operator.
2. Add a short subsection covering the rule itself:
- Every constructor parameter that changes the operator's results must appear in both `equals`
and `hashCode`.
- `nativeOp`, `originalPlan` and `serializedPlanOpt` are deliberately excluded, with a sentence
on why. They are per-instance serialization state, and `CometNativeExec.canonicalizePlans`
nulls `originalPlan` out during canonicalization.
- Getting this wrong does not fail loudly. `ReuseExchangeAndSubquery` shares a shuffle between
the two plans and the query returns one branch's rows twice, usually only on inputs where an
unrelated optimizer rule is not already making the two subtrees differ.
- Note that `CometBroadcastExchangeExec` and `CometNativeScanExec` use the other valid
convention, comparing `originalPlan` in place of the individual fields and overriding
`doCanonicalize` to match, so a reader who greps for a second example is not confused by it.
Step 6 (Add Tests) is also worth a line: if the new operator carries any field beyond its children
and output, it wants an exchange-reuse regression modelled on the ones in `CometAggregateSuite`,
`CometJoinSuite` and `CometGenerateExecSuite`. Those tests need to defeat whatever optimizer rule
would otherwise make the two branches differ, which is the part that is easy to get wrong and
produce a test that passes for the wrong reason.
### Additional context
#5831 tracks the automated guard for the same problem. The two are complementary. The guard catches
the omission at build time, the guide stops a contributor writing it in the first place.
Contributor guide
Assessment
This issue has not been assessed yet.