cloud-fan commented on code in PR #57157:
URL: https://github.com/apache/spark/pull/57157#discussion_r3553418896
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1135,6 +1135,17 @@ object ColumnPruning extends Rule[LogicalPlan] {
.map(_._2)
p.copy(child = g.copy(child = newChild, unrequiredChildIndex =
unrequiredIndices))
+ // prune unrequired pass-through columns from child of BinBy.
+ case p @ Project(_, b: BinBy) if p.references != b.outputSet =>
Review Comment:
The rewrite here is correct — this is a simplification suggestion, not a
correctness concern.
The `unrequiredChildIndex` field (plus `requiredChildOutput` and the
`output` override on `BinBy`) exists to exclude kernel-read columns from
`output` while keeping them in the child. But the set it can ever exclude is
provably bounded:
```
unrequired = b.references -- distributeColumns -- p.references
= {rangeStart, rangeEnd} -- p.references
```
`rangeStart`/`rangeEnd` are single `Attribute`s (not `Seq`s), and the
operator assert forces both to `Timestamp`/`TimestampNTZ` (8-byte fixed-width).
So the whole mechanism saves **at most two 8-byte timestamps** (<=16 bytes/row)
from the fan-out, and only when the query doesn't select the range columns —
while `BinBy` already emits `bin_start`/`bin_end`/`bin_distribute_ratio` +
scaled DISTRIBUTE columns unconditionally.
The `Generate` analogy that justifies the field there doesn't transfer:
`Generate` excludes `generator.references`, which are arbitrary-width (arrays,
structs, long strings), so pruning them from the fan-out pays off. `BinBy`'s
excluded columns are structurally capped at two fixed-width timestamps.
Suggest dropping the field, `requiredChildOutput`, and the `output`
override, and keeping only the child-pruning arm:
```scala
// prune unused pass-through columns from the child of BinBy.
case p @ Project(_, b: BinBy) if
!b.child.outputSet.subsetOf(p.references -- b.producedAttributes ++
b.references) =>
val requiredAttrs = p.references -- b.producedAttributes ++
b.references
p.copy(child = b.copy(child = prunedChild(b.child, requiredAttrs)))
```
This still prunes the fully-unused pass-through columns (`label` etc. — the
real win) via `prunedChild`; it only forgoes excluding the <=2 range columns
from `output`, which the outer `Project` trims anyway. `BinBy.output` reverts
to `child.output.map(replace) ++ appendedAttributes`. Trade: less operator
surface (a positional field every call site and future match must thread) vs.
<=16 bytes/row weaker fan-out pruning and losing byte-identical parallelism
with `Generate`. Non-blocking — both are result-identical, so keeping the
`Generate`-parallel form for consistency is a reasonable call if you prefer it.
--
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]
