[SPARK-21281][SQL] Use string types by default if array and map have no argument #18516

maropu · 2017-07-03T15:21:01Z

What changes were proposed in this pull request?

This pr modified code to use string types by default if array and map in functions have no argument. This behaviour is the same with Hive one;

hive> CREATE TEMPORARY TABLE t1 AS SELECT map();
hive> DESCRIBE t1;
_c0   map<string,string>                          

hive> CREATE TEMPORARY TABLE t2 AS SELECT array();
hive> DESCRIBE t2;
_c0   array<string>

How was this patch tested?

Added tests in DataFrameFunctionsSuite.

maropu · 2017-07-03T15:24:20Z

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

+    errMsg = intercept[IllegalArgumentException] {
+      spark.range(1).select(greatest())
+    }.getMessage
+    assert(errMsg.contains("requirement failed: greatest requires at least 2 arguments"))


Nit: Exception types are different between sql.functions.greatest and selectExpr("greatest()");

cala> spark.range(1).select(greatest()).show java.lang.IllegalArgumentException: requirement failed: greatest requires at least 2 arguments. at scala.Predef$.require(Predef.scala:224) at org.apache.spark.sql.functions$.greatest(functions.scala:1570) ... 48 elided scala> spark.range(1).selectExpr("greatest()").show org.apache.spark.sql.AnalysisException: cannot resolve 'greatest()' due to data type mismatch: GREATEST requires at least 2 arguments; line 1 pos 0; 'Project [unresolvedalias(greatest(), Some(<function1>))] +- Range (0, 1, step=1, splits=Some(4)) at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42) at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$checkAnalysis$1$$anonfun$apply$2.applyOrElse(CheckAnalysis.scala:95) at org.apache.spark.sql.catalyst.analysis.CheckAnalysis$$anonfun$checkAnalysis$1$$anonfun$apply$2.applyOrElse(CheckAnalysis.scala:87) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformUp$1.apply(TreeNode.scala:289) at org.apache.spark.sql.catalyst.trees.TreeNode$$anonfun$transformUp$1.apply(TreeNode.scala:289) at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(TreeNode.scala:70)

maropu · 2017-07-03T15:28:58Z

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

    ).foreach(assertValuesDoNotChangeAfterCoalesceOrUnion(_))
  }
+
+  test("SPARK-21281 fails if functions have no argument") {


We currently have the six functions that have variable-length arguments and cannot stand no argument in sql.functions.

Could you create a helper function for removing these duplicate codes?

SparkQA · 2017-07-03T16:38:27Z

Test build #79103 has finished for PR 18516 at commit 1e29eab.

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

maropu · 2017-07-04T02:25:09Z

The hive-related tests failed, so I checked the hive behaviour;

hive> CREATE TEMPORARY TABLE t1 AS SELECT map();
hive> DESCRIBE t1;
_c0   map<string,string>                          

hive> CREATE TEMPORARY TABLE t2 AS SELECT array();
hive> DESCRIBE t2;
_c0   array<string>

hive> CREATE TEMPORARY TABLE t3 AS SELECT coalesce();
FAILED: SemanticException [Error 10305]: CREATE-TABLE-AS-SELECT creates a VOID type, please use CAST to specify the type, near field:  _c0

hive> CREATE TEMPORARY TABLE t4 AS SELECT struct();
hive> DESCRIBE t4;
_c0                     struct<>   

hive> CREATE TEMPORARY TABLE t5 AS SELECT greatest();
FAILED: SemanticException [Error 10015]: Line 1:37 Arguments length mismatch 'greatest': greatest requires at least 2 arguments, got 0

hive> CREATE TEMPORARY TABLE t6 AS SELECT least();
FAILED: SemanticException [Error 10015]: Line 1:37 Arguments length mismatch 'least': least requires at least 2 arguments, got 0

In hive, array and map with no argument use string types by default. Spark fails when using struct with no argument though, it seems hive return an empty struct. If you get time, could you check this? @gatorsmile Thanks!

gatorsmile · 2017-07-04T03:47:18Z

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

-    TypeUtils.checkForSameTypeInputExpr(children.map(_.dataType), "function array")
+  override def checkInputDataTypes(): TypeCheckResult = {
+    if (children == Nil) {
+      TypeCheckResult.TypeCheckFailure("input to function coalesce cannot be empty")


coalesce ?

gatorsmile · 2017-07-04T03:47:28Z

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

  override def checkInputDataTypes(): TypeCheckResult = {
-    if (children.size % 2 != 0) {
+    if (children == Nil) {
+      TypeCheckResult.TypeCheckFailure("input to function coalesce cannot be empty")


coalesce ?

oh, my bad.

SparkQA · 2017-07-04T09:18:20Z

Test build #79138 has finished for PR 18516 at commit 1636f97.

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

SparkQA · 2017-07-04T11:43:04Z

Test build #79145 has finished for PR 18516 at commit 0b492fd.

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

dongjoon-hyun · 2017-07-04T18:34:50Z

Retest this please.

dongjoon-hyun · 2017-07-04T18:43:39Z

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

-      TypeCheckResult.TypeCheckSuccess
-    }
+    TypeUtils.checkTypeInputDimension(
+      children.map(_.dataType), s"function $prettyName", requiredMinDimension = 1)


Oh, now we have a consistent message instsead of at least one argument or cannot be empty. :)

SparkQA · 2017-07-04T20:18:01Z

Test build #79161 has finished for PR 18516 at commit 0b492fd.

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

SparkQA · 2017-07-05T03:04:04Z

Test build #79174 has finished for PR 18516 at commit 24834fc.

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

SparkQA · 2017-07-05T03:09:07Z

Test build #79175 has finished for PR 18516 at commit 28d060f.

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

SparkQA · 2017-07-05T05:19:14Z

Test build #79180 has finished for PR 18516 at commit d9c05a6.

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

SparkQA · 2017-07-05T07:04:56Z

Test build #79193 has finished for PR 18516 at commit 04cbf78.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2017-07-05T08:54:02Z

Jenkins, retest this please.

SparkQA · 2017-07-05T11:13:42Z

Test build #79209 has finished for PR 18516 at commit 04cbf78.

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

dongjoon-hyun · 2017-07-05T16:51:17Z

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

-          s" ${invalidNames.mkString(",")}")
-      } else if (!names.contains(null)) {
-        TypeCheckResult.TypeCheckSuccess
+      if (children.size % 2 != 0) {


We can use else if to flatten the nest if.

SparkQA · 2017-07-06T02:52:48Z

Test build #79253 has finished for PR 18516 at commit f71dc27.

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

dongjoon-hyun

+1, LGTM.

gatorsmile

LGTM

gatorsmile · 2017-07-07T23:37:54Z

retest this please

SparkQA · 2017-07-08T02:01:29Z

Test build #79353 has finished for PR 18516 at commit f71dc27.

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

gatorsmile · 2017-07-08T06:07:41Z

Thanks! Merging to master.

cloud-fan · 2018-09-10T11:49:51Z

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

-    if (children.size % 2 != 0) {
+    if (children.length < 1) {
+      TypeCheckResult.TypeCheckFailure(
+        s"input to function $prettyName requires at least one argument")


This is not related to what this PR claims to do. What's the reason behind this change?

This is a behavior change and caused a problem in #22373

sorry, but I don't remember correctly.
I looked over this pr again and I also think the modification is not related to this pr. So, it's ok to revert this part.

Strict argument checks for array() and map()

1e29eab

maropu commented Jul 3, 2017

View reviewed changes

gatorsmile reviewed Jul 4, 2017

View reviewed changes

Apply more fixes

04b904b

Use string types by default if array and map have no argument

0b492fd

maropu force-pushed the SPARK-21281 branch from 1636f97 to 0b492fd Compare July 4, 2017 10:01

dongjoon-hyun reviewed Jul 4, 2017

View reviewed changes

maropu force-pushed the SPARK-21281 branch 2 times, most recently from 24834fc to 28d060f Compare July 5, 2017 01:39

maropu changed the title ~~[SPARK-21281][SQL] Throw AnalysisException if array and map have no argument~~ [SPARK-21281][SQL] Use string types by default if array and map have no argument Jul 5, 2017

maropu force-pushed the SPARK-21281 branch from 28d060f to d9c05a6 Compare July 5, 2017 03:28

Apply more fixes

04cbf78

maropu force-pushed the SPARK-21281 branch from d9c05a6 to 04cbf78 Compare July 5, 2017 05:57

dongjoon-hyun reviewed Jul 5, 2017

View reviewed changes

Apply comments

f71dc27

dongjoon-hyun approved these changes Jul 7, 2017

View reviewed changes

gatorsmile approved these changes Jul 7, 2017

View reviewed changes

asfgit closed this in 7896e7b Jul 8, 2017

cloud-fan reviewed Sep 10, 2018

View reviewed changes

maropu mentioned this pull request Oct 30, 2019

[SPARK-29462] The data type of "array()" should be array<null> #26324

Closed

[SPARK-21281][SQL] Use string types by default if array and map have no argument #18516

[SPARK-21281][SQL] Use string types by default if array and map have no argument #18516

Uh oh!

Conversation

maropu commented Jul 3, 2017 • 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!

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!

SparkQA commented Jul 3, 2017

Uh oh!

maropu commented Jul 4, 2017

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!

SparkQA commented Jul 4, 2017

Uh oh!

SparkQA commented Jul 4, 2017

Uh oh!

dongjoon-hyun commented Jul 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 4, 2017

Uh oh!

SparkQA commented Jul 5, 2017

Uh oh!

SparkQA commented Jul 5, 2017

Uh oh!

SparkQA commented Jul 5, 2017

Uh oh!

SparkQA commented Jul 5, 2017

Uh oh!

maropu commented Jul 5, 2017

Uh oh!

SparkQA commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 6, 2017

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jul 7, 2017

Uh oh!

SparkQA commented Jul 8, 2017

Uh oh!

gatorsmile commented Jul 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu Sep 10, 2018 • 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!

Reviewers

maropu commented Jul 3, 2017 •

edited

Loading

maropu Sep 10, 2018 •

edited

Loading