Skip to content

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

Description

@andygrove

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions