[spark] branch branch-3.0 updated: [SPARK-32127][SQL] Check rules for MERGE INTO should use MergeAction.conditition other than MergeAction.children

[we...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

wenchen pushed a commit to branch branch-3.0
in repository https://gitbox.apache.org/repos/asf/spark.git


The following commit(s) were added to refs/heads/branch-3.0 by this push:
     new 503e56a  [SPARK-32127][SQL] Check rules for MERGE INTO should use MergeAction.conditition other than MergeAction.children
503e56a is described below

commit 503e56aec53c73d7673a79d2e150ac288e44ce7e
Author: xy_xin <xi...@alibaba-inc.com>
AuthorDate: Mon Jun 29 13:16:38 2020 +0000

    [SPARK-32127][SQL] Check rules for MERGE INTO should use MergeAction.conditition other than MergeAction.children
    
    ### What changes were proposed in this pull request?
    
    This pr fix a bug of check rules for MERGE INTO.
    
    ### Why are the changes needed?
    
    SPARK-30924 adds some check rules for MERGE INTO one of which ensures the first MATCHED clause must have a condition. However, it uses `MergeAction.children` in the checking which is not accurate for the case, and it lets the below case pass the check:
    
    ```
    MERGE INTO testcat1.ns1.ns2.tbl AS target
    xxx
    WHEN MATCHED THEN UPDATE SET target.col2 = source.col2
    WHEN MATCHED THEN DELETE
    xxx
    ```
    
    We should use `MergeAction.condition` instead.
    
    ### Does this PR introduce _any_ user-facing change?
    
    No
    
    ### How was this patch tested?
    
    An existed ut is modified.
    
    Closes #28943 from xianyinxin/SPARK-32127.
    
    Authored-by: xy_xin <xi...@alibaba-inc.com>
    Signed-off-by: Wenchen Fan <we...@databricks.com>
---
 .../org/apache/spark/sql/catalyst/parser/AstBuilder.scala      |  2 +-
 .../apache/spark/sql/catalyst/plans/logical/v2Commands.scala   | 10 +++++-----
 .../org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala  |  2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
index f6877d2..5f1bc32 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
@@ -468,7 +468,7 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
       throw new ParseException("There must be at least one WHEN clause in a MERGE statement", ctx)
     }
     // children being empty means that the condition is not set
-    if (matchedActions.length == 2 && matchedActions.head.children.isEmpty) {
+    if (matchedActions.length == 2 && matchedActions.head.condition.isEmpty) {
       throw new ParseException("When there are 2 MATCHED clauses in a MERGE statement, " +
         "the first MATCHED clause must have a condition", ctx)
     }
diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala
index 579157a..b4120d9 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala
@@ -346,25 +346,25 @@ case class MergeIntoTable(
   override def children: Seq[LogicalPlan] = Seq(targetTable, sourceTable)
 }
 
-sealed abstract class MergeAction(
-    condition: Option[Expression]) extends Expression with Unevaluable {
+sealed abstract class MergeAction extends Expression with Unevaluable {
+  def condition: Option[Expression]
   override def foldable: Boolean = false
   override def nullable: Boolean = false
   override def dataType: DataType = throw new UnresolvedException(this, "nullable")
   override def children: Seq[Expression] = condition.toSeq
 }
 
-case class DeleteAction(condition: Option[Expression]) extends MergeAction(condition)
+case class DeleteAction(condition: Option[Expression]) extends MergeAction
 
 case class UpdateAction(
     condition: Option[Expression],
-    assignments: Seq[Assignment]) extends MergeAction(condition) {
+    assignments: Seq[Assignment]) extends MergeAction {
   override def children: Seq[Expression] = condition.toSeq ++ assignments
 }
 
 case class InsertAction(
     condition: Option[Expression],
-    assignments: Seq[Assignment]) extends MergeAction(condition) {
+    assignments: Seq[Assignment]) extends MergeAction {
   override def children: Seq[Expression] = condition.toSeq ++ assignments
 }
 
diff --git a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
index 20b9855..55c3880 100644
--- a/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
+++ b/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala
@@ -1132,8 +1132,8 @@ class DDLParserSuite extends AnalysisTest {
           |MERGE INTO testcat1.ns1.ns2.tbl AS target
           |USING testcat2.ns1.ns2.tbl AS source
           |ON target.col1 = source.col1
-          |WHEN MATCHED THEN DELETE
           |WHEN MATCHED THEN UPDATE SET target.col2 = source.col2
+          |WHEN MATCHED THEN DELETE
           |WHEN NOT MATCHED AND (target.col2='insert')
           |THEN INSERT (target.col1, target.col2) values (source.col1, source.col2)
         """.stripMargin)


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