[GitHub] spark pull request #22246: [WIP] [SPARK-25235] [SHELL] Merge the REPL code i...

2018-08-27 Thread viirya
Github user viirya commented on a diff in the pull request:

https://github.com/apache/spark/pull/22246#discussion_r213164929
  
--- Diff: repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala ---
@@ -124,6 +141,26 @@ class SparkILoop(in0: Option[BufferedReader], out: 
JPrintWriter)
 super.replay()
   }
 
+  /**
+   * TODO: Remove `runClosure` when the support of Scala 2.11 is ended
+   * In Scala 2.12, we don't need to use `savingContextLoader` to execute 
the `body`.
+   * See `SI-8521 No blind save of context class loader` for detail.
+   */
+  private def runClosure(body: () => Boolean): Boolean = {
+if (isScala2_11) {
+  val loader = 
Utils.classForName("scala.reflect.internal.util.ScalaClassLoader$")
+.getDeclaredField("MODULE$")
+.get(null)
--- End diff --

Although it is a static method in Scala, it will be compiled to a 
non-static method in Java class. This class has a static member of the same 
type. The access of all static methods are through the static member.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22246: [WIP] [SPARK-25235] [SHELL] Merge the REPL code i...

2018-08-27 Thread dbtsai
Github user dbtsai commented on a diff in the pull request:

https://github.com/apache/spark/pull/22246#discussion_r213107503
  
--- Diff: repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala ---
@@ -124,6 +141,26 @@ class SparkILoop(in0: Option[BufferedReader], out: 
JPrintWriter)
 super.replay()
   }
 
+  /**
+   * TODO: Remove `runClosure` when the support of Scala 2.11 is ended
+   * In Scala 2.12, we don't need to use `savingContextLoader` to execute 
the `body`.
+   * See `SI-8521 No blind save of context class loader` for detail.
+   */
+  private def runClosure(body: () => Boolean): Boolean = {
+if (isScala2_11) {
+  val loader = 
Utils.classForName("scala.reflect.internal.util.ScalaClassLoader$")
+.getDeclaredField("MODULE$")
+.get(null)
--- End diff --

@viirya Out of my curiosity, since it's a static method, in theory, in the 
`invoke`, the first arg should be `null`. Why do you put the loader in? Also, 
this doesn't work in tests, and we get the method not found exception.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22246: [WIP] [SPARK-25235] [SHELL] Merge the REPL code i...

2018-08-27 Thread dbtsai
Github user dbtsai commented on a diff in the pull request:

https://github.com/apache/spark/pull/22246#discussion_r213107072
  
--- Diff: repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala ---
@@ -148,9 +148,13 @@ class SparkILoop(in0: Option[BufferedReader], out: 
JPrintWriter)
*/
   private def runClosure(body: () => Boolean): Boolean = {
 if (isScala2_11) {
+  val loader = 
Utils.classForName("scala.reflect.internal.util.ScalaClassLoader$")
+.getDeclaredField("MODULE$")
+.get(null)
+
   Utils.classForName("scala.reflect.internal.util.ScalaClassLoader$")
 .getDeclaredMethod("savingContextLoader", classOf[() => Boolean])
-.invoke(null, body)
+.invoke(loader, body)
--- End diff --

@viirya Out of my curiosity, since it's a static method, in theory, in the 
`invoke`, the first arg should be `null`. Why do you put the loader in?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org