david-mollitor-db opened a new pull request, #58724:
URL: https://github.com/apache/spark/pull/58724
### What changes were proposed in this pull request?
`ExpressionEquals` is the key type of `EquivalentExpressions.equivalenceMap`
(`mutable.HashMap[ExpressionEquals, ExpressionStats]`), the map that drives
common-subexpression
elimination (CSE). Its `hashCode` was:
```scala
override def hashCode: Int = Objects.hash(e.semanticHash(): Integer, height:
Integer)
```
`java.util.Objects.hash(...)` is varargs, so every call boxes both ints to
`Integer` and allocates an
`Object[2]`. This PR computes the hash directly, with no boxing or array
allocation:
```scala
override def hashCode: Int = {
val prime = 31
var result = 1
result = prime * result + e.semanticHash()
result = prime * result + height
result
}
```
While here, `equals` is reordered to test the O(1) `height` before the
recursive `semanticEquals`,
and the now-unused `import java.util.Objects` is removed.
### Why are the changes needed?
`ExpressionEquals.hashCode` is invoked on every `addExpr` /
`updateExprInMap` / `getExprState`
lookup during CSE, so expression-heavy queries generate a steady stream of
short-lived `Object[2]` +
boxed-`Integer` allocations during optimization/codegen. Computing the hash
directly removes that
per-lookup allocation.
The new form reproduces the **exact** value the old
`Objects.hash(e.semanticHash(), height)`
produced: `Objects.hash` delegates to `java.util.Arrays.hashCode`, which
starts `result` at 1 and
folds each element as `prime * result + element.hashCode()` (and
`Integer.hashCode` is the int
value). So the hash value -- and therefore the bucket distribution -- is
unchanged.
The `equals` reorder is behavior-preserving: `semanticEquals` implies equal
`height`, so testing the
cheap `height` first short-circuits an unequal comparison without walking
the expression tree, and
`&&` yields the identical boolean. `equals` runs only on hash-bucket
collisions, which is exactly
where the cheap pre-check helps.
### Does this PR introduce _any_ user-facing change?
No. This is an internal optimization with no behavior change.
### How was this patch tested?
Added a `SubexpressionEliminationSuite` test asserting the new `hashCode`
equals
`Objects.hash(e.semanticHash(): Integer, height: Integer)` across several
expressions (varying
`semanticHash` and `height`), pinning the byte-for-byte equivalence. Existing
`SubexpressionEliminationSuite` and `SubExprEvaluationRuntimeSuite` pass
unchanged; scalastyle is
clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
This pull request and its description were written by Isaac.
--
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]
