peter-toth commented on code in PR #58335:
URL: https://github.com/apache/spark/pull/58335#discussion_r3880264752
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/GroupPartitionsExec.scala:
##########
@@ -59,7 +58,7 @@ case class GroupPartitionsExec(
child: SparkPlan,
@transient joinKeyPositions: Option[Seq[Int]] = None,
@transient expectedPartitionKeys:
Option[Seq[(InternalRowComparableWrapper, Int)]] = None,
- @transient reducers: Option[Seq[Option[Reducer[_, _]]]] = None,
+ @transient reducers: Option[Seq[Option[KeyReducer]]] = None,
Review Comment:
**Finding 8** (my numbering) — an addition to this, measured on
`c01535cc6ed`.
The normalization does not fire in the two shapes this PR fixes. Since the
third commit stores the raw target transform, `reducedExpression` is the
*other* join side's expression: `t` for the identity-vs-transform reducer, `e2`
for the single-side-transform one. `QueryPlan.normalizeExpressions(expr,
child.output)` rewrites an attribute only when `child.output` contains its
exprId, and this node's child is the reduced side, so the other side's
attribute is left untouched. It fires only for the both-sides-reduce shape,
where `reducedExpression` is this side's own `e1` — the one shape whose
expression is stale anyway.
Measured with two `GroupPartitionsExec`s over `LocalTableScanExec(id#1)` and
`LocalTableScanExec(id#2)`, a value-equal `BucketReducer(2)`, and
`reducedExpression` built over a third attribute standing in for the other side
(`oid#11` / `oid#12`):
```
PROBE other-side attribute in reducedExpression, canonical equal = false
```
The same pair with `reducedExpression` over the node's own attribute — the
shape the new test builds — gives `true`.
There is a second, independent cause. The synthesized identity reducer is
`new Reducer[Any, Any] {...}`, so `KeyReducer.equals` compares it by reference.
Taking the reducer from `reducersBothWays` for an `identity(id)` / `bucket(2,
id)` pair, with everything else structurally identical:
```
PROBE identity-derived reducer, canonical equal = false
```
That half is pre-existing — the anonymous reducer was already a constructor
field of `GroupPartitionsExec` before this PR — so "with value-equal reducers"
in the commit message is accurate. It does mean the identity-vs-transform
shape, this PR's headline case, still does not deduplicate.
Fix shape: normalize `reducedExpression` against its own references instead
of the child's output — the scaladoc already says the attribute it carries is
not load-bearing, so a positional canonical form is enough — and make the
synthesized reducer a named case class over the target transform so it compares
by value, normalizing that expression too, or the exprId problem you just fixed
reappears inside the reducer. Extending the new test with an identity-derived
reducer, and with a `reducedExpression` over an attribute the child does not
output, would pin both.
--
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]
