cloud-fan commented on code in PR #56575:
URL: https://github.com/apache/spark/pull/56575#discussion_r3463167125
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala:
##########
@@ -454,19 +454,59 @@ trait RuntimeReplaceable extends Expression {
override val nodePatterns: Seq[TreePattern] = Seq(RUNTIME_REPLACEABLE)
override def nullable: Boolean = replacement.nullable
override def dataType: DataType = replacement.dataType
+ // The actual evaluation is delegated to `replacement`, so determinism must
reflect `replacement`,
+ // not this expression's `children` (which are the original arguments). For
example, the children
+ // of `Uniform` are literal bounds and a seed (all deterministic), while its
`replacement` is a
+ // non-deterministic `Rand`. This matters once a `RuntimeReplaceable` may
survive into the
+ // physical plan (see `eagerReplace`): the survival decision relies on an
accurate determinism
+ // signal.
+ override lazy val deterministic: Boolean = replacement.deterministic
Review Comment:
Good catch, fixed in bd8f0ee.
`RuntimeReplaceable` now does `override lazy val throwable: Boolean =
replacement.throwable`, alongside the existing `deterministic`/`foldable`
delegations. You're right that the inherited `children.exists(_.throwable)` was
wrong for a survivor: the children are the original arguments (non-throwing
literal bounds) while the `replacement` is where the throwing `Sequence` lives,
so `PushPredicateThroughJoin` (the SPARK-46707 guard) would relocate the
predicate below the join and surface `Illegal sequence boundaries` on rows the
join would otherwise discard.
Added a `FilterPushdownSuite` regression test mirroring the existing
SPARK-46707 join test: a wrapper whose children (`x.a`, `x.b`) are non-throwing
but whose `replacement` is an explicit-step `Sequence`; the predicate must stay
above the join. It fails without the delegation (the wrapper is pushed onto the
left input) and passes with it.
One implementation note for whoever reviews the test: the wrapper is
deliberately **not** an `InheritAnalysisRules` expression. With
`InheritAnalysisRules` the `replacement` becomes the wrapper's child, so the
inherited `children.exists(_.throwable)` would already observe the throwing
`Sequence` and the test would pass even without the fix. The test wrapper keeps
its original args as children (the `Uniform` / `MultiGetJsonObject` shape) so
it actually exercises the gap.
--
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]
