[SPARK-40374][SQL] Migrate type check failures of type creators onto error classes #38463

panbingkun · 2022-11-01T07:38:03Z

What changes were proposed in this pull request?

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

CreateMap (3):

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

Lines 205 to 214 in 1431975

    
             TypeCheckResult.TypeCheckFailure( 
        
               s"$prettyName expects a positive even number of arguments.") 
        
           } else if (!TypeCoercion.haveSameType(keys.map(_.dataType))) { 
        
             TypeCheckResult.TypeCheckFailure( 
        
               "The given keys of function map should all be the same type, but they are " + 
        
                 keys.map(_.dataType.catalogString).mkString("[", ", ", "]")) 
        
           } else if (!TypeCoercion.haveSameType(values.map(_.dataType))) { 
        
             TypeCheckResult.TypeCheckFailure( 
        
               "The given values of function map should all be the same type, but they are " + 
        
                 values.map(_.dataType.catalogString).mkString("[", ", ", "]"))

CreateNamedStruct (3):

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

Lines 445 to 457 in 1431975

    
           if (children.size % 2 != 0) { 
        
             TypeCheckResult.TypeCheckFailure(s"$prettyName expects an even number of arguments.") 
        
           } else { 
        
             val invalidNames = nameExprs.filterNot(e => e.foldable && e.dataType == StringType) 
        
             if (invalidNames.nonEmpty) { 
        
               TypeCheckResult.TypeCheckFailure( 
        
                 s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" + 
        
                   s" position, got: ${invalidNames.mkString(",")}") 
        
             } else if (!names.contains(null)) { 
        
               TypeCheckResult.TypeCheckSuccess 
        
             } else { 
        
               TypeCheckResult.TypeCheckFailure("Field name should not be null") 
        
             }

UpdateFields (2):

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

Lines 670 to 673 in 1431975

    
             TypeCheckResult.TypeCheckFailure("struct argument should be struct type, got: " + 
        
               dataType.catalogString) 
        
           } else if (newExprs.isEmpty) { 
        
             TypeCheckResult.TypeCheckFailure("cannot drop all fields in struct")

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

…error classes

LuciferYang · 2022-11-01T08:26:10Z

core/src/main/resources/error/error-classes.json

          "To convert values from <srcType> to <targetType>, you can use the functions <functionNames> instead."
        ]
      },
+      "CREATE_MAP_KEY_DIFF_TYPES" : {


Cloud we make DATA_DIFF_TYPES more general and reuse it?

but

Very hard to reuse it.
Maybe functionName pass to: map's keys or map's values, seems unreasonable

LuciferYang · 2022-11-01T08:26:23Z

core/src/main/resources/error/error-classes.json

+          "The given keys of function <functionName> should all be the same type, but they are <dataType>."
+        ]
+      },
+      "CREATE_MAP_VALUE_DIFF_TYPES" : {


LuciferYang · 2022-11-01T08:37:26Z

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

-          s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" +
-            s" position, got: ${invalidNames.mkString(",")}")
+        DataTypeMismatch(
+          errorSubClass = "CREATE_NAMED_STRUCT_WITHOUT_FOLDABLE_STRING",


Can NON_FOLDABLE_INPUT be reused?

It seems that would be hard to make NON_FOLDABLE_INPUT more general to cover the case. Let's introduce more specific error class for the case.

LuciferYang · 2022-11-01T08:39:47Z

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

+      )
    } else if (newExprs.isEmpty) {
-      TypeCheckResult.TypeCheckFailure("cannot drop all fields in struct")
+      DataTypeMismatch(


It seems that the original message did not clearly describe this error, the message doesn't look like DataTypeMismatch

It is some kind of UNEXPECTED_INPUT_TYPE: op (drop field) + its args are not supported because they produce empty struct. I am ok w/ this particular error class.

…error classes

AmplabJenkins · 2022-11-01T22:11:36Z

Can one of the admins verify this patch?

panbingkun · 2022-11-02T05:56:44Z

cc @MaxGekk

MaxGekk · 2022-11-02T11:51:02Z

+1, LGTM. Merging to master.
Thank you, @panbingkun and @LuciferYang for review.

…error classes ### What changes were proposed in this pull request? This pr replaces TypeCheckFailure by DataTypeMismatch in type checks in the complex type creator expressions, includes: 1. CreateMap (3): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L205-L214 2. CreateNamedStruct (3): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L445-L457 3. UpdateFields (2): https://github.com/apache/spark/blob/1431975723d8df30a25b2333eddcfd0bb6c57677/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala#L670-L673 ### 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#38463 from panbingkun/SPARK-40374. Authored-by: panbingkun <[email protected]> Signed-off-by: Max Gekk <[email protected]>

[SPARK-40374][SQL] Migrate type check failures of type creators onto …

7b1dfd8

…error classes

github-actions bot added CORE SQL labels Nov 1, 2022

LuciferYang reviewed Nov 1, 2022

View reviewed changes

[SPARK-40374][SQL] Migrate type check failures of type creators onto …

d1d2974

…error classes

MaxGekk approved these changes Nov 2, 2022

View reviewed changes

MaxGekk closed this in 68531ad Nov 2, 2022

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

	TypeCheckResult.TypeCheckFailure(
	s"$prettyName expects a positive even number of arguments.")
	} else if (!TypeCoercion.haveSameType(keys.map(_.dataType))) {
	TypeCheckResult.TypeCheckFailure(
	"The given keys of function map should all be the same type, but they are " +
	keys.map(_.dataType.catalogString).mkString("[", ", ", "]"))
	} else if (!TypeCoercion.haveSameType(values.map(_.dataType))) {
	TypeCheckResult.TypeCheckFailure(
	"The given values of function map should all be the same type, but they are " +
	values.map(_.dataType.catalogString).mkString("[", ", ", "]"))

	if (children.size % 2 != 0) {
	TypeCheckResult.TypeCheckFailure(s"$prettyName expects an even number of arguments.")
	} else {
	val invalidNames = nameExprs.filterNot(e => e.foldable && e.dataType == StringType)
	if (invalidNames.nonEmpty) {
	TypeCheckResult.TypeCheckFailure(
	s"Only foldable ${StringType.catalogString} expressions are allowed to appear at odd" +
	s" position, got: ${invalidNames.mkString(",")}")
	} else if (!names.contains(null)) {
	TypeCheckResult.TypeCheckSuccess
	} else {
	TypeCheckResult.TypeCheckFailure("Field name should not be null")
	}

	TypeCheckResult.TypeCheckFailure("struct argument should be struct type, got: " +
	dataType.catalogString)
	} else if (newExprs.isEmpty) {
	TypeCheckResult.TypeCheckFailure("cannot drop all fields in struct")

[SPARK-40374][SQL] Migrate type check failures of type creators onto error classes #38463

[SPARK-40374][SQL] Migrate type check failures of type creators onto error classes #38463

Uh oh!

Conversation

panbingkun commented Nov 1, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Nov 1, 2022

Uh oh!

panbingkun commented Nov 2, 2022

Uh oh!

MaxGekk commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

panbingkun Nov 1, 2022 •

edited

Loading