[SPARK-16286][SQL] Implement stack table generating function #14033

dongjoon-hyun · 2016-07-03T05:20:44Z

What changes were proposed in this pull request?

This PR implements stack table generating function.

How was this patch tested?

Pass the Jenkins tests including new testcases.

SparkQA · 2016-07-03T07:11:28Z

Test build #61675 has finished for PR 14033 at commit 6de93a1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Stack(children: Seq[Expression])

dongjoon-hyun · 2016-07-03T17:29:40Z

Rebased to resolve conflicts.

dongjoon-hyun · 2016-07-03T17:30:01Z

cc @rxin and @cloud-fan .

SparkQA · 2016-07-03T19:19:33Z

Test build #61693 has finished for PR 14033 at commit d6691f7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Stack(children: Seq[Expression])

SparkQA · 2016-07-03T23:00:15Z

Test build #61696 has finished for PR 14033 at commit f02e1dd.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Stack(children: Seq[Expression])

cloud-fan · 2016-07-04T04:09:57Z

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

As we override checkInputDataTypes here, the ImplicitCastInputTypes is useless now.
We need to take care of all type check logic in checkInputDataTypes ourselves.

Thank you for review again, @cloud-fan .
For this, I added type casting tests here.
https://github.com/apache/spark/pull/14033/files#diff-a2587541e08bf6e23df33738488d070aR30

Did I miss something there?

e.g. what if the first argument is not int type? and I'm also surprised that stack(1, 1.0, 2) works, we will cast 1.0 to int type, according to the definition of inputTypes

Oh, there is misleading comment. The first argument 1 is the number of row. Its type is checked by type-checker.
The type of first argument of data, 1.0, rules the followings.

Should I modified the description, the first data type rules, more clearly?

scala> sql("select stack(1.0,2,3)"); java.lang.ClassCastException: org.apache.spark.sql.types.Decimal cannot be cast to java.lang.Integer

we should throw AnalysisException instead of ClassCastException, the type checking is not working here.

Oh, I see now what you mean! Correctly, I missed that.
I'll add the logic and testcase. Thank you again.

SparkQA · 2016-07-04T06:19:28Z

Test build #61705 has finished for PR 14033 at commit e21bdd9.

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

SparkQA · 2016-07-04T06:26:06Z

Test build #61707 has finished for PR 14033 at commit 6b82f6c.

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

SparkQA · 2016-07-04T06:39:57Z

Test build #61709 has finished for PR 14033 at commit 3e3a794.

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

cloud-fan · 2016-07-04T08:42:19Z

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

Can you check what's the type coercion rule for hive? It looks to me that the values for same column should be same type, but not all values should be same type.

Oh. I overlooked that. Yes, right. It should be applied for the columns.
Hmm. Let me check and investigate more. Thank you for pointing out that.

Interesting. Here is the result from Hive. I'll fix this PR like Hive.

hive> select stack(2, 2.0, 3, 4, 5.0); FAILED: UDFArgumentException Argument 1's type (double) should be equal to argument 3's type (int)

Anyway, sorry for this. I completely misunderstood the behavior of this function.

dongjoon-hyun · 2016-07-04T11:15:29Z

Hi, @cloud-fan .
I rewrote the testcases and logics.
I squashed the commits into a single one because the diff became messy.

SparkQA · 2016-07-04T13:02:51Z

Test build #61721 has finished for PR 14033 at commit 51738d7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Stack(children: Seq[Expression])

cloud-fan · 2016-07-04T14:37:13Z

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

+
+  private lazy val numRows = try {
+    children.head.eval().asInstanceOf[Int]
+  } catch {


I don't think we need to try-catch here, it's guaranteed to be int after the type checking.

This is needed since numRows and numFields is used in elementSchema first before checkInputDataTypes.

elementSchema is a method, where do we call it before the type checking?

Oh, indeed. Without that, all test passes. elementSchema is not called before.
During developing, I thought I found a case for that. But, I must be confused at some mixed cases.

dongjoon-hyun · 2016-07-04T17:22:06Z

Thank you for fast review always, @cloud-fan !
I fixed one of three comments.

SparkQA · 2016-07-04T19:10:14Z

Test build #61729 has finished for PR 14033 at commit 4c5ed83.

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

SparkQA · 2016-07-05T05:12:29Z

Test build #61741 has finished for PR 14033 at commit 9d93ddc.

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

SparkQA · 2016-07-05T05:13:00Z

Test build #61742 has finished for PR 14033 at commit 8abcab5.

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

cloud-fan · 2016-07-05T05:49:19Z

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

+case class Stack(children: Seq[Expression])
+    extends Expression with Generator with CodegenFallback {
+
+  private lazy val numRows = children.head.eval().asInstanceOf[Int]


can you explain a bit more about this? It will be good if we can expression the logic more clear using math.ceil or something.

Sure. I will use math.ceil clearly.

…dundant modula operation.

dongjoon-hyun · 2016-07-05T16:51:47Z

Thank you so much always, @cloud-fan !

cloud-fan · 2016-07-05T17:43:25Z

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

+      case (e, index) => StructField(s"col$index", e.dataType)
+    })
+
+  override def eval(input: InternalRow): TraversableOnce[InternalRow] = {


It's better to call toArray here, as we will access it by index in a loop

cloud-fan · 2016-07-05T17:44:48Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/GeneratorExpressionSuite.scala

+
+    checkTuple(
+      Stack(Seq(2, "a", "b", "c").map(Literal(_))),
+      Seq(create_row("a", "b"), create_row("c", null)))


also add some test cases for type checking failure.

Oh, we cannot test that here since it's Expression-level testing.

I remembered the reason why I added try-catch before. (Currently, it is removed.)

+ checkTuple(Stack(Seq(1.0).map(Literal(_))), Seq(create_row())) ... java.lang.Double cannot be cast to java.lang.Integer java.lang.ClassCastException: java.lang.Double cannot be cast to java.lang.Integer

you can follow this one: 85f2303#diff-e4663e57952b37150642b33b998715a8R94

Thank you for the pointer. Nice!

cloud-fan · 2016-07-05T17:48:13Z

LGTM except some minor comments, thanks for working on it!

dongjoon-hyun · 2016-07-05T17:51:56Z

It's my pleasure. I'm still learning Spark. :)

dongjoon-hyun · 2016-07-05T17:59:48Z

Hi, @cloud-fan .
I added toArray and couldn't add type-checking tests on expression-level suite.
I hope that is okay for you. If not, please let me know.

SparkQA · 2016-07-05T18:30:56Z

Test build #61759 has finished for PR 14033 at commit 9913dd2.

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

dongjoon-hyun · 2016-07-05T18:33:13Z

Now, the testcase became much stronger.

SparkQA · 2016-07-05T18:43:56Z

Test build #61772 has finished for PR 14033 at commit 6cef803.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2016-07-05T18:58:24Z

Master branch is broken now. I'll retry to test later.

dongjoon-hyun · 2016-07-05T19:08:37Z

Retest this please

SparkQA · 2016-07-05T20:00:33Z

Test build #61769 has finished for PR 14033 at commit 7ed75b4.

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

SparkQA · 2016-07-05T21:08:41Z

Test build #61777 has finished for PR 14033 at commit 6cef803.

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

tejasapatil · 2016-07-05T22:25:06Z

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


+/**
+ * Separate v1, ..., vk into n rows. Each row will have k/n columns. n must be constant.
+ * {{{


nit: double )

Oops. Thank you, @tejasapatil !

SparkQA · 2016-07-06T00:34:09Z

Test build #61796 has finished for PR 14033 at commit 4445248.

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

tejasapatil · 2016-07-06T02:19:18Z

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

+  private lazy val numFields = Math.ceil((children.length - 1.0) / numRows).toInt
+
+  override def checkInputDataTypes(): TypeCheckResult = {
+    if (children.length <= 1) {


Can you also include the number of args passed and the args ?

What do you mean? Could you give some example what you want?

Sorry I merged this PR before see your comments. Yea including the number of args makes the error message more friendly, but not a big deal, @dongjoon-hyun you can fix it in your next PR by the way

Ah. Now I understand the meaning. Sure, someday later.
Maybe, the pattern is popular, so we can fix all of the error message together in a single PR.

cloud-fan · 2016-07-06T02:57:27Z

thanks, merging to master!

dongjoon-hyun · 2016-07-06T02:57:59Z

Thank you for merging, @cloud-fan !

This PR implements `stack` table generating function. Pass the Jenkins tests including new testcases. Author: Dongjoon Hyun <dongjoon@apache.org> Closes #14033 from dongjoon-hyun/SPARK-16286. (cherry picked from commit d0d2850) Signed-off-by: Reynold Xin <rxin@databricks.com>

cloud-fan · 2017-03-10T07:40:58Z

just found out that we didn't implement a type coercion rule for stack...

dongjoon-hyun · 2017-03-10T17:09:38Z

Oh, I'll look at that, @cloud-fan .

dongjoon-hyun · 2017-03-10T17:28:12Z

Do you mean the column-wise type coercion to pass the following case (differently from hive)?

hive> select stack(2,1,'a',3,2);
FAILED: UDFArgumentException Argument 2's type (string) should be equal to argument 4's type (int)

cloud-fan · 2017-03-10T18:27:48Z

let's follow hive, e.g. select stack(2, 1, null, 3, 2) can process

dongjoon-hyun · 2017-03-10T18:45:41Z

Ah, I understand the case. Hive allows NULL. I'll try to support the NULL case. Thanks, @cloud-fan .

hive> select stack(3, 1, 'a', 2, 'b', 3, 'c');
OK
1	a
2	b
3	c

hive> select stack(3, 1, 'a', 2, 'b', 3, null);
OK
1	a
2	b
3	NULL

hive> select stack(3, 1, 'a', 2, 'b', 3, 1);
FAILED: UDFArgumentException Argument 2's type (string) should be equal to argument 6's type (int)

hive> select stack(3, 1, NULL, 2, 'b', 3, 1);
FAILED: UDFArgumentException Argument 2's type (void) should be equal to argument 6's type (int)

dongjoon-hyun · 2017-03-10T20:58:47Z

I created the JIRA issue. I'll make a PR soon.
https://issues.apache.org/jira/browse/SPARK-19910

cloud-fan reviewed Jul 4, 2016
View reviewed changes

[SPARK-16286][SQL] Implement stack table generating function

51738d7

cloud-fan reviewed Jul 4, 2016
View reviewed changes

Use numRows consistently.

4c5ed83

Remove try-catch.

8abcab5

cloud-fan reviewed Jul 5, 2016
View reviewed changes

Use better expressions for numFields and elementSchema, and remove re…

9913dd2

…dundant modula operation.

cloud-fan reviewed Jul 5, 2016
View reviewed changes

Add toArray for performance.

7ed75b4

Add type checking failure testcases.

6cef803

tejasapatil reviewed Jul 5, 2016
View reviewed changes

Remove ')'.

4445248

tejasapatil reviewed Jul 6, 2016
View reviewed changes

asfgit closed this in d0d2850 Jul 6, 2016

dongjoon-hyun deleted the SPARK-16286 branch July 20, 2016 07:42

[SPARK-16286][SQL] Implement stack table generating function #14033

[SPARK-16286][SQL] Implement stack table generating function #14033

Uh oh!

Conversation

dongjoon-hyun commented Jul 3, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 3, 2016

Uh oh!

dongjoon-hyun commented Jul 3, 2016

Uh oh!

dongjoon-hyun commented Jul 3, 2016

Uh oh!

SparkQA commented Jul 3, 2016

Uh oh!

SparkQA commented Jul 3, 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!

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 4, 2016

Uh oh!

SparkQA commented Jul 4, 2016

Uh oh!

SparkQA commented Jul 4, 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!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 4, 2016

Uh oh!

SparkQA commented Jul 4, 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!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 4, 2016

Uh oh!

SparkQA commented Jul 4, 2016

Uh oh!

SparkQA commented Jul 5, 2016

Uh oh!

SparkQA commented Jul 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 5, 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!

dongjoon-hyun Jul 5, 2016 •

edited

Loading