-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-12115] [SparkR] Change numPartitions() to getNumPartitions() to be consistent with Scala/Python #10123
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
Conversation
|
This is actually not exported from SparkR - since it is first integrated in Spark 1.4, SparkR is exporting a smaller/different set of API. While it is possible to access this with Spark:::numPartitions(), it has been available since Spark 1.4 so this rename is actually going to be a breaking change (of an internal API). |
|
Test build #47121 has finished for PR 10123 at commit
|
|
Yeah, it's unfortunately because until a change 3 days ago, we had 3 different methods in 4 languages for this simple function. Now everything but R uses |
|
@felixcheung Thanks for your comments. I got it's not an exposed API, but I think to provide consistent function name is necessary especially when we want to expose RDD API someday. I think another solution is to add |
|
@srowen I think we have the consistent idea that to provide both |
|
+1 @yanboliang |
|
Test build #47135 has finished for PR 10123 at commit
|
|
Test build #47137 has finished for PR 10123 at commit
|
|
Yeah this looks fine. Since it wasn't a publicly exposed API I don't think the backwards compatibility matters that much -- but for right now I'm +1 on just deprecating |
R/pkg/R/RDD.R
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a note that this is deprecated here ?
Also this is the title of the document, so its better to have this be something like Gets the number of partitions of an RDD cc @felixcheung
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, although we are not generating doc for internal API via @noRd below...
Still, better to be descriptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, if we are deprecating, consider adding
.Deprecated(newFuncSig, old = oldFuncSig)
call to the deprecated version
|
Test build #47179 has finished for PR 10123 at commit
|
|
LGTM. Merging this. Thanks @yanboliang |
… be consistent with Scala/Python Change ```numPartitions()``` to ```getNumPartitions()``` to be consistent with Scala/Python. <del>Note: If we can not catch up with 1.6 release, it will be breaking change for 1.7 that we also need to explain in release note.<del> cc sun-rui felixcheung shivaram Author: Yanbo Liang <[email protected]> Closes #10123 from yanboliang/spark-12115. (cherry picked from commit 6979edf) Signed-off-by: Shivaram Venkataraman <[email protected]>
Change
numPartitions()togetNumPartitions()to be consistent with Scala/Python.Note: If we can not catch up with 1.6 release, it will be breaking change for 1.7 that we also need to explain in release note.cc @sun-rui @felixcheung @shivaram