Skip to content

Conversation

@zjffdu
Copy link
Contributor

@zjffdu zjffdu commented Jun 19, 2018

What is this PR for?

This PR add support for scala 2.12 of SparkInterpreter. In this PR, I did some refactoring of whole spark modules. Each scala version interrpeter will be loaded dynamically via URLClassLoad, so that we can just write code once and compile it multiple times via different scala version and load it dynamically based on the current scala version.

What type of PR is it?

[Feature | Refactoring]

Todos

  • - Task

What is the Jira issue?

How should this be tested?

  • CI passed, UT of scala 2.12 is added and passed

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@zjffdu zjffdu changed the title ZEPPELIN-3552. Support Scala 2.12 of SparkInterpreter [WIP] ZEPPELIN-3552. Support Scala 2.12 of SparkInterpreter Jun 19, 2018
@felixcheung
Copy link
Member

hmm, Spark doesn't support Scala 2.12?

@zjffdu
Copy link
Contributor Author

zjffdu commented Jun 27, 2018

The first scala supported spark is 2.4.0 IIUC

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that there are a few unresolved subtasks that no one is working on at the moment, I'm not sure if this would be resolved by Spark 2.4.0 release. So my thought would be not yet for now.

https://issues.apache.org/jira/browse/SPARK-14220

@antonkulaga
Copy link

Spark 2.4 will officially support Scala 2.12, so it will be great if Zeppelin will support it together with Spark. And also, there are some libs that are Scala 2.12 only

@zjffdu
Copy link
Contributor Author

zjffdu commented Aug 21, 2018

We would try to support it after spark 2.4 release.

@zjffdu zjffdu mentioned this pull request Oct 17, 2018
1 task

sparkILoop.in = reader
sparkILoop.initializeSynchronous()
callMethod(sparkILoop, "scala$tools$nsc$interpreter$ILoop$$loopPostInit")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm .. weird. From my testing against Spark 2.4.0 RC, this failed to find the method because it became a nested function (https://github.com/scala/scala/blob/v2.12.6/src/repl/scala/tools/nsc/interpreter/ILoop.scala#L993). Let me try to manually test against this one.

import scala.tools.nsc.interpreter._

/**
* SparkInterpreter for scala-2.11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scala-2.11 -> scala-2.12 :-)

@HyukjinKwon
Copy link
Member

Spark 2.4 will officially support Scala 2.12, so it will be great if Zeppelin will support it together with Spark. And also, there are some libs that are Scala 2.12 only

The default distribution will still be with Scala 2.11 for Spark 2.4 if I am not mistaken. It is nice to support it but Spark 2.4 with 2.11 should be supported first as a higher priority. I can work on 2.4.0 with 2.12 support further after this one got merged.

@HyukjinKwon
Copy link
Member

Hey Jeff, I can take over this one too if you're busy since I took a look for similar code paths.

@conker84
Copy link
Contributor

conker84 commented Apr 2, 2019

Hi @felixcheung @zjffdu any news on this? Spark 3.0 (which will be out late of summer) seems to remove the support to v 2.11 so it would be great merge this ASAP

@HyukjinKwon
Copy link
Member

Sorry, actually I am stuck in some works .. I think I wouldn't be able to take over this.

@zjffdu
Copy link
Contributor Author

zjffdu commented Apr 3, 2019

Don't worry @HyukjinKwon let me continue this.

@zjffdu zjffdu force-pushed the ZEPPELIN-3552 branch 2 times, most recently from 03d3122 to 657a443 Compare April 3, 2019 05:10
@zjffdu zjffdu changed the title [WIP] ZEPPELIN-3552. Support Scala 2.12 of SparkInterpreter ZEPPELIN-3552. Support Scala 2.12 of SparkInterpreter Apr 3, 2019
@zjffdu zjffdu force-pushed the ZEPPELIN-3552 branch 12 times, most recently from 7581580 to 7feee7b Compare April 4, 2019 09:14
<scala.version>2.10.5</scala.version>
<scala.binary.version>2.10</scala.binary.version>
<scalatest.version>2.2.4</scalatest.version>
<scalatest.version>3.0.7</scalatest.version>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.0.7 support both scala 2.10/2.11/2.12

@felixcheung
Copy link
Member

whoa long time

@antonkulaga
Copy link

Scala 2.12 is no longer experimental in Spark 2.4.1 so I suggest targeting 2.4.1

@conker84
Copy link
Contributor

Any ETA for this?

@zjffdu
Copy link
Contributor Author

zjffdu commented Apr 19, 2019

@conker84 I make some updates to this PR, and it basically works in my local environment. But I still need more time to refine this PR. I suppose this could be done in 0.9.

@conker84
Copy link
Contributor

Ok thanks!
Spark 3.0 will add native Property Graph & Cypher Support, so we need this to start to test how Zeppelin behaves with it.
Please ping me when it will be merged!

@zjffdu zjffdu force-pushed the ZEPPELIN-3552 branch 2 times, most recently from 608046f to ddcf7b7 Compare April 27, 2019 13:35
@conker84
Copy link
Contributor

Any news on this?

@conker84
Copy link
Contributor

Ping

@zjffdu
Copy link
Contributor Author

zjffdu commented May 31, 2019

@conker84 I am busy on other stuff recently, but make some progress on this. The plan is still completing it before 0.9 which is planed to be released this summer.

@zjffdu zjffdu force-pushed the ZEPPELIN-3552 branch 2 times, most recently from 56e4c56 to ef2c6af Compare June 12, 2019 04:19
@zjffdu
Copy link
Contributor Author

zjffdu commented Jun 24, 2019

Folks, This PR is ready for review. Now scala 2.12 is supported.
@felixcheung @jongyoul @conker84 Could you help review or try this PR ? Thanks

@felixcheung
Copy link
Member

LGTM

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too

@zjffdu
Copy link
Contributor Author

zjffdu commented Jun 26, 2019

Will merge if no more comments

@asfgit asfgit closed this in f61bddd Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants