Skip to content
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-4795][Core] Redesign the "primitive type => Writable" implicit APIs to make them be activated automatically #3642

Closed
wants to merge 5 commits into from

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Dec 9, 2014

Try to redesign the "primitive type => Writable" implicit APIs to make them be activated automatically and without breaking binary compatibility.

However, this PR will breaking the source compatibility if people use xxxToXxxWritable occasionally. See the unit test in graphx.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24243 has started for PR 3642 at commit ac1e903.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24244 has started for PR 3642 at commit 3009328.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24243 has finished for PR 3642 at commit ac1e903.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • m => m.getReturnType().toString != "class java.lang.Object" &&

@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/24243/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24244 has finished for PR 3642 at commit 3009328.

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

@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/24244/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24245 has started for PR 3642 at commit 39343de.

  • This patch merges cleanly.

@@ -40,7 +40,7 @@ class ShortestPathsSuite extends FunSuite with LocalSparkContext {
val graph = Graph.fromEdgeTuples(edges, 1)
val landmarks = Seq(1, 4).map(_.toLong)
val results = ShortestPaths.run(graph, landmarks).vertices.collect.map {
case (v, spMap) => (v, spMap.mapValues(_.get))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an example of breaking source compatibility, although it's used the implicit intToIntWritable occasionally. _.get => intToIntWritable(_).get.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or we can keep xxxToXxxWritable still implicit for the source compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think you can keep that and have the new one because the compiler will complain

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have any example? I added implicit to them and compiled codes successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

If they are not ambiguous, I'd add the implicits back to make sure we never break.

@ankurdave - why is there a get in this test case? Is the get just redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look like the get is redundant, since Int should be sufficient for this purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

If they are not ambiguous, I'd add the implicits back to make sure we never break.

I added them back.

@SparkQA
Copy link

SparkQA commented Dec 9, 2014

Test build #24245 has finished for PR 3642 at commit 39343de.

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

@AmplabJenkins
Copy link

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

@rxin
Copy link
Contributor

rxin commented Feb 3, 2015

@zsxwing if we merge this one, are there any other usecases for importing SparkContext._ ?

Conflicts:
	core/src/main/scala/org/apache/spark/SparkContext.scala
@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26636 has started for PR 3642 at commit 0b9017f.

  • This patch merges cleanly.

@zsxwing
Copy link
Member Author

zsxwing commented Feb 3, 2015

@zsxwing if we merge this one, are there any other usecases for importing SparkContext._ ?

There will be no implicit methods/objects in SparkContext object. So people don't need to import SparkContext._ for implicit methods/objects.

Since the compatibility is very important, it's better to get more pairs of eyes to look at it.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26636 has finished for PR 3642 at commit 0b9017f.

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

@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/26636/
Test FAILed.

@zsxwing
Copy link
Member Author

zsxwing commented Feb 3, 2015

Jenkins, retest this please.

1 similar comment
@rxin
Copy link
Contributor

rxin commented Feb 3, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26647 has started for PR 3642 at commit 0b9017f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26647 has finished for PR 3642 at commit 0b9017f.

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

@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/26647/
Test FAILed.

@zsxwing
Copy link
Member Author

zsxwing commented Feb 3, 2015

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26651 has started for PR 3642 at commit 0b9017f.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 3, 2015

Test build #26651 has finished for PR 3642 at commit 0b9017f.

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

@AmplabJenkins
Copy link

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

@SparkQA
Copy link

SparkQA commented Feb 4, 2015

Test build #26704 has started for PR 3642 at commit 914b2d6.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Feb 4, 2015

Test build #26704 has finished for PR 3642 at commit 914b2d6.

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

@AmplabJenkins
Copy link

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

@rxin
Copy link
Contributor

rxin commented Feb 4, 2015

Ok I'm going to merge this. Thanks for working on it.

@asfgit asfgit closed this in d37978d Feb 4, 2015
asfgit pushed a commit that referenced this pull request Feb 4, 2015
… APIs to make them be activated automatically

Try to redesign the "primitive type => Writable" implicit APIs to make them be activated automatically and without breaking binary compatibility.

However, this PR will breaking the source compatibility if people use `xxxToXxxWritable` occasionally. See the unit test in `graphx`.

Author: zsxwing <zsxwing@gmail.com>

Closes #3642 from zsxwing/SPARK-4795 and squashes the following commits:

914b2d6 [zsxwing] Add implicit back to the Writables methods
0b9017f [zsxwing] Add some docs
a0e8509 [zsxwing] Merge branch 'master' into SPARK-4795
39343de [zsxwing] Fix the unit test
64853af [zsxwing] Reorganize the rest 'implicit' methods in SparkContext

(cherry picked from commit d37978d)
Signed-off-by: Reynold Xin <rxin@databricks.com>
@zsxwing zsxwing deleted the SPARK-4795 branch February 4, 2015 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants