Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Aug 10, 2019

What changes were proposed in this pull request?

This PR upgrade Scala to 2.12.10.

Release notes:

  • Fix regression in large string interpolations with non-String typed splices

  • Revert "Generate shallower ASTs in pattern translation"

  • Fix regression in classpath when JARs have 'a.b' entries beside 'a/b'

  • Faster compiler: 5–10% faster since 2.12.8

  • Improved compatibility with JDK 11, 12, and 13

  • Experimental support for build pipelining and outline type checking

More details:
https://github.com/scala/scala/releases/tag/v2.12.10
https://github.com/scala/scala/releases/tag/v2.12.9

How was this patch tested?

Existing tests

@SparkQA
Copy link

SparkQA commented Aug 10, 2019

Test build #108914 has finished for PR 25404 at commit 839c27e.

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

@dongjoon-hyun
Copy link
Member

Retest this please.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 10, 2019

The failures seems relevant, @wangyum . Could you check the failures?

  • [info] *** 212 TESTS FAILED ***
[info] - Sort Array *** FAILED *** (10 milliseconds)
[info]   Code generation of sort_array([2,1,3], true) failed:
[info]   java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
[info]   java.lang.ClassCastException: java.lang.Integer cannot be cast to java.lang.String
...

@SparkQA
Copy link

SparkQA commented Aug 10, 2019

Test build #108924 has finished for PR 25404 at commit 839c27e.

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

@srowen
Copy link
Member

srowen commented Aug 11, 2019

Wow that's a weird one, but I played with it and, bizarrely this change in the generate code makes it work:

      } else if ($o2 == null) {
        return -$sortOrder * $nullOrder;
      }

to

      } else if ($o2 == null) {
        return -$sortOrder * ${nullOrder.toString};
      }

I can only guess something goes wrong in string interpolation because nullOrder is a NullOrder and typedef for an Int, and is boxed to Integer somewhere, but something believes it is a String.

... and then I decided to search for open bugs and yep this is it:
scala/bug#11665

We may just need to wait for 2.12.10

@wangyum
Copy link
Member Author

wangyum commented Aug 12, 2019

Thank you @srowen Let's wait for 2.12.10.

@wangyum wangyum closed this Aug 12, 2019
@wangyum wangyum deleted the SPARK-28683 branch August 12, 2019 00:06
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Aug 12, 2019

Hi, All. Let's reuse SPARK-28683 for 2.12.10. I reopened the JIRA issue for that.

@wangyum wangyum restored the SPARK-28683 branch September 11, 2019 04:47
@wangyum wangyum changed the title [SPARK-28683][BUILD] Upgrade Scala to 2.12.9 [SPARK-28683][BUILD] Upgrade Scala to 2.12.10 Sep 11, 2019
@wangyum wangyum reopened this Sep 11, 2019
@SparkQA
Copy link

SparkQA commented Sep 11, 2019

Test build #110459 has finished for PR 25404 at commit e5811fd.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Sep 11, 2019

sbt.ResolveException: unresolved dependency: com.typesafe.genjavadoc#genjavadoc-plugin_2.12.10;0.13: not found
	at sbt.IvyActions$.sbt$IvyActions$$resolve(IvyActions.scala:320)
	at sbt.IvyActions$$anonfun$updateEither$1.apply(IvyActions.scala:191)
	at sbt.IvyActions$$anonfun$updateEither$1.apply(IvyActions.scala:168)

Let's wait for genjavadoc-plugin_2.12.10: https://repo1.maven.org/maven2/com/typesafe/genjavadoc/

@srowen
Copy link
Member

srowen commented Sep 11, 2019

OK by me, once the javadoc plugin is available and tests pass. Looks like the problem with string interpolation was resolved.

@dongjoon-hyun
Copy link
Member

Thank you, @wangyum and @srowen . Yes. Let's retry when it becomes ready.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 12, 2019

Ur, it seems to be delayed due to the release manager issue. Thank you for pushing it, @wangyum !

@wangyum
Copy link
Member Author

wangyum commented Sep 17, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110780 has finished for PR 25404 at commit e5811fd.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member Author

wangyum commented Sep 17, 2019

genjavadoc 0.14 has released: https://repo1.maven.org/maven2/com/typesafe/genjavadoc/genjavadoc-plugin_2.12.10/0.14/

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110781 has finished for PR 25404 at commit 075134d.

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

@dongjoon-hyun
Copy link
Member

Finally! Thanks!

@dongjoon-hyun
Copy link
Member

Retest this please.

@SparkQA
Copy link

SparkQA commented Sep 17, 2019

Test build #110793 has started for PR 25404 at commit 075134d.

@srowen
Copy link
Member

srowen commented Sep 17, 2019

Well... that's weird. Lots of tests fail because they get slightly different answers, and almost all look like tests that depend on a seeded random number generator at some level.

Take https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/110781/testReport/test.org.apache.spark/JavaAPISuite/sample/ for example.

  @Test
  public void sample() {
    List<Integer> ints = IntStream.iterate(1, x -> x + 1)
      .limit(20)
      .boxed()
      .collect(Collectors.toList());
    JavaRDD<Integer> rdd = sc.parallelize(ints);
    // the seeds here are "magic" to make this work out nicely
    JavaRDD<Integer> sample20 = rdd.sample(true, 0.2, 8);
    assertEquals(2, sample20.count());
    JavaRDD<Integer> sample20WithoutReplacement = rdd.sample(false, 0.2, 2);
    assertEquals(4, sample20WithoutReplacement.count());
  }

It's seeded, so ought to produce the same answer. This has worked the same way for a long time, giving that particular answer with that particular seed. It's pretty straightforwardly using a java.util.Random, seeded. What could have changed?

At the moment I'm wondering if somehow a compiler or collections change causes the input to be partitioned differently or iterated over differently. Still thinking about it and reviewing the (pretty minor) Scala changes.

One way or the other I don't think we can let the behavior change for a seeded sample, but not clear whether it's some subtle assumption Spark makes or something else going on.

@SparkQA
Copy link

SparkQA commented Sep 18, 2019

Test build #110882 has finished for PR 25404 at commit da7b223.

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

@wangyum
Copy link
Member Author

wangyum commented Sep 18, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 18, 2019

Test build #110901 has finished for PR 25404 at commit da7b223.

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

@wangyum wangyum changed the title [SPARK-28683][BUILD][test-hadoop3.2][test-java11] Upgrade Scala to 2.12.10 [SPARK-28683][BUILD][test-hadoop3.2][test-maven] Upgrade Scala to 2.12.10 Sep 18, 2019
@wangyum
Copy link
Member Author

wangyum commented Sep 18, 2019

retest this please

@srowen
Copy link
Member

srowen commented Sep 18, 2019

@dongjoon-hyun ah OK, I see what you did now. By using the equivalent two-arg bytesHash method, the behavior didn't change. But the implementation of two-arg bytesHash didn't change in Scala 2.12.10 so it avoids a behavior change.

Looks promising that tests have passed here once.

@dongjoon-hyun
Copy link
Member

Yes. Right, @srowen !

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 18, 2019

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-28683][BUILD][test-hadoop3.2][test-maven] Upgrade Scala to 2.12.10 [SPARK-28683][BUILD] Upgrade Scala to 2.12.10 Sep 18, 2019
@dongjoon-hyun
Copy link
Member

Hmm. The last test has been running 5 hr 43 min already. It seems that we will hit timeout soon. :(

@dongjoon-hyun
Copy link
Member

The last run passed all Scala/Java UTs and moved to Python test.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  05:24 h
[INFO] Finished at: 2019-09-18T12:39:46-07:00
[INFO] ------------------------------------------------------------------------

@kiszk
Copy link
Member

kiszk commented Sep 18, 2019

LGTM, pending Jenkins

@dongjoon-hyun
Copy link
Member

I checked and noticed that the PR Builder timeout 430m. It looks safe. I was confused with the other Jenkins job configurations which are shorter than this.

@SparkQA
Copy link

SparkQA commented Sep 18, 2019

Test build #110911 has finished for PR 25404 at commit da7b223.

  • This patch fails from timeout after a configured wait of 400m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

Hmm. It's weird because it's terminated before the configuration.
Screen Shot 2019-09-18 at 1 28 27 PM

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/110911/
Test FAILed.

@dongjoon-hyun
Copy link
Member

Anyway, this Scala change looks safe to me. I'll merge this to master. Thank you, @wangyum , @srowen , @kiszk !

Please make a backport to branch-2.4.

@wangyum wangyum deleted the SPARK-28683 branch September 19, 2019 00:26
dongjoon-hyun pushed a commit that referenced this pull request Sep 19, 2019
### What changes were proposed in this pull request?

This PR backport #25404 to branch-2.4.

### Why are the changes needed?
Backport from master.

### Does this PR introduce any user-facing change?
No.

### How was this patch tested?
Existing tests

Closes #25839 from wangyum/SPARK-28683-branch-2.4.

Authored-by: Yuming Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
rshkv pushed a commit to palantir/spark that referenced this pull request Jun 18, 2020
This PR upgrade Scala to **2.12.10**.

Release notes:
- Fix regression in large string interpolations with non-String typed splices
- Revert "Generate shallower ASTs in pattern translation"
- Fix regression in classpath when JARs have 'a.b' entries beside 'a/b'

- Faster compiler: 5–10% faster since 2.12.8
- Improved compatibility with JDK 11, 12, and 13
- Experimental support for build pipelining and outline type checking

More details:
https://github.com/scala/scala/releases/tag/v2.12.10
https://github.com/scala/scala/releases/tag/v2.12.9

Existing tests

Closes apache#25404 from wangyum/SPARK-28683.

Authored-by: Yuming Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
rshkv pushed a commit to palantir/spark that referenced this pull request Jul 16, 2020
This PR upgrade Scala to **2.12.10**.

Release notes:
- Fix regression in large string interpolations with non-String typed splices
- Revert "Generate shallower ASTs in pattern translation"
- Fix regression in classpath when JARs have 'a.b' entries beside 'a/b'

- Faster compiler: 5–10% faster since 2.12.8
- Improved compatibility with JDK 11, 12, and 13
- Experimental support for build pipelining and outline type checking

More details:
https://github.com/scala/scala/releases/tag/v2.12.10
https://github.com/scala/scala/releases/tag/v2.12.9

Existing tests

Closes apache#25404 from wangyum/SPARK-28683.

Authored-by: Yuming Wang <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants