Skip to content

Commit

Permalink
[SPARK-32127][SQL] Check rules for MERGE INTO should use MergeAction.…
Browse files Browse the repository at this point in the history
…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 <xianyin.xxy@alibaba-inc.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
  • Loading branch information
xy_xin authored and cloud-fan committed Jun 29, 2020
1 parent a00470d commit 503e56a
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 503e56a

Please sign in to comment.