-
Notifications
You must be signed in to change notification settings - Fork 28.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-30090][SHELL] Adapt Spark REPL to Scala 2.13 #28545
Conversation
Yea this is fine as a test, but in the end we will need to make the .toSeq / .toMap changes first I think, to get it close to compilable. (And we need a new profile rather than modify the _2.12 -> _2.13 etc) Thanks! please test away. I was thinking we'd get 3.0 out the door before returning to 2.13 work, but OK to start on the few big last chunky changes. I have a branch for .toSeq somewhere but it's already out of date. |
(I'd ask it to test but there is no 2.13 build available, so it will just fail) |
patch LGTM. we expect to roll Scala 2.13.3 in about two weeks, so there's an opportunity to get the change in now. (2.13.4 will likely follow circa early August, though our release scheduling is malleable depending on what issues are reported. in any case, we publish nightlies, so development work needn't wait on our releases) |
Did you try writing the auto-run script to a temporary file, calling |
I considered it, but then a new temp file would be created every time the REPL is started... It would work probably. Also, I thought that maybe this setting should be left untouched to the user. |
Hi, @karolchmist . Thank you for contribution. |
Hey @dongjoon-hyun , I'd gladly resume it. Is there a branch I should rebase my work on? My current branch is based on BTW, Scala 2.13.3 was just released, containing the proposed change to make this PR work. |
You can just add more commits to the branch in this PR, if possible; it's already based on apache/master, which is what we need here. You can open a new PR too. That'd be great if you can get the 2.13 REPL working and I can review. |
@karolchmist is this something you'd still be willing to continue? Would be fantastic to have support for Scala 2.13 in Spark 3.1.0! |
Yeah we are pretty close to done otherwise - it should all compile, tests pass up through REPL at least. Should be open for anyone to take a run at. |
I'm on it! |
I can't build master with scala-2.13... (It builds with 2.12). Any ideas why? Maybe there is now a CI job for Scala 2.13?
|
Try |
Thanks @srowen, it worked better, but now it fails in
|
Yeah, we're not tracking/enforcing 2.13 compilation in CI/CD yet, so it's possible that recent changes will occasionally break 2.13 compilation. Looks like this happened in SPARK-32364. You are welcome to open a follow up PR for that JIRA to fix this - or I am happy to. I think the problem is that the map in question is now CaseInsensitiveMap, which is immutable, but used with |
I'll fix it in a new PR 👍 |
### What changes were proposed in this pull request? This is a follow-up of #29160. This allows Spark SQL project to compile for Scala 2.13. ### Why are the changes needed? It's needed for #28545 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? I compiled with Scala 2.13. It fails in `Spark REPL` project, which will be fixed by #28545 Closes #29584 from karolchmist/SPARK-32364-scala-2.13. Authored-by: Karol Chmist <info+github@chmist.com> Signed-off-by: Sean Owen <srowen@gmail.com>
@karolchmist your PR is merged; hope that helps unblock you |
d975102
to
b385521
Compare
6727171
to
b0842ee
Compare
994f58a
to
a4f42e9
Compare
Hello @srowen , for me the PR is ready to be reviewed. I extracted a Scala 2.12/2.13 specific code to separate directories, as the interface of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good if it works!
@@ -86,31 +84,6 @@ class ReplSuite extends SparkFunSuite with BeforeAndAfterAll { | |||
"Interpreter output contained '" + message + "':\n" + output) | |||
} | |||
|
|||
test("propagation of local properties") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test just obsolete now? Just want to understand where we might be taking away tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to Repl2Suite (because it's different between Scala 2.12 and 2.13)
Jenkins test this please |
Test build #128527 has finished for PR 28545 at commit
|
Looks good, as long as 2.12 still works, we can yet fix any other small issues with the REPL in 2.13 as we enable tests later in the 3.1.0 cycle. Thanks a lot! this was one of the few remaining pieces. |
…e/Repl2Suite` back into `SingletonReplSuite/ReplSuite` ### What changes were proposed in this pull request? This pr aims to merge test cases from `SingletonRepl2Suite/Repl2Suite` back into `SingletonReplSuite/ReplSuite` to reduce duplicate code. ### Why are the changes needed? #28545 split the relevant test cases from `SingletonReplSuite/ReplSuite` into `SingletonRepl2Suite/Repl2Suite`, distinguishing different test versions of Scala 2.12 and Scala 2.13. Currently, Spark 4.0 no longer supports Scala 2.12, so they can be merged back into the original files to reduce duplicate code. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass GitHub Actions ### Was this patch authored or co-authored using generative AI tooling? No Closes #43104 from LuciferYang/SPARK-45318. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
What changes were proposed in this pull request?
This is an attempt to adapt Spark REPL to Scala 2.13.
It is based on a scala-2.13 branch made by @smarter.
I had to set Scala version to 2.13 in some places, and to adapt some other modules, before I could start working on the REPL itself. These are separate commits on the branch that probably would be fixed beforehand, and thus dropped before the merge of this PR.
I couldn't find a way to run the initialization code with existing REPL classes in Scala 2.13.2, so I modified REPL in Scala to make it work. With this modification I managed to run Spark Shell, along with the units tests passing, which is good news.
The bad news is that it requires an upstream change in Scala, which must be accepted first. I'd be happy to change it if someone points a way to do it differently. If not, I'd propose a PR in Scala to introduce
ILoop.internalReplAutorunCode
.Why are the changes needed?
REPL in Scala changed quite a lot, so current version of Spark REPL needed to be adapted.
Does this PR introduce any user-facing change?
In the previous version of
SparkILoop
, a lot of Scala'sILoop
code was overridden and duplicated to make the welcome message a bit more pleasant. In this PR, the message is in a bit different order, but it's still acceptable IMHO.Before this PR:
With this PR:
It seems that currently the welcoming message is still an improvement from the original ticket, albeit in a different order. As a bonus, some fragile code duplication was removed.
How was this patch tested?
Existing tests pass in
repl
module. The REPL runs in a terminal and the following code executed correctly: