You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mmolimar <gi...@git.apache.org> on 2018/09/10 15:03:27 UTC
[GitHub] spark pull request #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Ja...
GitHub user mmolimar opened a pull request:
https://github.com/apache/spark/pull/22383
[SPARK-25395][JavaAPI] Removing Optional Spark Java API
## What changes were proposed in this pull request?
Previous Spark versions didn't require Java 8 and an ``Optional`` Spark Java API had to be implemented to support optional values.
Since Spark 2.4 uses Java 8, the ``Optional`` Spark Java API should be removed so that Spark uses the original Java API.
## How was this patch tested?
``OptionalSuite`` class was removed to test Spark Java API ``Optional`` class (this class as well).
Notice that the ``get`` method in the Spark Java API ``Optional`` class throws a ``NullPointerException`` when the value is not set whereas the native Java API ``java.util.Optional`` throws a ``NoSuchElementException``.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mmolimar/spark SPARK-25395
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22383.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22383
----
commit 4daf0ece245a2dc640be217cf4ad481ea430f996
Author: Mario Molina <mm...@...>
Date: 2018-09-10T14:48:13Z
Removing Optional Spark Java API
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by mmolimar <gi...@git.apache.org>.
Github user mmolimar commented on the issue:
https://github.com/apache/spark/pull/22383
I agree @srowen.
What do you think about reusing the current implementation we already have, for example, in the guava lib instead of having that class in Spark?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22383
Yeah, I think we just can't do this unfortunately. It was worth looking into.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4376 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4376/testReport)** for PR 22383 at commit [`6eaa3f1`](https://github.com/apache/spark/commit/6eaa3f1dde5489e836b19494a3a3a4f83d94139c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224866566
--- Diff: project/MimaExcludes.scala ---
@@ -36,6 +36,8 @@ object MimaExcludes {
// Exclude rules for 3.0.x
lazy val v30excludes = v24excludes ++ Seq(
+ // [SPARK-25362][JavaAPI] Replace Spark Optional class with Java Optional
+ ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")
--- End diff --
Sorry @mmolimar this needs a rebase; another change just added a line here and it conflicts. Otherwise looking good
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Replace Spark Optional class with...
Posted by mmolimar <gi...@git.apache.org>.
Github user mmolimar commented on the issue:
https://github.com/apache/spark/pull/22383
Done @srowen
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4376 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4376/testReport)** for PR 22383 at commit [`6eaa3f1`](https://github.com/apache/spark/commit/6eaa3f1dde5489e836b19494a3a3a4f83d94139c).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by mmolimar <gi...@git.apache.org>.
Github user mmolimar commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224948273
--- Diff: project/MimaExcludes.scala ---
@@ -36,6 +36,8 @@ object MimaExcludes {
// Exclude rules for 3.0.x
lazy val v30excludes = v24excludes ++ Seq(
+ // [SPARK-25362][JavaAPI] Replace Spark Optional class with Java Optional
+ ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")
--- End diff --
No worries. Done ;-)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Java API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22383
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25395][JavaAPI] Replace Spark Optional cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224228243
--- Diff: core/src/test/java/test/org/apache/spark/JavaAPISuite.java ---
@@ -476,10 +476,10 @@ public void leftOuterJoin() {
new Tuple2<>(2, 'z'),
new Tuple2<>(4, 'w')
));
- List<Tuple2<Integer,Tuple2<Integer,Optional<Character>>>> joined =
+ List<Tuple2<Integer,Tuple2<Integer, Optional<Character>>>> joined =
--- End diff --
@mmolimar Could you revert these whitespace changes?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224965080
--- Diff: project/MimaExcludes.scala ---
@@ -36,9 +36,11 @@ object MimaExcludes {
// Exclude rules for 3.0.x
lazy val v30excludes = v24excludes ++ Seq(
- ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.io.SnappyCompressionCodec.version"),
+ // [SPARK-25362][JavaAPI] Replace Spark Optional class with Java Optional
+ ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")
ProblemFilters.exclude[IncompatibleMethTypeProblem]("org.apache.spark.api.java.JavaPairRDD.flatMapValues"),
--- End diff --
I realize that I neglected to leave comments in the lines above about the source of these exclusions in earlier changes. While you're here, could you add above this line:
`// [SPARK-19287] JavaPairRDD flatMapValues requires function returning Iterable, not Iterator`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224965113
--- Diff: project/MimaExcludes.scala ---
@@ -36,9 +36,11 @@ object MimaExcludes {
// Exclude rules for 3.0.x
lazy val v30excludes = v24excludes ++ Seq(
- ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.io.SnappyCompressionCodec.version"),
+ // [SPARK-25362][JavaAPI] Replace Spark Optional class with Java Optional
+ ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")
ProblemFilters.exclude[IncompatibleMethTypeProblem]("org.apache.spark.api.java.JavaPairRDD.flatMapValues"),
ProblemFilters.exclude[IncompatibleMethTypeProblem]("org.apache.spark.streaming.api.java.JavaPairDStream.flatMapValues")
+ ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.io.SnappyCompressionCodec.version"),
--- End diff --
And above the line here, add
`// [SPARK-24109] Remove class SnappyOutputStreamWrapper`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4379 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4379/testReport)** for PR 22383 at commit [`f188243`](https://github.com/apache/spark/commit/f1882433071ccb9661a5e41d0ff3358d3ea6d5a3).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Java API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22383
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Java API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22383
We can't do this until we decide whether the next version will be a major version or not. This breaks backwards compatibility. (See the bug I duplicated your bug to.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22383
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Java API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22383
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4368 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4368/testReport)** for PR 22383 at commit [`054c79a`](https://github.com/apache/spark/commit/054c79a9f2940440346bca631bd926a6a4e434b6).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Replace Spark Optional class with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22383
Oops, I mean SPARK-25362. SPARK-25395 was a duplicate.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22383
Spark's class only reason to exist is so we do *not* use the Guava class, since everyone and their pet want to use a different version of Guava (and Spark shades it for that reason).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4379 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4379/testReport)** for PR 22383 at commit [`f188243`](https://github.com/apache/spark/commit/f1882433071ccb9661a5e41d0ff3358d3ea6d5a3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22383: [SPARK-25362][JavaAPI] Replace Spark Optional cla...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22383#discussion_r224964998
--- Diff: project/MimaExcludes.scala ---
@@ -36,6 +36,8 @@ object MimaExcludes {
// Exclude rules for 3.0.x
lazy val v30excludes = v24excludes ++ Seq(
+ // [SPARK-25362][JavaAPI] Replace Spark Optional class with Java Optional
+ ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")
--- End diff --
Ah, this has to have a comma after it, and the line that was moved to the end needs the comma removed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Replace Spark Optional class with...
Posted by mmolimar <gi...@git.apache.org>.
Github user mmolimar commented on the issue:
https://github.com/apache/spark/pull/22383
Updated @srowen
The PR title already contains SPARK-25395, is that what you're expecting or another PR?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22383
**[Test build #4368 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4368/testReport)** for PR 22383 at commit [`054c79a`](https://github.com/apache/spark/commit/054c79a9f2940440346bca631bd926a6a4e434b6).
* This patch **fails MiMa tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22383
Yeah, you'll have to add this to the 3.0 excludes section of project/MimaExcludes:
`ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.api.java.Optional")`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25395][JavaAPI] Removing Optional Spark Java API
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22383
Yeah this is the right kind of change but needs to wait for now. Can you update the title?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by mmolimar <gi...@git.apache.org>.
Github user mmolimar commented on the issue:
https://github.com/apache/spark/pull/22383
No problem. Done ;-)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22383: [SPARK-25362][JavaAPI] Replace Spark Optional class with...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22383
Oh, hm:
```
Serialization stack:
- object not serializable (class: java.util.Optional, value: Optional[x])
- field (class: scala.Tuple2, name: _2, type: class java.lang.Object)
- object (class scala.Tuple2, (1,Optional[x]))
- field (class: scala.Tuple2, name: _2, type: class java.lang.Object)
- object (class scala.Tuple2, (1,(1,Optional[x])))
- element of array (index: 0)
- array (class [Lscala.Tuple2;, size 5)
```
So `java.util.Optional` isn't `Serializable`. Well, that may scuttle this whole idea. I think we're going to find a number of instances where Spark or user apps need to `collect()` `Optional` objects.
Unless someone has a bright idea I think we can't do this. Same reason I was unable to change Spark to use `java.util.function` interfaces -- lambdas aren't otherwise `Serializable` in Java!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org