[SPARK-17989][SQL] Check ascendingOrder type in sort_array function rather than throwing ClassCastException #15532

HyukjinKwon · 2016-10-18T16:09:13Z

What changes were proposed in this pull request?

This PR proposes to check the second argument, ascendingOrder rather than throwing ClassCastException exception message.

select sort_array(array('b', 'd'), '1');

Before

16/10/19 13:16:08 ERROR SparkSQLDriver: Failed in [select sort_array(array('b', 'd'), '1')]
java.lang.ClassCastException: org.apache.spark.unsafe.types.UTF8String cannot be cast to java.lang.Boolean
    at scala.runtime.BoxesRunTime.unboxToBoolean(BoxesRunTime.java:85)
    at org.apache.spark.sql.catalyst.expressions.SortArray.nullSafeEval(collectionOperations.scala:185)
    at org.apache.spark.sql.catalyst.expressions.BinaryExpression.eval(Expression.scala:416)
    at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:50)
    at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:43)
    at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292)
    at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292)
    at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(TreeNode.scala:74)
    at org.apache.spark.sql.catalyst.trees.TreeNode.transformDown(TreeNode.scala:291)
    at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformDown$1.apply(TreeNode.scala:297)

After

Error in query: cannot resolve 'sort_array(array('b', 'd'), '1')' due to data type mismatch: Sort order in second argument requires a boolean literal.; line 1 pos 7;

How was this patch tested?

Unit test in DataFrameFunctionsSuite.

rxin · 2016-10-18T17:56:49Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala

can we move this into "sql/core/src/test/resources/sql-tests/inputs/array.sql"

SparkQA · 2016-10-18T18:20:14Z

Test build #67129 has finished for PR 15532 at commit 5f73245.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tejasapatil · 2016-10-18T19:06:04Z

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

Can you explicitly mention that the error is in context of sort_array() ? For SQL query which spans 100+ lines, its easy to get lost while tracking why it failed. Having better error messages would make that easier for users.

HyukjinKwon · 2016-10-18T22:50:26Z

Thank you both. Will adress the comments soon.

SparkQA · 2016-10-19T05:52:22Z

Test build #67167 has finished for PR 15532 at commit 55c0804.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-10-19T05:55:23Z

sql/core/src/test/resources/sql-tests/inputs/array.sql

add some comment explaining what this is testing? (error reporting)

Sure, I will.

rxin · 2016-10-19T05:56:00Z

sql/core/src/test/resources/sql-tests/results/array.sql.out

looking at the entirety of the error message, it looks like maybe we don't need to call out sort_array in the error message, since it is already here.

cc @tejasapatil

FWIW, It seems it does not print it (prettyName) in the message (from the expression) as default in ExpectsInputTypes/ImplicitCastInputTypes.

s"argument ${idx + 1} requires ${expected.simpleString} type, " + s"however, '${child.sql}' is of ${child.dataType.simpleString} type."

Yes. Now that I see the final error message, its fine to omit that.

Thank you for confirming @tejasapatil

rxin · 2016-10-19T06:33:02Z

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

i think you should just say "requires boolean type" and remove "as non-null".

I don't know what that was saying ...

Even better, say "requires a boolean literal"

maybe even ignore the actual value to keep it short.

(also technically your current english sentence is grammatically incorrect; you can't use comma to separate however here)

Actually, I wrote this as I was worried of the case cast(NULL as boolean).

Sort order in second argument requires boolean type as non-null, however, it is 'CAST(NULL AS BOOLEAN)' as boolean type.

I will just make it short. Actually, that sentence was copied from the default one.

… ClassCastException

rxin · 2016-10-19T07:02:17Z

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

+          TypeCheckResult.TypeCheckSuccess
+        case _ =>
+          TypeCheckResult.TypeCheckFailure(
+            "Sort order in second argument requires a boolean literal.")


I just checked the error msg for ExpectsInputTypes and it is not consistent anymore. Anyway I think this is fine. I will go through this and fix them myself later.

rxin · 2016-10-19T07:02:28Z

LGTM pending Jenkins.

HyukjinKwon · 2016-10-19T07:09:32Z

Thank you @rxin and @tejasapatil

SparkQA · 2016-10-19T08:40:23Z

Test build #67174 has finished for PR 15532 at commit eafe4d6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-10-19T08:59:29Z

Test build #67176 has finished for PR 15532 at commit 6ff7c7d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-10-20T02:35:40Z

Thanks - merging in master/branch-2.0.

…ather than throwing ClassCastException ## What changes were proposed in this pull request? This PR proposes to check the second argument, `ascendingOrder` rather than throwing `ClassCastException` exception message. ```sql select sort_array(array('b', 'd'), '1'); ``` **Before** ``` 16/10/19 13:16:08 ERROR SparkSQLDriver: Failed in [select sort_array(array('b', 'd'), '1')] java.lang.ClassCastException: org.apache.spark.unsafe.types.UTF8String cannot be cast to java.lang.Boolean at scala.runtime.BoxesRunTime.unboxToBoolean(BoxesRunTime.java:85) at org.apache.spark.sql.catalyst.expressions.SortArray.nullSafeEval(collectionOperations.scala:185) at org.apache.spark.sql.catalyst.expressions.BinaryExpression.eval(Expression.scala:416) at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:50) at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:43) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292) at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(TreeNode.scala:74) at org.apache.spark.sql.catalyst.trees.TreeNode.transformDown(TreeNode.scala:291) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformDown$1.apply(TreeNode.scala:297) ``` **After** ``` Error in query: cannot resolve 'sort_array(array('b', 'd'), '1')' due to data type mismatch: Sort order in second argument requires a boolean literal.; line 1 pos 7; ``` ## How was this patch tested? Unit test in `DataFrameFunctionsSuite`. Author: hyukjinkwon <[email protected]> Closes #15532 from HyukjinKwon/SPARK-17989. (cherry picked from commit 4b2011e) Signed-off-by: Reynold Xin <[email protected]>

…ather than throwing ClassCastException ## What changes were proposed in this pull request? This PR proposes to check the second argument, `ascendingOrder` rather than throwing `ClassCastException` exception message. ```sql select sort_array(array('b', 'd'), '1'); ``` **Before** ``` 16/10/19 13:16:08 ERROR SparkSQLDriver: Failed in [select sort_array(array('b', 'd'), '1')] java.lang.ClassCastException: org.apache.spark.unsafe.types.UTF8String cannot be cast to java.lang.Boolean at scala.runtime.BoxesRunTime.unboxToBoolean(BoxesRunTime.java:85) at org.apache.spark.sql.catalyst.expressions.SortArray.nullSafeEval(collectionOperations.scala:185) at org.apache.spark.sql.catalyst.expressions.BinaryExpression.eval(Expression.scala:416) at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:50) at org.apache.spark.sql.catalyst.optimizer.ConstantFolding$$anonfun$apply$1$$anonfun$applyOrElse$1.applyOrElse(expressions.scala:43) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$3.apply(TreeNode.scala:292) at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(TreeNode.scala:74) at org.apache.spark.sql.catalyst.trees.TreeNode.transformDown(TreeNode.scala:291) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformDown$1.apply(TreeNode.scala:297) ``` **After** ``` Error in query: cannot resolve 'sort_array(array('b', 'd'), '1')' due to data type mismatch: Sort order in second argument requires a boolean literal.; line 1 pos 7; ``` ## How was this patch tested? Unit test in `DataFrameFunctionsSuite`. Author: hyukjinkwon <[email protected]> Closes apache#15532 from HyukjinKwon/SPARK-17989.

HyukjinKwon changed the title ~~[SPARK-17989][SQL] Check ascendingOrder type in sort_array function ahead~~ [SPARK-17989][SQL] Check ascendingOrder type in sort_array function rather than throwing ClassCastException Oct 18, 2016

rxin reviewed Oct 18, 2016

View reviewed changes

tejasapatil reviewed Oct 18, 2016

View reviewed changes

HyukjinKwon force-pushed the SPARK-17989 branch from 5f73245 to cbd9605 Compare October 19, 2016 04:04

rxin reviewed Oct 19, 2016

View reviewed changes

HyukjinKwon force-pushed the SPARK-17989 branch from 55c0804 to eafe4d6 Compare October 19, 2016 06:28

rxin reviewed Oct 19, 2016

View reviewed changes

Check ascendingOrder type in sort_array function rather than throwing…

6ff7c7d

… ClassCastException

HyukjinKwon force-pushed the SPARK-17989 branch from eafe4d6 to 6ff7c7d Compare October 19, 2016 06:50

rxin reviewed Oct 19, 2016

View reviewed changes

asfgit closed this in 4b2011e Oct 20, 2016

HyukjinKwon deleted the SPARK-17989 branch January 2, 2018 03:44

[SPARK-17989][SQL] Check ascendingOrder type in sort_array function rather than throwing ClassCastException #15532

[SPARK-17989][SQL] Check ascendingOrder type in sort_array function rather than throwing ClassCastException #15532

Uh oh!

Conversation

HyukjinKwon commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 18, 2016

Uh oh!

SparkQA commented Oct 19, 2016

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!

HyukjinKwon Oct 19, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Oct 19, 2016

Uh oh!

HyukjinKwon commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

SparkQA commented Oct 19, 2016

Uh oh!

rxin commented Oct 20, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HyukjinKwon commented Oct 18, 2016 •

edited

Loading

HyukjinKwon Oct 19, 2016 •

edited

Loading