LuciferYang commented on code in PR #59209:
URL: https://github.com/apache/spark/pull/59209#discussion_r4204437744
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +67,16 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // The children of a `CreateVariable` are the `ResolvedIdentifier`s
naming the variables, not
+ // rows to evaluate the default over, and `V2CommandStrategy` plans the
command only while
+ // they stay so. `CreateVariableExec` evaluates the default once, so a
`With` in it is left in
+ // place to memoize its definition, as one in a conditional branch is.
Review Comment:
Confirmed: with the counting UDF your LIKE default calls it 3 times, against
once in a SELECT. The comment now says a later rule that copies the `With`,
such as `LikeSimplification`, makes each copy evaluate the definition, and the
description no longer says once. The single-pattern case also duplicates a
plain nondeterministic input: with `pick()` returning `'ay'` or `'bz'` at
random, `pick() LIKE 'a%z'` over 1000 rows gave 256 trues in one run on master,
where 0 is right. Filed as SPARK-60028.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +67,16 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // The children of a `CreateVariable` are the `ResolvedIdentifier`s
naming the variables, not
+ // rows to evaluate the default over, and `V2CommandStrategy` plans the
command only while
+ // they stay so. `CreateVariableExec` evaluates the default once, so a
`With` in it is left in
+ // place to memoize its definition, as one in a conditional branch is.
+ case c: CreateVariable if c.defaultExpr.containsPattern(WITH_EXPRESSION)
=>
+ val default = c.defaultExpr.child
+ val rewritten =
default.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case w: With => inlineDefsThatGainNothing(w)
+ }
+ if (rewritten eq default) c else c.copy(defaultExpr =
c.defaultExpr.copy(child = rewritten))
case p if p.expressions.exists(_.containsPattern(WITH_EXPRESSION)) =>
Review Comment:
Fixed by #59217 (SPARK-59962), which inlines a command's definitions instead
of hoisting them. With it, your ALTER followed by an INSERT succeeds; on master
the INSERT fails with UNRESOLVED_COLUMN on `_common_expr_1`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +67,16 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // The children of a `CreateVariable` are the `ResolvedIdentifier`s
naming the variables, not
+ // rows to evaluate the default over, and `V2CommandStrategy` plans the
command only while
+ // they stay so. `CreateVariableExec` evaluates the default once, so a
`With` in it is left in
+ // place to memoize its definition, as one in a conditional branch is.
+ case c: CreateVariable if c.defaultExpr.containsPattern(WITH_EXPRESSION)
=>
+ val default = c.defaultExpr.child
+ val rewritten =
default.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case w: With => inlineDefsThatGainNothing(w)
+ }
+ if (rewritten eq default) c else c.copy(defaultExpr =
c.defaultExpr.copy(child = rewritten))
case p if p.expressions.exists(_.containsPattern(WITH_EXPRESSION)) =>
applyInternal(p)
Review Comment:
Fixed by #59217, which has this case in `DeleteFromTableSuiteBase` and pins
the runtime group filter with the copied-row count.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +67,16 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // The children of a `CreateVariable` are the `ResolvedIdentifier`s
naming the variables, not
Review Comment:
Fixed by #59217, covered in `GeneratedColumnWriteSuite`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -67,6 +67,16 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
// changed anything: `mapExpressions` and `withNewChildren` preserve
reference equality when
// they rewrite nothing, which is what makes this detectable.
if ((rewrittenAgg eq agg) && (rewrittenProj eq proj)) p else
rewrittenProj
+ // The children of a `CreateVariable` are the `ResolvedIdentifier`s
naming the variables, not
+ // rows to evaluate the default over, and `V2CommandStrategy` plans the
command only while
+ // they stay so. `CreateVariableExec` evaluates the default once, so a
`With` in it is left in
+ // place to memoize its definition, as one in a conditional branch is.
+ case c: CreateVariable if c.defaultExpr.containsPattern(WITH_EXPRESSION)
=>
+ val default = c.defaultExpr.child
+ val rewritten =
default.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case w: With => inlineDefsThatGainNothing(w)
+ }
+ if (rewritten eq default) c else c.copy(defaultExpr =
c.defaultExpr.copy(child = rewritten))
case p if p.expressions.exists(_.containsPattern(WITH_EXPRESSION)) =>
applyInternal(p)
Review Comment:
With #59217 the ALTER no longer throws `ClassCastException`, and `DEFAULT
nullif(my_udf(1), 0)` gets the not-constant error. `nullif(nullif(1, 2), 3)`
still does not fold to 1, though: any NULLIF or BETWEEN column default, even
`nullif(1, 3)`, is rejected as not constant on master, on a v1 table as well,
because the default analysis replaces it with a `With` that `ConstantFolding`
cannot fold and never runs `RewriteWithExpression`. Filed as SPARK-60029.
--
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]
