[SPARK-40748][SQL] Migrate type check failures of conditions onto error classes #38438

panbingkun · 2022-10-30T12:35:14Z

What changes were proposed in this pull request?

This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the conditional expressions, includes:

If (2):

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

Lines 61 to 67 in 1431975

    
           override def checkInputDataTypes(): TypeCheckResult = { 
        
             if (predicate.dataType != BooleanType) { 
        
               TypeCheckResult.TypeCheckFailure( 
        
                 "type of predicate expression in If should be boolean, " + 
        
                   s"not ${predicate.dataType.catalogString}") 
        
             } else if (!TypeCoercion.haveSameType(inputTypesForMerging)) { 
        
               TypeCheckResult.TypeCheckFailure(s"differing types in '$sql' " +

CaseWhen (2):

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

Lines 175 to 183 in 1431975

    
               TypeCheckResult.TypeCheckFailure( 
        
                 s"WHEN expressions in CaseWhen should all be boolean type, " + 
        
                   s"but the ${index + 1}th when expression's type is ${branches(index)._1}") 
        
             } 
        
           } else { 
        
             val branchesStr = branches.map(_._2.dataType).map(dt => s"WHEN ... THEN ${dt.catalogString}") 
        
               .mkString(" ") 
        
             val elseStr = elseValue.map(expr => s" ELSE ${expr.dataType.catalogString}").getOrElse("") 
        
             TypeCheckResult.TypeCheckFailure(

InSubquery (2):

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

Lines 378 to 396 in 1431975

    
                 TypeCheckResult.TypeCheckFailure( 
        
                   s""" 
        
                      |The number of columns in the left hand side of an IN subquery does not match the 
        
                      |number of columns in the output of subquery. 
        
                      |#columns in left hand side: ${values.length}. 
        
                      |#columns in right hand side: ${query.childOutputs.length}. 
        
                      |Left side columns: 
        
                      |[${values.map(_.sql).mkString(", ")}]. 
        
                      |Right side columns: 
        
                      |[${query.childOutputs.map(_.sql).mkString(", ")}].""".stripMargin) 
        
               } else if (!DataType.equalsStructurally( 
        
                 query.dataType, value.dataType, ignoreNullability = true)) { 
        
                 val mismatchedColumns = values.zip(query.childOutputs).flatMap { 
        
                   case (l, r) if l.dataType != r.dataType => 
        
                     Seq(s"(${l.sql}:${l.dataType.catalogString}, ${r.sql}:${r.dataType.catalogString})") 
        
                   case _ => None 
        
                 } 
        
                 TypeCheckResult.TypeCheckFailure(

In (1):

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala

Line 453 in 1431975

TypeCheckResult.TypeCheckFailure(s"Arguments must be same type but were: " +

Why are the changes needed?

Migration onto error classes unifies Spark SQL error messages.

Does this PR introduce any user-facing change?

Yes. The PR changes user-facing error messages.

How was this patch tested?

Add new UT
Update existed UT
Pass GA

…or classes

AmplabJenkins · 2022-10-30T21:25:22Z

Can one of the admins verify this patch?

…or classes

MaxGekk · 2022-10-31T08:36:51Z

@panbingkun Could you resolve conflicts, please.

…or classes

MaxGekk · 2022-10-31T14:29:31Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java

-        .forEach(s ->
-          Assert.assertTrue(e.getMessage().toLowerCase(Locale.ROOT)
-            .contains(s.toLowerCase(Locale.ROOT))));
+    System.out.println(e.getMessage().toLowerCase(Locale.ROOT));


Is it needed?

Sorry, I left the debugging code by mistake.

MaxGekk · 2022-10-31T14:36:03Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java

+    Arrays.asList(
+      "datatype_mismatch.data_diff_types",
+      "cannot resolve \"(a in (b))\"",
+      "due to data type mismatch: input to `in` should all be the same type, " +
+        "but it's [\"int\", \"array<int>\"].").forEach(s ->
+      Assert.assertTrue(e.getMessage().toLowerCase(Locale.ROOT)
+        .contains(s.toLowerCase(Locale.ROOT))));


Let's don't compare error messages. If you cannot call checkError() easily, just check error classes (maybe more if you can).

I will check:
1.Exception Type -> AnalysisException
2.Error classes.

…or classes

MaxGekk · 2022-11-01T07:10:34Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java

-        .forEach(s ->
-          Assert.assertTrue(e.getMessage().toLowerCase(Locale.ROOT)
-            .contains(s.toLowerCase(Locale.ROOT))));
+    Assert.assertTrue(e.message().startsWith("[DATATYPE_MISMATCH.DATA_DIFF_TYPES]"));


e is an AnalysisException, correct? Can't you just check its errorClass?

e is an AnalysisException

…or classes

MaxGekk

Waiting for CI. @panbingkun Please, re-trigger GAs.

MaxGekk · 2022-11-01T17:10:08Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java

-            .contains(s.toLowerCase(Locale.ROOT))));
+    Assert.assertTrue(e.getErrorClass().equals("DATATYPE_MISMATCH.DATA_DIFF_TYPES"));
+    Map<String, String> messageParameters = new HashMap() {
+      {


Just in case, are the inner {} really needed?

…or classes

MaxGekk · 2022-11-02T05:11:55Z

+1, LGTM. Merging to master.
Thank you, @panbingkun.

…or classes ### What changes were proposed in this pull request? This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the conditional expressions, includes: 1. If (2): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala#L61-L67 2. CaseWhen (2): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala#L175-L183 3. InSubquery (2): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala#L378-L396 4. In (1): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala#L453 ### Why are the changes needed? Migration onto error classes unifies Spark SQL error messages. ### Does this PR introduce _any_ user-facing change? Yes. The PR changes user-facing error messages. ### How was this patch tested? 1. Add new UT 2. Update existed UT 3. Pass GA Closes apache#38438 from panbingkun/SPARK-40748. Authored-by: panbingkun <[email protected]> Signed-off-by: Max Gekk <[email protected]>

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

78542bb

…or classes

github-actions bot added CORE SQL labels Oct 30, 2022

panbingkun added 2 commits October 31, 2022 11:32

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

f39f382

…or classes

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

13dc588

…or classes

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

09b039c

…or classes

MaxGekk requested changes Oct 31, 2022

View reviewed changes

panbingkun added 2 commits November 1, 2022 10:04

Merge branch 'master' into SPARK-40748

a83d434

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

62a39ff

…or classes

panbingkun requested a review from MaxGekk November 1, 2022 02:21

MaxGekk reviewed Nov 1, 2022

View reviewed changes

panbingkun requested a review from MaxGekk November 1, 2022 11:15

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

c660a24

…or classes

MaxGekk approved these changes Nov 1, 2022

View reviewed changes

MaxGekk reviewed Nov 1, 2022

View reviewed changes

[SPARK-40748][SQL] Migrate type check failures of conditions onto err…

c50c1c3

…or classes

MaxGekk approved these changes Nov 2, 2022

View reviewed changes

MaxGekk closed this in 23c54ad Nov 2, 2022

panbingkun deleted the SPARK-40748 branch November 7, 2022 02:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-40748][SQL] Migrate type check failures of conditions onto error classes #38438

[SPARK-40748][SQL] Migrate type check failures of conditions onto error classes #38438

Uh oh!

panbingkun commented Oct 30, 2022 •

edited

Loading

Uh oh!

AmplabJenkins commented Oct 30, 2022

Uh oh!

MaxGekk commented Oct 31, 2022

Uh oh!

MaxGekk Oct 31, 2022

Uh oh!

panbingkun Nov 1, 2022

Uh oh!

MaxGekk Oct 31, 2022

Uh oh!

panbingkun Nov 1, 2022

Uh oh!

MaxGekk Nov 1, 2022

Uh oh!

panbingkun Nov 1, 2022

Uh oh!

MaxGekk left a comment

Uh oh!

MaxGekk Nov 1, 2022

Uh oh!

MaxGekk commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	override def checkInputDataTypes(): TypeCheckResult = {
	if (predicate.dataType != BooleanType) {
	TypeCheckResult.TypeCheckFailure(
	"type of predicate expression in If should be boolean, " +
	s"not ${predicate.dataType.catalogString}")
	} else if (!TypeCoercion.haveSameType(inputTypesForMerging)) {
	TypeCheckResult.TypeCheckFailure(s"differing types in '$sql' " +

	TypeCheckResult.TypeCheckFailure(
	s"WHEN expressions in CaseWhen should all be boolean type, " +
	s"but the ${index + 1}th when expression's type is ${branches(index)._1}")
	}
	} else {
	val branchesStr = branches.map(_._2.dataType).map(dt => s"WHEN ... THEN ${dt.catalogString}")
	.mkString(" ")
	val elseStr = elseValue.map(expr => s" ELSE ${expr.dataType.catalogString}").getOrElse("")
	TypeCheckResult.TypeCheckFailure(

	TypeCheckResult.TypeCheckFailure(
	s"""
	\|The number of columns in the left hand side of an IN subquery does not match the
	\|number of columns in the output of subquery.
	\|#columns in left hand side: ${values.length}.
	\|#columns in right hand side: ${query.childOutputs.length}.
	\|Left side columns:
	\|[${values.map(_.sql).mkString(", ")}].
	\|Right side columns:
	\|[${query.childOutputs.map(_.sql).mkString(", ")}].""".stripMargin)
	} else if (!DataType.equalsStructurally(
	query.dataType, value.dataType, ignoreNullability = true)) {

	val mismatchedColumns = values.zip(query.childOutputs).flatMap {
	case (l, r) if l.dataType != r.dataType =>
	Seq(s"(${l.sql}:${l.dataType.catalogString}, ${r.sql}:${r.dataType.catalogString})")
	case _ => None
	}
	TypeCheckResult.TypeCheckFailure(

[SPARK-40748][SQL] Migrate type check failures of conditions onto error classes #38438

[SPARK-40748][SQL] Migrate type check failures of conditions onto error classes #38438

Uh oh!

Conversation

panbingkun commented Oct 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

AmplabJenkins commented Oct 30, 2022

Uh oh!

MaxGekk commented Oct 31, 2022

Uh oh!

MaxGekk Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

panbingkun Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

panbingkun Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

panbingkun Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

panbingkun commented Oct 30, 2022 •

edited

Loading