cloud-fan commented on code in PR #57157:
URL: https://github.com/apache/spark/pull/57157#discussion_r3560213235
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1135,6 +1135,13 @@ object ColumnPruning extends Rule[LogicalPlan] {
.map(_._2)
p.copy(child = g.copy(child = newChild, unrequiredChildIndex =
unrequiredIndices))
+ // prune unused pass-through columns from the child of BinBy. The range
and DISTRIBUTE inputs
+ // the kernel reads are kept via `b.references`; only fully-unused
pass-throughs are dropped.
+ case p @ Project(_, b: BinBy)
Review Comment:
This arm is correct, but it looks functionally redundant with the generic
`Project(_, child)` fallback at `:1199`.
`Project(_, BinBy)` isn't intercepted by any arm between here and `:1199`:
`GeneratorNestedColumnAliasing` (`:1146`) matches only `Project(_, g:
Generate)`, and `NestedColumnAliasing` (`:1195`) matches `Project(projectList,
child)` only when `canProjectPushThrough(child)` or `canPruneOn(child)` — and
`BinBy` is in neither list (`NestedColumnAliasing.scala:199-219`). So without
this arm, `Project(_, BinBy)` falls through to the generic fallback:
```scala
case p @ Project(_, child) if !child.isInstanceOf[Project] =>
val required = child.references ++ p.references
...
val newChildren = child.children.map(c => prunedChild(c, required))
```
For `child = b: BinBy` that computes `required = b.references ++
p.references` and calls the same `prunedChild(b.child, required)`. Compared to
this arm's `requiredAttrs = (p.references -- b.producedAttributes) ++
b.references`: `b.producedAttributes` (scaled DISTRIBUTE + appended attrs) all
carry fresh `ExprId`s, so none are in `b.child.output`; subtracting them from
`p.references` can't change which child attributes survive `prunedChild`'s
filter. The two reference sets prune the child identically, and the two guards
fire on the same plans.
Unlike the `Generate` arm (which must set `unrequiredChildIndex`, a field
the generic fallback can't populate) or the `Expand` arm (which rewrites
projections), this arm does no irreducible work beyond `prunedChild` — so it
appears eliminable. The new `ColumnPruningSuite` test would also still pass
with the arm deleted, so it doesn't actually distinguish the arm from the
fallback.
Is there a reason to keep the explicit arm — a planned future field on
`BinBy`, or a deliberate choice to make the pruning discoverable at this call
site? If not, consider dropping it and relying on the generic fallback.
Non-blocking either way.
--
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]
