Skip to content

[SPARK-4397][Core] Reorganize 'implicit's to improve the API convenience #3262

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.

Sign up for GitHub

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

Closed
zsxwing wants to merge 11 commits into from
Closed

[SPARK-4397][Core] Reorganize 'implicit's to improve the API convenience #3262

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1eda9e4
Reorganize 'implicit's to improve the API convenience
zsxwing Nov 14, 2014
3ac4f07
Add license header
zsxwing Nov 14, 2014
9b73188
Fix the code style issue
zsxwing Nov 14, 2014
3bdcae2
Move WritableConverter implicits to object WritableConverter
zsxwing Nov 14, 2014
185c12f
Remove simpleWritableConverter from SparkContext
zsxwing Nov 15, 2014
7266218
Add comments to warn the duplicate codes in SparkContext
zsxwing Nov 15, 2014
34641d4
Move ImplicitSuite to org.apache.sparktest
zsxwing Nov 16, 2014
52353de
Remove private[spark] from object WritableConverter
zsxwing Nov 17, 2014
2b5f5a4
Comments for the deprecated functions
zsxwing Nov 17, 2014
9c27aff
Move implicit functions to object RDD and forward old functions to ne…
zsxwing Nov 17, 2014
fc30314
Update the comments
zsxwing Nov 21, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove simpleWritableConverter from SparkContext
  • Loading branch information
@zsxwing
zsxwing committed Nov 15, 2014
commit 185c12f1bd372d6a0d164e373c958b826ec0d22f
12 changes: 3 additions & 9 deletions core/src/main/scala/org/apache/spark/SparkContext.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import org.apache.spark.storage._
import org.apache.spark.ui.SparkUI
import org.apache.spark.ui.jobs.JobProgressListener
import org.apache.spark.util._
import org.apache.spark.WritableConverter.simpleWritableConverter

/**
* Main entry point for Spark functionality. A SparkContext represents the connection to a Spark
Expand Down Expand Up @@ -1503,13 +1504,6 @@ object SparkContext extends Logging {
arr.map(x => anyToWritable(x)).toArray)
}

// Helper objects for converting common types to Writable
private def simpleWritableConverter[T, W <: Writable: ClassTag](convert: W => T)
: WritableConverter[T] = {
val wClass = classTag[W].runtimeClass.asInstanceOf[Class[W]]
new WritableConverter[T](_ => wClass, x => convert(x.asInstanceOf[W]))
}

@deprecated("An API for backforward compatibility", "1.2.0")
Copy link
Contributor

@rxin rxin Nov 15, 2014

Choose a reason for hiding this comment

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

is the simpleWritableConverter in SparkContext still necessary?

def intWritableConverter(): WritableConverter[Int] =
simpleWritableConverter[Int, IntWritable](_.get)
Expand Down Expand Up @@ -1769,10 +1763,10 @@ private[spark] class WritableConverter[T](
val convert: Writable => T)
extends Serializable

object WritableConverter {
private[spark] object WritableConverter {
Copy link
Member Author

@zsxwing zsxwing Nov 15, 2014

Choose a reason for hiding this comment

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

Should this be private[spark] or public? Now class WritableConverter is private[spark], so I set object WritableConverter to private[spark] too.

Copy link
Contributor

@mateiz mateiz Nov 16, 2014

Choose a reason for hiding this comment

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

You should add some tests or examples that are outside the Spark package, but try to use it. My guess is that it needs to be public.

Copy link
Contributor

@mateiz mateiz Nov 16, 2014

Choose a reason for hiding this comment

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

Actually that would be great to add for the whole patch; write some test programs in a different package (e.g. org.apache.sparktest) and make sure all the conversions can happen.

Copy link
Member Author

@zsxwing zsxwing Nov 16, 2014

Choose a reason for hiding this comment

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

My guess is that it needs to be public.

If it's private[spark], people are still able to use it implicitly. But they cannot import these methods or call them directly. In other words, my question is should we enable people to import them or call them directly.

Copy link
Member Author

@zsxwing zsxwing Nov 16, 2014

Choose a reason for hiding this comment

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

write some test programs in a different package (e.g. org.apache.sparktest) and make sure all the conversions can happen.

Moved ImplicitSuite to org.apache.sparktest.

Copy link
Contributor

@mateiz mateiz Nov 17, 2014

Choose a reason for hiding this comment

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

Hmm, I find this pretty weird, so IMO we should make them public. I wouldn't be surprised if this gets removed in some version of Scala otherwise.


// Helper objects for converting common types to Writable
private def simpleWritableConverter[T, W <: Writable: ClassTag](convert: W => T)
private[spark] def simpleWritableConverter[T, W <: Writable: ClassTag](convert: W => T)
: WritableConverter[T] = {
val wClass = classTag[W].runtimeClass.asInstanceOf[Class[W]]
new WritableConverter[T](_ => wClass, x => convert(x.asInstanceOf[W]))
Expand Down
2 changes: 1 addition & 1 deletion core/src/test/scala/org/apache/spark/SparkContextSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class SparkContextSuite extends FunSuite {
bytesWritable.set(inputArray, 0, 10)
bytesWritable.set(inputArray, 0, 5)

val converter = SparkContext.bytesWritableConverter()
val converter = WritableConverter.bytesWritableConverter()
val byteArray = converter.convert(bytesWritable)
assert(byteArray.length === 5)

Expand Down