[SPARK-23908][SQL][FOLLOW-UP] Rename inputs to arguments, and add argument type check. #22075

ueshin · 2018-08-11T06:49:32Z

What changes were proposed in this pull request?

This is a follow-up pr of #21954 to address comments.

Rename ambiguous name inputs to arguments.
Add argument type check and remove hacky workaround.
Address other small comments.

How was this patch tested?

Existing tests and some additional tests.

ueshin · 2018-08-11T06:49:55Z

cc @cloud-fan @hvanhovell

SparkQA · 2018-08-11T07:05:01Z

Test build #94599 has finished for PR 22075 at commit 3ccd995.

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

ueshin · 2018-08-11T07:54:54Z

Jenkins, retest this please.

SparkQA · 2018-08-11T11:55:28Z

Test build #94604 has finished for PR 22075 at commit 3ccd995.

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

cloud-fan · 2018-08-11T13:20:19Z

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

+  override def arguments: Seq[Expression] = argument :: Nil

-  override def inputs: Seq[Expression] = input :: Nil
+  def argumentType: AbstractDataType


shall we define a def argumentTypes: Seq[AbstractDataType] in HigherOrderFunction and implement checkArgumentDataTypes there?

cloud-fan · 2018-08-11T13:21:14Z

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


  override def functions: Seq[Expression] = function :: Nil

  def expectingFunctionType: AbstractDataType = AnyDataType


ditto, why don't we define it in the HigherOrderFunction?

SparkQA · 2018-08-12T17:49:40Z

Test build #94651 has finished for PR 22075 at commit 388c2d3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait HigherOrderFunction extends Expression with ExpectsInputTypes
trait SimpleHigherOrderFunction extends HigherOrderFunction

ueshin · 2018-08-13T00:17:33Z

Jenkins, retest this please.

cloud-fan · 2018-08-13T01:17:03Z

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

    assert(ex2.getMessage.contains("data type mismatch: argument 1 requires array type"))
+
+    val ex3 = intercept[AnalysisException] {
+      df.selectExpr("transform(a, x -> x)")


what's the previous behavior?

The previous behavior was the same. I just added to check the behavior is as expected.

cloud-fan · 2018-08-13T04:06:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

+        // Check argument data types of higher-order functions downwards first.
+        // If the arguments of the higher-order functions are resolved but the type check fails,
+        // the argument functions will not get resolved, but we should report the argument type
+        // check failure instead of claiming the function arguments are unresolved.


"function arguments" -> "argument functions"?

cloud-fan · 2018-08-13T04:06:28Z

LGTM

cloud-fan · 2018-08-13T04:16:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

      case operator: LogicalPlan =>
+        // Check argument data types of higher-order functions downwards first.
+        // If the arguments of the higher-order functions are resolved but the type check fails,
+        // the argument functions will not get resolved, but we should report the argument type


actually, I think it's clearer to say functions, instead of argument functions. Sorry for the back and forth.

I'm worried that if we say only functions, we might be confused whether the "function" means the higher-order function or the function as an argument. WDYT?

ah i see, makes sense.

SparkQA · 2018-08-13T04:21:09Z

Test build #94657 has finished for PR 22075 at commit 388c2d3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
trait HigherOrderFunction extends Expression with ExpectsInputTypes
trait SimpleHigherOrderFunction extends HigherOrderFunction

SparkQA · 2018-08-13T04:52:45Z

Test build #94663 has finished for PR 22075 at commit 4b48a39.

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

SparkQA · 2018-08-13T07:05:01Z

Test build #94668 has finished for PR 22075 at commit deee1dc.

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

HyukjinKwon · 2018-08-13T07:07:06Z

retest this please

SparkQA · 2018-08-13T07:54:56Z

Test build #94674 has finished for PR 22075 at commit deee1dc.

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

ueshin · 2018-08-13T08:03:44Z

Jenkins, retest this please.

SparkQA · 2018-08-13T12:08:37Z

Test build #94679 has finished for PR 22075 at commit deee1dc.

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

cloud-fan · 2018-08-13T12:58:39Z

thanks, merging to master!

mn-mikke · 2018-08-13T15:02:24Z

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

-          s"however, '${merge.sql}' is of ${merge.dataType.catalogString} type.")
-    } else {
-      TypeCheckResult.TypeCheckSuccess
+    checkArgumentDataTypes() match {


just a quick question: Isn't calling of checkArgumentDataTypes extra here if checkArgumentDataTypes is called as such before checkInputDataTypes?

I called it here again to check the whole data types when calling checkInputDataTypes(), just in case.

ueshin added 4 commits August 11, 2018 15:21

Rename inputs to arguments.

d2bf2be

Rename partialArguments to argInfo.

1ece98b

Add argument data type check.

fb23aba

Address other comments.

3ccd995

ueshin mentioned this pull request Aug 11, 2018

[SPARK-23938][SQL] Add map_zip_with function #22017

Closed

cloud-fan reviewed Aug 11, 2018

View reviewed changes

ueshin added 2 commits August 13, 2018 00:47

Merge branch 'master' into issues/SPARK-23908/fup1

32ae8fa

Address comments.

388c2d3

cloud-fan reviewed Aug 13, 2018

View reviewed changes

Reword a comment.

4b48a39

cloud-fan reviewed Aug 13, 2018

View reviewed changes

Fix a comment.

deee1dc

cloud-fan reviewed Aug 13, 2018

View reviewed changes

asfgit closed this in b804ca5 Aug 13, 2018

mn-mikke reviewed Aug 13, 2018

View reviewed changes

mn-mikke mentioned this pull request Aug 14, 2018

[SPARK-23932][SQL] Higher order function zip_with #22031

Closed


		override def functions: Seq[Expression] = function :: Nil

		def expectingFunctionType: AbstractDataType = AnyDataType

[SPARK-23908][SQL][FOLLOW-UP] Rename inputs to arguments, and add argument type check. #22075

[SPARK-23908][SQL][FOLLOW-UP] Rename inputs to arguments, and add argument type check. #22075

Uh oh!

Conversation

ueshin commented Aug 11, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ueshin commented Aug 11, 2018

Uh oh!

SparkQA commented Aug 11, 2018

Uh oh!

ueshin commented Aug 11, 2018

Uh oh!

SparkQA commented Aug 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 12, 2018

Uh oh!

ueshin commented Aug 13, 2018

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!

cloud-fan commented Aug 13, 2018

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 Aug 13, 2018

Uh oh!

SparkQA commented Aug 13, 2018

Uh oh!

SparkQA commented Aug 13, 2018

Uh oh!

HyukjinKwon commented Aug 13, 2018

Uh oh!

SparkQA commented Aug 13, 2018

Uh oh!

ueshin commented Aug 13, 2018

Uh oh!

SparkQA commented Aug 13, 2018

Uh oh!

cloud-fan commented Aug 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants