peter-toth commented on code in PR #57956:
URL: https://github.com/apache/spark/pull/57956#discussion_r3770989619
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/PullOutVariantExtractions.scala:
##########
@@ -177,11 +177,20 @@ object PullOutVariantExtractions extends
Rule[LogicalPlan] {
case _ => false
}
+ private def isJoinHoistable(e: Expression): Boolean = {
Review Comment:
**Finding 4.** @qlong's worry that there'd be no signal if
`Cast`/`VariantGet` get classified as throwable later is well founded, and I
think there's a sharper version of it: the flag isn't this rule's to define.
`CombineFilters` (`Optimizer.scala:2039`) and `PushPredicateThroughJoin`
(`:2475`, `:2507`) both branch on `!cond.throwable`, so nobody can set
`Cast.throwable = true` or `VariantGet.throwable = true` in order to activate
this gate -- that would change filter merging and join-condition pushdown for
every query at the same time. So the hook can't really be honored on purpose
for variant shredding, and in the other direction, if the flag is ever flipped
for an unrelated reason this rule silently stops shredding across joins as a
side effect. `ExprUtils.scala:244-246` records a similar reservation for a
different caller: "this deliberately does not rely on `Expression.throwable`,
which is opt-in metadata that most expressions do not override."
That makes @qlong's suggested test worth more than a minor nit, and I'd also
add a comment next to `isJoinHoistable` saying the check is inert today and
what would make it fire, because right now it reads as a live guard. For the
record on how inert it is: `throwable` defaults to
`children.exists(_.throwable)` (`Expression.scala:169`) and `Sequence`
(`collectionOperations.scala:3479`) is the only override in all of `sql/`,
while `isHoistable` only admits an `Attribute`/`GetStructField` chain plus
foldable `Literal`s -- so `isJoinHoistable` is currently exactly `isHoistable`.
--
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]
