[SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 #19854

kellyzly · 2017-11-30T08:21:50Z

…a-2.12 and JDK9

What changes were proposed in this pull request?

Some compile error after upgrading to scala-2.12

spark_source/core/src/main/scala/org/apache/spark/executor/Executor.scala:455: ambiguous reference to overloaded definition, method limit in class ByteBuffer of type (x$1: Int)java.nio.ByteBuffer
method limit in class Buffer of type ()Int
match expected type ?
     val resultSize = serializedDirectResult.limit
error

The limit method was moved from ByteBuffer to the superclass Buffer and it can no longer be called without (). The same reason for position method.

/home/zly/prj/oss/jdk9_HOS_SOURCE/spark_source/sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformationExec.scala:427: ambiguous reference to overloaded definition, [error] both method putAll in class Properties of type (x$1: java.util.Map[_, _])Unit [error] and  method putAll in class Hashtable of type (x$1: java.util.Map[_ <: Object, _ <: Object])Unit [error] match argument types (java.util.Map[String,String])
 [error]       props.putAll(outputSerdeProps.toMap.asJava)
 [error]             ^

This is because the key type is Object instead of String which is unsafe.

How was this patch tested?

running tests

Please review http://spark.apache.org/contributing.html before opening a pull request.

srowen

This kind of change is fine as it's backwards-compatible, if it resolves a real compiler issues later. But collect as many of this type of problem in this PR rather than across several.

SparkQA · 2017-11-30T08:28:39Z

Test build #4001 has finished for PR 19854 at commit fb30b90.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-11-30T16:55:32Z

ok to test

SparkQA · 2017-11-30T16:59:21Z

Test build #84344 has finished for PR 19854 at commit d3f0efe.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2017-11-30T17:00:09Z

Let's fix the title to [SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 and JDK9 and fill up the PR description BTW.

HyukjinKwon · 2017-11-30T17:01:26Z

core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala

@@ -99,7 +99,7 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
  private def calcChecksum(block: ByteBuffer): Int = {
    val adler = new Adler32()
    if (block.hasArray) {
-      adler.update(block.array, block.arrayOffset + block.position, block.limit - block.position)
+      adler.update(block.array, block.arrayOffset + block.position(), block.limit() - block.position())


This exceeds 100 character length limit. See the logs in Jenkins tests.

srowen · 2017-11-30T18:00:48Z

common/unsafe/src/main/java/org/apache/spark/unsafe/Platform.java

-      cleanerField.set(buffer, cleaner);
+//    sun.misc.Cleaner is not used in JDK9(see SPARK-22659), we can use CleanUtil in HADOOP-12760 to solve it
+//    Cleaner cleaner = Cleaner.create(buffer, () -> freeMemory(memory));
+//    cleanerField.set(buffer, cleaner);


This change isn't valid and isn't related

Sure I can remove the comment like " sun.misc.Cleaner is not used in JDK9(see SPARK-22659), we can use CleanUtil in HADOOP-12760 to solve it"
sun.misc.Cleaner has moved to a new location in OpenJDK 9. If uncommenting code about Cleaner, compile will fail. Cleaner is used to release memory. If commenting code about cleaner, there maybe some function lost. Can you give me some suggestions?

@srowen ： have deleted changes about Platform.java. So current pr is only for solving problems met in scala-2.12 and can not fix the problem about JDK9.

srowen · 2017-11-30T18:01:21Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformationExec.scala

-    properties.putAll(propsMap.asJava)
+    // properties.putAll(propsMap.asJava)
+    // see https://github.com/apache/kafka/pull/3647
+    propsMap.foreach{ case (k, v) => properties.put(k, v) }


Nit: space before brace

I think the direct reference should be scala/bug#10418.

And we should not leave commented code.

…a-2.12 and JDK9

viirya · 2017-12-01T04:24:44Z

external/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaTestUtils.scala

@@ -296,7 +296,8 @@ class KafkaTestUtils(withBrokerProps: Map[String, Object] = Map.empty) extends L
    props.put("replica.socket.timeout.ms", "1500")
    props.put("delete.topic.enable", "true")
    props.put("offsets.topic.num.partitions", "1")
-    props.putAll(withBrokerProps.asJava)
+    // props.putAll(withBrokerProps.asJava)
+    withBrokerProps.foreach{ case (k, v) => props.put(k, v) }


Is this related?

scala/bug#10418. I see.

yes, the change to the putAll is similar as the solution provided in scala/bug#10418

viirya · 2017-12-01T04:33:54Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformationExec.scala

@@ -424,7 +426,9 @@ case class HiveScriptIOSchema (
    recordReaderClass.map { klass =>
      val instance = Utils.classForName(klass).newInstance().asInstanceOf[RecordReader]
      val props = new Properties()
-      props.putAll(outputSerdeProps.toMap.asJava)
+      // props.putAll(outputSerdeProps.toMap.asJava)
+      // see https://github.com/apache/kafka/pull/3647


ditto. I think it is better to remove commented code and refer to scala/bug#10418.

srowen · 2017-12-04T17:08:07Z

Ping @kellyzly

SparkQA · 2017-12-06T07:16:15Z

Test build #84525 has finished for PR 19854 at commit dbe6b3f.

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

HyukjinKwon · 2017-12-06T08:19:07Z

external/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaTestUtils.scala

@@ -296,7 +296,8 @@ class KafkaTestUtils(withBrokerProps: Map[String, Object] = Map.empty) extends L
    props.put("replica.socket.timeout.ms", "1500")
    props.put("delete.topic.enable", "true")
    props.put("offsets.topic.num.partitions", "1")
-    props.putAll(withBrokerProps.asJava)
+    // props.putAll(withBrokerProps.asJava)
+    withBrokerProps.foreach{ case (k, v) => props.put(k, v) }


Nit: h{ -> h {

SparkQA · 2017-12-06T11:52:25Z

Test build #84540 has finished for PR 19854 at commit 83a09cb.

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

jiangxb1987 · 2017-12-06T12:31:34Z

Please fix the title and also fulfill the PR description.

…12 and jdk9

kellyzly · 2017-12-07T01:55:32Z

thanks @HyukjinKwon ,@srowen, @viirya 's review

srowen · 2017-12-07T16:04:20Z

Merged to master

srowen reviewed Nov 30, 2017

View reviewed changes

HyukjinKwon reviewed Nov 30, 2017

View reviewed changes

srowen reviewed Nov 30, 2017

View reviewed changes

SPARK-22660:Use position() and limit() to fix ambiguity issue in scal…

fb30b90

…a-2.12 and JDK9

viirya reviewed Dec 1, 2017

View reviewed changes

kellyzly changed the title ~~SPARK-22660:Use position() and limit() to fix ambiguity issue in scal…~~ [SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 and JDK9 Dec 6, 2017

kellyzly force-pushed the SPARK-22660 branch from d3f0efe to dbe6b3f Compare December 6, 2017 03:44

kellyzly changed the title ~~[SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 and JDK9~~ [SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 Dec 6, 2017

HyukjinKwon reviewed Dec 6, 2017

View reviewed changes

srowen approved these changes Dec 6, 2017

View reviewed changes

HyukjinKwon approved these changes Dec 6, 2017

View reviewed changes

kellyzly added 3 commits December 6, 2017 21:54

SPARK-22661:Fix the putAll compile error when compiling with scala-2.…

8ede045

…12 and jdk9

fix comments

dbe6b3f

fix comments2

83a09cb

asfgit closed this in f41c0a9 Dec 7, 2017

[SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 #19854

[SPARK-22660][BUILD] Use position() and limit() to fix ambiguity issue in scala-2.12 #19854

Uh oh!

Conversation

kellyzly commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 30, 2017

Uh oh!

HyukjinKwon commented Nov 30, 2017

Uh oh!

SparkQA commented Nov 30, 2017

Uh oh!

HyukjinKwon commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srowen commented Dec 4, 2017

Uh oh!

SparkQA commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 6, 2017

Uh oh!

jiangxb1987 commented Dec 6, 2017

Uh oh!

kellyzly commented Dec 7, 2017

Uh oh!

srowen commented Dec 7, 2017

Uh oh!

Uh oh!

kellyzly commented Nov 30, 2017 •

edited

Loading