[SPARK-13456][SQL] fix creating encoders for case classes defined in Spark shell #11410

cloud-fan · 2016-02-27T09:40:28Z

What changes were proposed in this pull request?

case classes defined in REPL are wrapped by line classes, and we have a trick for scala 2.10 REPL to automatically register the wrapper classes to OuterScope so that we can use when create encoders.
However, this trick doesn't work right after we upgrade to scala 2.11, and unfortunately the tests are only in scala 2.10, which makes this bug hidden until now.

This PR moves the encoder tests to scala 2.11 ReplSuite, and fixes this bug by another approach(the previous trick can't port to scala 2.11 REPL): make OuterScope smarter that can detect classes defined in REPL and load the singleton of line wrapper classes automatically.

How was this patch tested?

the migrated encoder tests in ReplSuite

cloud-fan · 2016-02-27T09:40:53Z

cc @marmbrus @liancheng @rxin

cloud-fan · 2016-02-27T09:41:44Z

repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala

+        |import org.apache.spark.sql.TypedColumn
+        |/** An `Aggregator` that adds up any numeric type returned by the given function. */
+        |class SumOf[I, N : Numeric](f: I => N) extends
+        |  org.apache.spark.sql.expressions.Aggregator[I, N, N] {


The import doesn't work here, not sure why

created https://issues.apache.org/jira/browse/SPARK-13611 for this issue

SparkQA · 2016-02-27T09:43:46Z

Test build #52115 has finished for PR 11410 at commit a2c4617.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-02-27T11:51:42Z

Test build #52116 has finished for PR 11410 at commit a0a8424.

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

cloud-fan · 2016-02-27T12:47:56Z

retest this please

SparkQA · 2016-02-27T14:47:59Z

Test build #52119 has finished for PR 11410 at commit a0a8424.

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

cloud-fan · 2016-02-27T15:11:32Z

retest this please.

liancheng · 2016-02-27T15:15:41Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/OuterScopes.scala

+    if (outer == null) {
+      outerCls match {
+        case REPLClass(line) =>
+          val cls1 = Utils.classForName(line + "$read$")


Let's add a comment here to explain the REPL use case.

SparkQA · 2016-02-27T17:27:47Z

Test build #52129 has finished for PR 11410 at commit a9d832b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- //INSTANCE()method to get the single instance of class$read. Then call$iw()``

SparkQA · 2016-02-27T17:33:35Z

Test build #52126 has finished for PR 11410 at commit a0a8424.

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

cloud-fan · 2016-02-28T05:10:54Z

retest this please

SparkQA · 2016-02-28T07:27:13Z

Test build #52137 has finished for PR 11410 at commit a9d832b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- //INSTANCE()method to get the single instance of class$read. Then call$iw()``

marmbrus · 2016-02-29T18:21:33Z

repl/scala-2.11/src/test/scala/org/apache/spark/repl/ReplSuite.scala

+        |}.toColumn
+        |
+        |val ds = Seq(1, 2, 3, 4).toDS()
+        |ds.select(simpleSum).collect


I think your solution works, but it might be good to add a test case where the scope that gets captured has a side effect (i.e. create a file so if you double execute the outer scope it will fail the second time).

I tried to add such a test, but couldn't figure it out. The outer scope is the line wrapper class that generated by scala REPL framework and I'm not sure how to insert side effect into it.

I was just thinking you do something like create a file and then make sure that file handle gets used in the closure. If you got something wrong the second time the file is created it will fail.

SparkQA · 2016-03-01T10:30:19Z

Test build #52228 has finished for PR 11410 at commit f44e479.

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

SparkQA · 2016-03-02T05:54:39Z

Test build #52286 has finished for PR 11410 at commit a0510c4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- //INSTANCE()method to get the single instance of class$read. Then call$iw()``

cloud-fan · 2016-03-02T07:04:49Z

retest this please

SparkQA · 2016-03-02T09:32:00Z

Test build #52303 has finished for PR 11410 at commit a0510c4.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- //INSTANCE()method to get the single instance of class$read. Then call$iw()``

cloud-fan · 2016-03-08T08:27:32Z

retest this please

SparkQA · 2016-03-08T11:40:09Z

Test build #52650 has finished for PR 11410 at commit 8b79215.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- |createFile();case class TestCaseClass(value: Int)

yhuai · 2016-03-08T21:51:50Z

@retronym Will have time to do another round of review? Thank you!

SparkQA · 2016-03-09T04:25:28Z

Test build #52715 has finished for PR 11410 at commit 9be89ff.

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

cloud-fan · 2016-03-11T02:10:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/OuterScopes.scala

+            obj = getter.invoke(obj)
+            getter = iwGetter(getter.getReturnType)
+          }
+


hi @retronym , now I loop until there is no $iw method, is this looks good to you?

Yes, this looks cleaner to me.

yhuai · 2016-03-11T04:31:00Z

@retronym Thank you for the review! I'd like to merge this fix. Sounds good?

cloud-fan · 2016-03-15T04:00:16Z

retest this please

SparkQA · 2016-03-15T05:48:16Z

Test build #53157 has finished for PR 11410 at commit 9be89ff.

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

retronym · 2016-03-15T05:54:05Z

Since offering review, I've found a more direct way to get to the object.

Welcome to Scala 2.11.8 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_71).
Type in expressions for evaluation. Or try :help.

scala> $intp.isettings.unwrapStrings = false
$intp.isettings.unwrapStrings: Boolean = false

scala> var foo = ""
foo: String = ""

scala> foo = "2"
foo: String = 2

scala> val last = $intp.lastRequest
last: $intp.Request = Request(line=foo = "2", 1 trees)

scala> last.fullAccessPath
res1: String = $line4.$read.$iw.$iw

scala> val iw = $line4.$read.$iw.$iw
iw: $line4.$read.$iw.$iw.type = $line4.$read$$iw$$iw$@23444a36

scala> iw.getClass.getMethods.find(_.getName.startsWith("$ires")).head.invoke(iw)
res2: Object = 2

scala> def nullIRes(x: Any) = { val cls = x.getClass; cls.getDeclaredFields.filter(_.getName.startsWith("$ires")).foreach{fld => fld.setAccessible(true); fld.set(x, null)}}
nullIRes: (x: Any)Unit

scala> $intp.interpret(s"nullIRes(${last.fullAccessPath})")
res4: scala.tools.nsc.interpreter.IR.Result = Success

scala> iw.getClass.getMethods.find(_.getName.startsWith("$ires")).head.invoke(iw)
res6: Object = null

cloud-fan · 2016-03-15T06:57:22Z

Hi @retronym , your idea looks good, but in our situation, we only have a class name string and it's not that easy to get the line object. BTW the REPL classes are loaded dynamically in Spark, I think we have to use reflection to work on them. Any ideas?

rxin · 2016-03-18T07:39:36Z

ping @retronym again

cloud-fan · 2016-03-18T07:52:48Z

retest this please.

SparkQA · 2016-03-18T09:44:46Z

Test build #53520 has finished for PR 11410 at commit 9be89ff.

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

retronym · 2016-03-18T12:33:17Z

So long as you've got some defence from a test case, your current solution seems okay.

SparkQA · 2016-03-18T17:08:38Z

Test build #53541 has finished for PR 11410 at commit 41c2cc5.

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

SparkQA · 2016-03-21T02:42:26Z

Test build #53645 has finished for PR 11410 at commit 47e0e39.

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

cloud-fan · 2016-03-21T02:52:08Z

retest this please

SparkQA · 2016-03-21T05:00:21Z

Test build #53651 has finished for PR 11410 at commit 47e0e39.

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

cloud-fan · 2016-03-21T05:39:54Z

retest this please

SparkQA · 2016-03-21T07:33:59Z

Test build #53658 has finished for PR 11410 at commit 47e0e39.

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

cloud-fan · 2016-03-21T07:55:55Z

retest this please.

SparkQA · 2016-03-21T10:17:08Z

Test build #53666 has finished for PR 11410 at commit 47e0e39.

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

cloud-fan · 2016-03-21T11:51:05Z

cc @rxin

yhuai · 2016-03-21T17:36:19Z

Thanks. Merging to master.

…Spark shell ## What changes were proposed in this pull request? case classes defined in REPL are wrapped by line classes, and we have a trick for scala 2.10 REPL to automatically register the wrapper classes to `OuterScope` so that we can use when create encoders. However, this trick doesn't work right after we upgrade to scala 2.11, and unfortunately the tests are only in scala 2.10, which makes this bug hidden until now. This PR moves the encoder tests to scala 2.11 `ReplSuite`, and fixes this bug by another approach(the previous trick can't port to scala 2.11 REPL): make `OuterScope` smarter that can detect classes defined in REPL and load the singleton of line wrapper classes automatically. ## How was this patch tested? the migrated encoder tests in `ReplSuite` Author: Wenchen Fan <wenchen@databricks.com> Closes apache#11410 from cloud-fan/repl.

…ase class defined in REPL ## What changes were proposed in this pull request? In apache#11410, we missed a corner case: define the inner class and use it in `Dataset` at the same time by using paste mode. For this case, the inner class and the `Dataset` are inside same line object, when we build the `Dataset`, we try to get outer pointer from line object, and it will fail because the line object is not initialized yet. https://issues.apache.org/jira/browse/SPARK-13456?focusedCommentId=15209174&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15209174 is an example for this corner case. This PR make the process of getting outer pointer from line object lazy, so that we can successfully build the `Dataset` and finish initializing the line object. ## How was this patch tested? new test in repl suite. Author: Wenchen Fan <wenchen@databricks.com> Closes apache#11931 from cloud-fan/repl.

cloud-fan reviewed Feb 27, 2016
View reviewed changes

liancheng reviewed Feb 27, 2016
View reviewed changes

cloud-fan force-pushed the repl branch from cac4a21 to a9d832b Compare February 27, 2016 15:33

marmbrus reviewed Feb 29, 2016
View reviewed changes

cloud-fan force-pushed the repl branch from 2c97f5a to a0510c4 Compare March 2, 2016 03:58

cloud-fan reviewed Mar 11, 2016
View reviewed changes

cloud-fan added 6 commits March 18, 2016 23:16

fix creating encoders for case classes defined in Spark shell

5185e82

fix style

f3ec377

add comment

0073545

more robust

a89ffd0

add test

2699052

address comments

41c2cc5

cloud-fan force-pushed the repl branch from 9be89ff to 41c2cc5 Compare March 18, 2016 15:20

update

47e0e39

asfgit closed this in 43ebf7a Mar 21, 2016

cloud-fan mentioned this pull request Mar 24, 2016

[SPARK-13456][SQL][follow-up] lazily generate the outer pointer for case class defined in REPL #11931

Closed

[SPARK-13456][SQL] fix creating encoders for case classes defined in Spark shell #11410

[SPARK-13456][SQL] fix creating encoders for case classes defined in Spark shell #11410

Uh oh!

Conversation

cloud-fan commented Feb 27, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Feb 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 27, 2016

Uh oh!

SparkQA commented Feb 27, 2016

Uh oh!

cloud-fan commented Feb 27, 2016

Uh oh!

SparkQA commented Feb 27, 2016

Uh oh!

cloud-fan commented Feb 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 27, 2016

Uh oh!

SparkQA commented Feb 27, 2016

Uh oh!

cloud-fan commented Feb 28, 2016

Uh oh!

SparkQA commented Feb 28, 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!

SparkQA commented Mar 1, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

cloud-fan commented Mar 2, 2016

Uh oh!

SparkQA commented Mar 2, 2016

Uh oh!

cloud-fan commented Mar 8, 2016

Uh oh!

SparkQA commented Mar 8, 2016

Uh oh!

yhuai commented Mar 8, 2016

Uh oh!

SparkQA commented Mar 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhuai commented Mar 11, 2016

Uh oh!

cloud-fan commented Mar 15, 2016

Uh oh!

SparkQA commented Mar 15, 2016

Uh oh!

retronym commented Mar 15, 2016

Uh oh!

cloud-fan commented Mar 15, 2016

Uh oh!

rxin commented Mar 18, 2016

Uh oh!

cloud-fan commented Mar 18, 2016

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!

retronym commented Mar 18, 2016

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!