Skip to content

Commit

Permalink
SPARK-2632, SPARK-2576. Fixed by only importing what is necessary dur…
Browse files Browse the repository at this point in the history
…ing class definition.

Without this patch, it imports everything available in the scope.

```scala

scala> val a = 10l
val a = 10l
a: Long = 10

scala> import a._
import a._
import a._

scala> case class A(a: Int) // show
case class A(a: Int) // show
class $read extends Serializable {
  def <init>() = {
    super.<init>;
    ()
  };
  class $iwC extends Serializable {
    def <init>() = {
      super.<init>;
      ()
    };
    class $iwC extends Serializable {
      def <init>() = {
        super.<init>;
        ()
      };
      import org.apache.spark.SparkContext._;
      class $iwC extends Serializable {
        def <init>() = {
          super.<init>;
          ()
        };
        val $VAL5 = $line5.$read.INSTANCE;
        import $VAL5.$iw.$iw.$iw.$iw.a;
        class $iwC extends Serializable {
          def <init>() = {
            super.<init>;
            ()
          };
          import a._;
          class $iwC extends Serializable {
            def <init>() = {
              super.<init>;
              ()
            };
            class $iwC extends Serializable {
              def <init>() = {
                super.<init>;
                ()
              };
              case class A extends scala.Product with scala.Serializable {
                <caseaccessor> <paramaccessor> val a: Int = _;
                def <init>(a: Int) = {
                  super.<init>;
                  ()
                }
              }
            };
            val $iw = new $iwC.<init>
          };
          val $iw = new $iwC.<init>
        };
        val $iw = new $iwC.<init>
      };
      val $iw = new $iwC.<init>
    };
    val $iw = new $iwC.<init>
  };
  val $iw = new $iwC.<init>
}
object $read extends scala.AnyRef {
  def <init>() = {
    super.<init>;
    ()
  };
  val INSTANCE = new $read.<init>
}
defined class A
```

With this patch, it just imports  only the necessary.

```scala

scala> val a = 10l
val a = 10l
a: Long = 10

scala> import a._
import a._
import a._

scala> case class A(a: Int) // show
case class A(a: Int) // show
class $read extends Serializable {
  def <init>() = {
    super.<init>;
    ()
  };
  class $iwC extends Serializable {
    def <init>() = {
      super.<init>;
      ()
    };
    class $iwC extends Serializable {
      def <init>() = {
        super.<init>;
        ()
      };
      case class A extends scala.Product with scala.Serializable {
        <caseaccessor> <paramaccessor> val a: Int = _;
        def <init>(a: Int) = {
          super.<init>;
          ()
        }
      }
    };
    val $iw = new $iwC.<init>
  };
  val $iw = new $iwC.<init>
}
object $read extends scala.AnyRef {
  def <init>() = {
    super.<init>;
    ()
  };
  val INSTANCE = new $read.<init>
}
defined class A

scala>

```

This patch also adds a `:fallback` mode on being enabled it will restore the spark-shell's 1.0.0 behaviour.

Author: Prashant Sharma <scrapcodes@gmail.com>
Author: Yin Huai <huai@cse.ohio-state.edu>
Author: Prashant Sharma <prashant.s@imaginea.com>

Closes apache#1635 from ScrapCodes/repl-fix-necessary-imports and squashes the following commits:

b1968d2 [Prashant Sharma] Added toschemaRDD to test case.
0b712bb [Yin Huai] Add a REPL test to test importing a method.
02ad8ff [Yin Huai] Add a REPL test for importing SQLContext.createSchemaRDD.
ed6d0c7 [Prashant Sharma] Added a fallback mode, incase users run into issues while using repl.
b63d3b2 [Prashant Sharma] SPARK-2632, SPARK-2576. Fixed by only importing what is necessary during class definition.
  • Loading branch information
ScrapCodes authored and marmbrus committed Aug 1, 2014
1 parent 2cdc3e5 commit 1499101
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 5 deletions.
6 changes: 6 additions & 0 deletions repl/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@
<version>${project.version}</version>
<scope>runtime</scope>
</dependency>
<dependency>
<groupId>org.apache.spark</groupId>
<artifactId>spark-sql_${scala.binary.version}</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-server</artifactId>
Expand Down
17 changes: 17 additions & 0 deletions repl/src/main/scala/org/apache/spark/repl/SparkILoop.scala
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,20 @@ class SparkILoop(in0: Option[BufferedReader], protected val out: JPrintWriter,
case xs => xs find (_.name == cmd)
}
}
private var fallbackMode = false

private def toggleFallbackMode() {
val old = fallbackMode
fallbackMode = !old
System.setProperty("spark.repl.fallback", fallbackMode.toString)
echo(s"""
|Switched ${if (old) "off" else "on"} fallback mode without restarting.
| If you have defined classes in the repl, it would
|be good to redefine them incase you plan to use them. If you still run
|into issues it would be good to restart the repl and turn on `:fallback`
|mode as first command.
""".stripMargin)
}

/** Show the history */
lazy val historyCommand = new LoopCommand("history", "show the history (optional num is commands to show)") {
Expand Down Expand Up @@ -299,6 +313,9 @@ class SparkILoop(in0: Option[BufferedReader], protected val out: JPrintWriter,
nullary("reset", "reset the repl to its initial state, forgetting all session entries", resetCommand),
shCommand,
nullary("silent", "disable/enable automatic printing of results", verbosity),
nullary("fallback", """
|disable/enable advanced repl changes, these fix some issues but may introduce others.
|This mode will be removed once these fixes stablize""".stripMargin, toggleFallbackMode),
cmd("type", "[-v] <expr>", "display the type of an expression without evaluating it", typeCommand),
nullary("warnings", "show the suppressed warnings from the most recent line which had any", warningsCommand)
)
Expand Down
7 changes: 6 additions & 1 deletion repl/src/main/scala/org/apache/spark/repl/SparkIMain.scala
Original file line number Diff line number Diff line change
Expand Up @@ -892,11 +892,16 @@ import org.apache.spark.util.Utils
def definedTypeSymbol(name: String) = definedSymbols(newTypeName(name))
def definedTermSymbol(name: String) = definedSymbols(newTermName(name))

val definedClasses = handlers.exists {
case _: ClassHandler => true
case _ => false
}

/** Code to import bound names from previous lines - accessPath is code to
* append to objectName to access anything bound by request.
*/
val SparkComputedImports(importsPreamble, importsTrailer, accessPath) =
importsCode(referencedNames.toSet)
importsCode(referencedNames.toSet, definedClasses)

/** Code to access a variable with the specified name */
def fullPath(vname: String) = {
Expand Down
15 changes: 11 additions & 4 deletions repl/src/main/scala/org/apache/spark/repl/SparkImports.scala
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,9 @@ trait SparkImports {
* last one imported is actually usable.
*/
case class SparkComputedImports(prepend: String, append: String, access: String)
def fallback = System.getProperty("spark.repl.fallback", "false").toBoolean

protected def importsCode(wanted: Set[Name]): SparkComputedImports = {
protected def importsCode(wanted: Set[Name], definedClass: Boolean): SparkComputedImports = {
/** Narrow down the list of requests from which imports
* should be taken. Removes requests which cannot contribute
* useful imports for the specified set of wanted names.
Expand All @@ -124,8 +125,14 @@ trait SparkImports {
// Single symbol imports might be implicits! See bug #1752. Rather than
// try to finesse this, we will mimic all imports for now.
def keepHandler(handler: MemberHandler) = handler match {
case _: ImportHandler => true
case x => x.definesImplicit || (x.definedNames exists wanted)
/* This case clause tries to "precisely" import only what is required. And in this
* it may miss out on some implicits, because implicits are not known in `wanted`. Thus
* it is suitable for defining classes. AFAIK while defining classes implicits are not
* needed.*/
case h: ImportHandler if definedClass && !fallback =>
h.importedNames.exists(x => wanted.contains(x))
case _: ImportHandler => true
case x => x.definesImplicit || (x.definedNames exists wanted)
}

reqs match {
Expand Down Expand Up @@ -182,7 +189,7 @@ trait SparkImports {
// ambiguity errors will not be generated. Also, quote
// the name of the variable, so that we don't need to
// handle quoting keywords separately.
case x: ClassHandler =>
case x: ClassHandler if !fallback =>
// I am trying to guess if the import is a defined class
// This is an ugly hack, I am not 100% sure of the consequences.
// Here we, let everything but "defined classes" use the import with val.
Expand Down
27 changes: 27 additions & 0 deletions repl/src/test/scala/org/apache/spark/repl/ReplSuite.scala
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,33 @@ class ReplSuite extends FunSuite {
assertDoesNotContain("error:", output)
assertDoesNotContain("Exception", output)
}

test("SPARK-2576 importing SQLContext.createSchemaRDD.") {
// We need to use local-cluster to test this case.
val output = runInterpreter("local-cluster[1,1,512]",
"""
|val sqlContext = new org.apache.spark.sql.SQLContext(sc)
|import sqlContext.createSchemaRDD
|case class TestCaseClass(value: Int)
|sc.parallelize(1 to 10).map(x => TestCaseClass(x)).toSchemaRDD.collect
""".stripMargin)
assertDoesNotContain("error:", output)
assertDoesNotContain("Exception", output)
}

test("SPARK-2632 importing a method from non serializable class and not using it.") {
val output = runInterpreter("local",
"""
|class TestClass() { def testMethod = 3 }
|val t = new TestClass
|import t.testMethod
|case class TestCaseClass(value: Int)
|sc.parallelize(1 to 10).map(x => TestCaseClass(x)).collect
""".stripMargin)
assertDoesNotContain("error:", output)
assertDoesNotContain("Exception", output)
}

if (System.getenv("MESOS_NATIVE_LIBRARY") != null) {
test("running on Mesos") {
val output = runInterpreter("localquiet",
Expand Down

0 comments on commit 1499101

Please sign in to comment.