[SPARK-59019][SQL] Make RewriteWithExpression non-excludable - #58323
[SPARK-59019][SQL] Make RewriteWithExpression non-excludable#58323shrirangmhalgi wants to merge 2 commits into
Conversation
Sorry, something went wrong.
shrirangmhalgi
left a comment
There was a problem hiding this comment.
@peter-toth / @cloud-fan could you PTAL - this PR adds RewriteWithExpression to nonExcludableRules. Without it, With nodes from RuntimeReplaceable replacements survive into codegen when excluded via config.
Thank you
Sorry, something went wrong.
peter-toth
left a comment
There was a problem hiding this comment.
Thanks for the PR, @shrirangmhalgi!
The fix looks right and minimal. ReplaceExpressions runs inside FinishAnalysis, which is already non-excludable, and it is what turns Between and NullIf into a With (the only two With producers in the tree). RewriteWithExpression is the only rule that removes With, and its batch holds just that rule, so excluding it drops the whole batch. ConvertToLocalRelation skips projections holding an Unevaluable, so nothing folds the node away and it reaches Unevaluable.doGenCode. I re-ran the new test with the nonExcludableRules line deleted: it fails with exactly the reported [INTERNAL_ERROR] Cannot generate code for expression: with(...), and passes with the line restored. Nothing blocking from me.
Non-blocking
- 1. Hardcoded rule name in the test: the test spells the rule out as a string while the fix uses RewriteWithExpression.ruleName. A rename would leave the test green while it stops guarding anything. [inline: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:5323]
- 2. Affected versions understate the reach: RewriteWithExpression shipped in 4.0.0 (SPARK-45760, 01c294b05f3, first tagged v4.0.0), so a BETWEEN query with the rule excluded also crashes on every 4.x. SPARK-59019 lists Affects Version 5.0.0 only. Please widen it, and say whether you want this backported.
Minor
- 3. Comment gives a property, not a consequence: the other explained entries in this list say what breaks if the rule is excluded; this one describes what With is. [inline: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:324]
Sorry, something went wrong.
| withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> | ||
| "org.apache.spark.sql.catalyst.optimizer.RewriteWithExpression") { |
There was a problem hiding this comment.
Finding 1. The rule name is a literal string here, while the fix at sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:325 uses RewriteWithExpression.ruleName. If the rule is ever renamed or moved packages, the fix keeps working and this string silently stops matching. excludedRules then excludes nothing, the test passes for the wrong reason, and the crash it guards is unprotected again. A compiler-checked reference removes that failure mode. This file already does it that way at line 3325 (ConvertToLocalRelation.ruleName), and the import at line 38 is the one to extend.
| withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> | |
| "org.apache.spark.sql.catalyst.optimizer.RewriteWithExpression") { | |
| withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> RewriteWithExpression.ruleName) { |
With RewriteWithExpression added to the org.apache.spark.sql.catalyst.optimizer import at line 38.
Sorry, something went wrong.
| // execution, so it must never be excludable. | ||
| ConvertToCatalyst.ruleName, | ||
| FinishAnalysis.ruleName, | ||
| // With is Unevaluable and must be rewritten before physical planning. |
There was a problem hiding this comment.
Finding 3. The other two entries in this list that carry a comment say what breaks when the rule is excluded: ConvertToCatalyst right above ("excluding it would leak that node into execution"), and CleanupDynamicPruningFilters in SparkOptimizer.nonExcludableRules. This comment describes With instead, so it reads as background rather than a reason to keep the rule. Naming the failure matches the neighbours and gives the next reader the string they would search for.
| // With is Unevaluable and must be rewritten before physical planning. | |
| // ReplaceExpressions (in FinishAnalysis) turns Between/NullIf into the Unevaluable | |
| // With expression; excluding this rule leaks it into codegen and fails with INTERNAL_ERROR. |
Sorry, something went wrong.
### What changes were proposed in this pull request? Add `RewriteWithExpression` to the `nonExcludableRules` list in `Optimizer.scala`. ### Why are the changes needed? `With` expressions are `Unevaluable` — they are internal tree constructs for common subexpression elimination that must be rewritten before physical planning. `FinishAnalysis` replaces `RuntimeReplaceable` expressions (e.g., `Between`) with `With` nodes; `RewriteWithExpression` must then rewrite those nodes into executable form. If excluded via `spark.sql.optimizer.excludedRules`, the `With` nodes survive into codegen and throw `INTERNAL_ERROR: Cannot generate code for expression: with(...)`. ### Does this PR introduce _any_ user-facing change? Yes. Users who previously set `spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.RewriteWithExpression` will no longer see the rule excluded (it becomes a no-op config entry). This is strictly better — the previous behavior was a crash. ### How was this patch tested? Added a regression test in `SQLQuerySuite` that runs a `BETWEEN` query with `RewriteWithExpression` in `excludedRules` and asserts correct results. ### Was this patch authored or co-authored using generative AI tooling? Yes.
1. Use RewriteWithExpression.ruleName instead of hardcoded string in test (compiler-checked reference prevents silent breakage on rename). 2. Update comment to describe what breaks when excluded, matching the pattern of adjacent entries (ConvertToCatalyst, CleanupDynamicPruningFilters).
shrirangmhalgi
left a comment
There was a problem hiding this comment.
Thankyou @peter-toth for the thorough review! Addressed both inline suggestions in the latest commit.
- Switched to RewriteWithExpression.ruleName (compiler-checked).
- Updated comment to describe the failure mode, matching ConvertToCatalyst's pattern.
For the JIRA: widened Affects Version to include 4.0.0 as well on SPARK-59019. We should be backporting the fix to branch-4.0 as well if possible
Sorry, something went wrong.
What changes were proposed in this pull request?
Add RewriteWithExpression to the nonExcludableRules list in Optimizer.scala.
Why are the changes needed?
With expressions are Unevaluable — they are internal tree constructs for common subexpression elimination that must be rewritten before physical planning. FinishAnalysis replaces RuntimeReplaceable expressions (e.g., Between) with With nodes; RewriteWithExpression must then rewrite those nodes into executable form. If excluded via spark.sql.optimizer.excludedRules, the With nodes survive into codegen and throw INTERNAL_ERROR: Cannot generate code for expression: with(...).
Does this PR introduce any user-facing change?
Yes. Users who previously set spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.RewriteWithExpression will no longer see the rule excluded (it becomes a no-op config entry). This is strictly better — the previous behavior was a crash.
How was this patch tested?
Added a regression test in SQLQuerySuite that runs a BETWEEN query with RewriteWithExpression in excludedRules and asserts correct results.
Was this patch authored or co-authored using generative AI tooling?
Yes. Co-Authored using Claude Opus 4.8