LuciferYang commented on code in PR #58045:
URL: https://github.com/apache/spark/pull/58045#discussion_r3821187764
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RewriteWithExpression.scala:
##########
@@ -183,6 +215,15 @@ object RewriteWithExpression extends Rule[LogicalPlan] {
val newExpr = c.withNewAlwaysEvaluatedInputs(newAlwaysEvaluatedInputs)
// Use transformUp to handle nested With.
newExpr.transformUpWithPruning(_.containsPattern(WITH_EXPRESSION)) {
+ case w @ With(_, defs) if defs.exists(shouldPreEvaluateInBranch(_,
commonExprIdSet)) =>
Review Comment:
Thanks for the pointer on lazy memoization. I went and tried the closest
thing reachable in the plan, and it does not hold up. Reporting the negative
result here since it rules out a direction rather than just failing to reach
one.
**The guard experiment.** The idea was to keep the eager pre-evaluation but
wrap the column in `If(branch is reached, definition, default)`, so a stateful
generator is only advanced on the rows that reach the branch. Once-when-taken,
zero-times-otherwise, expressed in the plan.
It breaks on subexpression elimination. A guard in a plain `Project` is
outside conditional evaluation, so to CSE it is an `alwaysEvaluatedInputs` of
the `If`: a part repeated across guards gets hoisted to the top of the
projection and evaluated up front
(`spark.sql.subexpressionElimination.skipForShortcutExpr` is false by default).
A condition that can raise then raises on rows the branch never reached:
```sql
SELECT CASE WHEN a = 0 THEN false
WHEN 6 / a > 2 THEN rand(1) BETWEEN 0 AND 1
WHEN 6 / a < -2 THEN rand(2) BETWEEN 0 AND 1
ELSE false END
FROM t -- t contains a = 0
```
gives `DIVIDE_BY_ZERO`, frame in `project_subExpr_0$`. Limiting how many
guards a projection may hold does not help, since a single guard repeating `6 /
a` internally does the same.
The only fix I found is to build guards out of nodes that cannot raise, and
that leaves very little:
| condition | guard |
|---|---|
| `a > 0`, `a > b`, `a IS NULL`, `a IN (1,2,3)`, `s = 'x'` | kept |
| `a + b > 0`, `a * 2 > b`, `abs(a) > 0`, `upper(s) = 'X'`, `cast(a as long)
> 0` | dropped |
Any arithmetic, function call or cast on a column loses it, and `WHEN a + b
> 0` is an ordinary thing to write. It is also dropped when the guard reads the
other side of a join, when the condition carries a subquery, when a preceding
condition is nondeterministic, and for conditional expressions outside
`If`/`CaseWhen`/`Coalesce`/`NaNvl`. All silently. A mechanism that only applies
to plain column comparisons and fails invisibly elsewhere does not deliver the
property you asked for, so I left it out of the PR rather than shipping a
version of it.
**On excluding stateful generators.** I don't think that can be satisfied
together with the fix. The generators whose extra evaluations are observable
are exactly the ones that produce the wrong result: `rand`, `randn`, `uuid`,
`monotonically_increasing_id`. What remains after excluding them —
`spark_partition_id`, `input_file_name`, the input-file-block pair — is
constant within a task, so evaluating it twice was never wrong to begin with.
Excluding them removes the fix rather than narrowing it, and the end-to-end
test in `ColumnExpressionSuite` would have to go with it.
For what the eager path does cost: Spark makes no commitment about which row
draws which value from a seeded generator. The seed is `seed + partitionIndex`,
so the sequence already moves with partitioning, splits and AQE, and
`spark.sql.alwaysInlineCommonExpr` already makes it configuration-dependent.
There is precedent for shipping that kind of shift as a migration note — since
4.3 nondeterministic filters are no longer pushed to DSv2, for the same class
of reason. Happy to add a migration-guide entry here if you want one.
**Lazy memoization itself.** Filed as SPARK-58902 with the shapes it would
fix. It is worth more than the guard was: the allowlist in this PR turns
expressions down by shape rather than behavior, so `randstr(3, 0) BETWEEN 'a'
AND 'b'` inside a branch still gets two independent draws today even though
only `randstr(-1, 0)` raises. Memoization would fix that class in one go, and
it would also close the pre-existing join TODO in this rule. It needs an
evaluable expression with per-row caching plus codegen support, which `With` /
`CommonExpressionDef` / `CommonExpressionRef` cannot be today, so it is a
separate change rather than something to fold in here.
Your call on the scope of this PR.
--
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]
