andygrove opened a new issue, #5832:
URL: https://github.com/apache/datafusion-comet/issues/5832
### 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.
--
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]
