peter-toth commented on code in PR #58323:
URL: https://github.com/apache/spark/pull/58323#discussion_r3879181966
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -321,6 +321,8 @@ abstract class Optimizer(catalogManager: CatalogManager)
// execution, so it must never be excludable.
ConvertToCatalyst.ruleName,
FinishAnalysis.ruleName,
+ // With is Unevaluable and must be rewritten before physical planning.
Review 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.
```suggestion
// ReplaceExpressions (in FinishAnalysis) turns Between/NullIf into
the Unevaluable
// With expression; excluding this rule leaks it into codegen and
fails with INTERNAL_ERROR.
```
##########
sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala:
##########
@@ -5315,6 +5315,20 @@ class SQLQuerySuite extends SharedSparkSession with
AdaptiveSparkPlanHelper
checkToRDD = false)
}
}
+
+ test("SPARK-59019: BETWEEN succeeds when RewriteWithExpression is in
excludedRules") {
+ // RewriteWithExpression is non-excludable, so adding it to excludedRules
has no effect.
+ // Before the fix, this threw INTERNAL_ERROR because With nodes reached
codegen.
+ withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ "org.apache.spark.sql.catalyst.optimizer.RewriteWithExpression") {
Review 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.
```suggestion
withSQLConf(SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
RewriteWithExpression.ruleName) {
```
With `RewriteWithExpression` added to the
`org.apache.spark.sql.catalyst.optimizer` import at line 38.
--
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]
