[SPARK-7527][Core]Fix createNullValue to return the correct null values and REPL mode detection #6735

zsxwing · 2015-06-10T02:57:40Z

The root cause of SPARK-7527 is createNullValue returns an incompatible value Byte(0) for char and boolean.

This PR fixes it and corrects the class name of the main class, and also adds an unit test to demonstrate it.

…etection

zsxwing · 2015-06-10T02:58:06Z

/cc @andrewor14 @srowen @preeze

SparkQA · 2015-06-10T04:12:58Z

Test build #34557 has finished for PR 6735 at commit 5f92dc1.

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

zsxwing · 2015-06-10T06:11:51Z

retest this please

SparkQA · 2015-06-10T07:20:24Z

Test build #34564 has finished for PR 6735 at commit 5f92dc1.

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

srowen · 2015-06-10T09:40:38Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

@@ -109,7 +109,16 @@ private[spark] object ClosureCleaner extends Logging {

  private def createNullValue(cls: Class[_]): AnyRef = {
    if (cls.isPrimitive) {
-      new java.lang.Byte(0: Byte) // Should be convertible to any primitive type
+      if (cls == java.lang.Boolean.TYPE) {


Can you use match syntax here?
I see why boolean needs a special case, but a byte widens to a char in Java so I don't think that's necessary?

The following code will fail:

case class X(x: Char) object Foo1 extends App { val c = classOf[X] val x = c.getConstructors()(0) println(x) println(x.newInstance(new java.lang.Byte(0: Byte))) }

I don't know why char is also a special case.

srowen · 2015-06-10T09:44:04Z

Interesting, I'd love to hear that this is the fix. The current failure is:

[info] - clean complicated nested non-serializable closures *** FAILED *** (15 milliseconds)
[info]   Expected exception org.apache.spark.SparkException to be thrown, but java.lang.reflect.InvocationTargetException was thrown. (ClosureCleanerSuite2.scala:98)
...

[info]   Cause: java.lang.NullPointerException:
[info]   at org.apache.spark.util.ClosureCleanerSuite2$$anonfun$36.<init>(ClosureCleanerSuite2.scala:512)

zsxwing · 2015-06-10T09:56:25Z

It's because enclosingObject in instantiateClass is null. The bytecodes in constructor will check the first parameter:

  public org.apache.spark.streaming.StreamingContext$$anonfun$binaryRecordsStream$1(org.apache.spark.streaming.StreamingContext, java.lang.String, int);
    Code:
       0: aload_1
       1: ifnonnull     12
       4: new           #119                // class java/lang/NullPointerException
       7: dup
       8: invokespecial #122                // Method java/lang/NullPointerException."<init>":()V
      11: athrow
      12: aload_0
      13: aload_1
      14: putfield      #27                 // Field $outer:Lorg/apache/spark/streaming/StreamingContext;
      17: aload_0
      18: aload_2
      19: putfield      #57                 // Field directory$2:Ljava/lang/String;
      22: aload_0
      23: iload_3
      24: putfield      #49                 // Field recordLength$1:I
      27: aload_0
      28: invokespecial #123                // Method scala/runtime/AbstractFunction0."<init>":()V
      31: return

Before this fix, Utils.isInInterpreter always returns true because of the wrong class name... Now the code path of Utils.isInInterpreter == false is activated.

I'm looking for a fix for this NullPointerException.

zsxwing · 2015-06-10T12:15:23Z

The issue is that if a closure is cleaned twice, when cleaning the closure at the second time, the $outer of this closure may have already been set to null. Then when we call instantiateClass for this closure, enclosingObject will be null because we cannot find a parent (https://github.com/zsxwing/spark/blob/SPARK-7527/core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala#L264).

Because the compiler always checks the first parameter in the constructor, so NullPointerException is thrown.

Since the code for Utils.isInInterpreter == false is not used and nobody reports any issue, how about just removing it and always using the code for Utils.isInInterpreter == true?

srowen · 2015-06-10T12:29:40Z

Heh, well it's supposed to be used when in the spark-shell I suppose. However I agree that it seems to have not been used for a long time, accidentally.

I don't know the history of this bit of code. I am not sure about the assumption in the comments that the constructor has no effects -- did it mean a case class? It sort of looks like an optimization, to try to call the constructor directly via reflection.

Well... I think it wouldn't hurt to try removing it as you say and see what happens?

SparkQA · 2015-06-10T15:06:31Z

Test build #34595 has finished for PR 6735 at commit bbdb271.

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

SparkQA · 2015-06-10T15:18:41Z

Test build #34594 has finished for PR 6735 at commit b0a0e7e.

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

andrewor14 · 2015-06-10T18:44:56Z

core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

+        case java.lang.Boolean.TYPE => new java.lang.Boolean(false)
+        case java.lang.Character.TYPE => new java.lang.Character('\0')
+        case java.lang.Void.TYPE =>
+          // This should not happen because `Foo(void x) {}` cannot be compiled.


nit: cannot be compiled -> does not compile

andrewor14 · 2015-06-10T18:56:06Z

LGTM. All of my comments are documentation related. I will fix these on merge myself.

This is going into master.

srowen · 2015-06-10T19:05:24Z

core/src/main/scala/org/apache/spark/util/Utils.scala

@@ -1804,15 +1804,10 @@ private[spark] object Utils extends Logging {

  lazy val isInInterpreter: Boolean = {


Can we just delete this now?

Let's keep this for now. I remember there's some discussion saying that this should be abstracted to a shared method so it can be used elsewhere.

in fact #6734 uses it.

Ah right, now I remember that. Now the logic is correct, hopefully.

…lues and REPL mode detection The root cause of SPARK-7527 is `createNullValue` returns an incompatible value `Byte(0)` for `char` and `boolean`. This PR fixes it and corrects the class name of the main class, and also adds an unit test to demonstrate it. Author: zsxwing <zsxwing@gmail.com> Closes apache#6735 from zsxwing/SPARK-7527 and squashes the following commits: bbdb271 [zsxwing] Use pattern match in createNullValue b0a0e7e [zsxwing] Remove the noisy in the test output 903e269 [zsxwing] Remove the code for Utils.isInInterpreter == false 5f92dc1 [zsxwing] Fix createNullValue to return the correct null values and REPL mode detection

…lues and REPL mode detection The root cause of SPARK-7527 is `createNullValue` returns an incompatible value `Byte(0)` for `char` and `boolean`. This PR fixes it and corrects the class name of the main class, and also adds an unit test to demonstrate it. Author: zsxwing <zsxwing@gmail.com> Closes #6735 from zsxwing/SPARK-7527 and squashes the following commits: bbdb271 [zsxwing] Use pattern match in createNullValue b0a0e7e [zsxwing] Remove the noisy in the test output 903e269 [zsxwing] Remove the code for Utils.isInInterpreter == false 5f92dc1 [zsxwing] Fix createNullValue to return the correct null values and REPL mode detection

Fix createNullValue to return the correct null values and REPL mode d…

5f92dc1

…etection

pwendell mentioned this pull request Jun 10, 2015

[SPARK-7261][Core] Change default log level to WARN in the REPL #6734

Closed

srowen reviewed Jun 10, 2015
View reviewed changes

zsxwing added 3 commits June 10, 2015 20:41

Remove the code for Utils.isInInterpreter == false

903e269

Remove the noisy in the test output

b0a0e7e

Use pattern match in createNullValue

bbdb271

andrewor14 reviewed Jun 10, 2015
View reviewed changes

srowen reviewed Jun 10, 2015
View reviewed changes

asfgit closed this in e90c9d9 Jun 10, 2015

zsxwing deleted the SPARK-7527 branch June 11, 2015 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-7527][Core]Fix createNullValue to return the correct null values and REPL mode detection #6735

[SPARK-7527][Core]Fix createNullValue to return the correct null values and REPL mode detection #6735

zsxwing commented Jun 10, 2015

zsxwing commented Jun 10, 2015

SparkQA commented Jun 10, 2015

zsxwing commented Jun 10, 2015

SparkQA commented Jun 10, 2015

srowen Jun 10, 2015

zsxwing Jun 10, 2015

srowen commented Jun 10, 2015

zsxwing commented Jun 10, 2015

zsxwing commented Jun 10, 2015

srowen commented Jun 10, 2015

SparkQA commented Jun 10, 2015

SparkQA commented Jun 10, 2015

andrewor14 Jun 10, 2015

andrewor14 commented Jun 10, 2015

srowen Jun 10, 2015

andrewor14 Jun 10, 2015

andrewor14 Jun 10, 2015

srowen Jun 10, 2015

		@@ -1804,15 +1804,10 @@ private[spark] object Utils extends Logging {

		lazy val isInInterpreter: Boolean = {

[SPARK-7527][Core]Fix createNullValue to return the correct null values and REPL mode detection #6735

[SPARK-7527][Core]Fix createNullValue to return the correct null values and REPL mode detection #6735

Conversation

zsxwing commented Jun 10, 2015

zsxwing commented Jun 10, 2015

SparkQA commented Jun 10, 2015

zsxwing commented Jun 10, 2015

SparkQA commented Jun 10, 2015

srowen Jun 10, 2015

Choose a reason for hiding this comment

zsxwing Jun 10, 2015

Choose a reason for hiding this comment

srowen commented Jun 10, 2015

zsxwing commented Jun 10, 2015

zsxwing commented Jun 10, 2015

srowen commented Jun 10, 2015

SparkQA commented Jun 10, 2015

SparkQA commented Jun 10, 2015

andrewor14 Jun 10, 2015

Choose a reason for hiding this comment

andrewor14 commented Jun 10, 2015

srowen Jun 10, 2015

Choose a reason for hiding this comment

andrewor14 Jun 10, 2015

Choose a reason for hiding this comment

andrewor14 Jun 10, 2015

Choose a reason for hiding this comment

srowen Jun 10, 2015

Choose a reason for hiding this comment