[GitHub] [spark] maryannxue commented on a change in pull request #25039: [SPARK-28237][SQL] Add an Idempotent batch strategy to catch potential bugs in corresponding rules

[GitBox <gi...@apache.org>]

maryannxue commented on a change in pull request #25039: [SPARK-28237][SQL] Add an Idempotent batch strategy to catch potential bugs in corresponding rules
URL: https://github.com/apache/spark/pull/25039#discussion_r300051708
 
 

 ##########
 File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/rules/RuleExecutor.scala
 ##########
 @@ -47,6 +47,9 @@ abstract class RuleExecutor[TreeType <: TreeNode[_]] extends Logging {
    */
   abstract class Strategy { def maxIterations: Int }
 
+  /** A strategy that is idempotent, i.e. additional runs should not change query plans */
+  case object Idempotent extends Strategy { val maxIterations = 1 }
+
   /** A strategy that only runs once. */
   case object Once extends Strategy { val maxIterations = 1 }
 
 Review comment:
   After we make sure all rules are idempotent, `Once` would be for test only and the names would all seem weird and confusing.
   One way is to use `Once` for the semantics of idempotent, and add a test-only `OnceUnchecked` for all the catalyst rule tests (I don't suppose we should use this anywhere else).
   Another option is to add another ENV variable, similar to `SPARK_TESTING` (sth. like `SPARK_RULE_TESTING`) for the `RuleExecutor` to skip the check.
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org