apache / apache/datafusion-comet

Adding a New Operator guide omits the equals/hashCode requirement for Comet plan operators

Open
#5,832 1 comment 0 reactions 1 assignee Claimed by @rich7420 View on GitHub
documentation enhancement requires-triage
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.