[SPARK-16581][SPARKR] Make JVM backend calling functions public #14775

shivaram · 2016-08-23T19:49:12Z

What changes were proposed in this pull request?

This change exposes a public API in SparkR to create objects, call methods on the Spark driver JVM

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

Unit tests, CRAN checks

…java-api

shivaram · 2016-08-23T19:50:47Z

cc @felixcheung @olarayej

SparkQA · 2016-08-23T20:23:09Z

Test build #64307 has finished for PR 14775 at commit d267f2f.

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

felixcheung · 2016-08-23T21:20:41Z

R/pkg/R/backend.R

+#'
+#' Create a new Java object in the JVM running the Spark driver.
+#'
+#' @param className name of the class to create


when certain JVM types are created because our SerDe they don't always come back as jobj - should we call this out?

Java fully qualified class name

I tried to qualify this in the comment on what gets returned. The trouble is that we dont have an externally visible documentation of what types will get converted vs. what will not. Also I think this is bound to change with versions (like the SQL decimal change for example).

However this is a very low level API that is only for advanced developers. So I wonder if we should just leave a pointer to the source file ?

I think the behavior could use some explaining and agree that this API is really for advanced or package developers. How about we put a statement or a few words to that effect in the API doc? I think it's ok we don't put a link or describe this in details in the programming guide. Maybe a .md file later?

I added some comments in a Details section. Let me know if it reads ok. I agree that having a md file might be useful in the long run.

felixcheung · 2016-08-23T21:30:30Z

I think the downside of naming them as-is and keeping the signature (... at the end) are that it would be very hard to change or add to the signature later on (say, to add a context/context_id)

How about naming them sparkR.callJMethod or similar for the exported versions to wrap around the internal ones?
this way we don't have to break internal stuff and could just add a new method if/when we want.
we could also add additional parameter checks or translation as needed that might not apply to internal stuff, which could also mitigate risk

felixcheung · 2016-08-23T21:30:58Z

R/pkg/R/backend.R

+#'
+#' Call a Java method in the JVM running the Spark driver.
+#'
+#' @param objId object to invoke the method on. Should be a "jobj" created by newJObject.


if we have a wrapper, it would be better to name this x

Address code review comments

shivaram · 2016-08-27T22:17:37Z

@felixcheung Good point about having a wrapper -- That will make it easier to update the methods going forward. I added a new file jvm.R with the wrapper functions.

SparkQA · 2016-08-27T22:35:39Z

Test build #64539 has finished for PR 14775 at commit 0959208.

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

SparkQA · 2016-08-27T22:45:24Z

Test build #64540 has finished for PR 14775 at commit 448de0c.

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

felixcheung · 2016-08-28T04:56:44Z

R/pkg/DESCRIPTION

@@ -11,7 +11,7 @@ Authors@R: c(person("Shivaram", "Venkataraman", role = c("aut", "cre"),
                    email = "felixcheung@apache.org"),
             person(family = "The Apache Software Foundation", role = c("aut", "cph")))
 URL: http://www.apache.org/ http://spark.apache.org/
-BugReports: https://issues.apache.org/jira/secure/CreateIssueDetails!init.jspa?pid=12315420&components=12325400&issuetype=4
+BugReports: http://issues.apache.org/jira/browse/SPARK


Perhaps that's too long but that opens directly to a form with the component field preset to SparkR - otherwise it seems easy to get lost?
Also BugReports is supposed to be forbug.report(package = "SparkR")

Other than being long that link also doesn't seem to work if you are not logged in (would be good if you can also check this). The other thing we could do is to just link to the wiki page at https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark#ContributingtoSpark-ContributingBugReports -- Do you think that is better ?

ah, I thought I tested it logged out. You are right, let's scratch that then.

I like the idea with the wiki - though should that mention when to check with user@spark, when to email dev@spark and when to open a JIRA?

I updated the wiki - Let me know if it looks better or if you have other suggestions.

felixcheung · 2016-08-28T05:21:21Z

github seems to like to hide stuff.
FYI, this comment #14775 (comment)
and this #14775 (comment)

shivaram · 2016-08-29T17:59:38Z

Thanks @felixcheung - Addressed both the comments. Let me know if this looks good.

SparkQA · 2016-08-29T18:36:11Z

Test build #64585 has finished for PR 14775 at commit d1ec80b.

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

felixcheung · 2016-08-29T19:16:10Z

LGTM. Thanks.

## What changes were proposed in this pull request? This change exposes a public API in SparkR to create objects, call methods on the Spark driver JVM ## How was this patch tested? (Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests) Unit tests, CRAN checks Author: Shivaram Venkataraman <shivaram@cs.berkeley.edu> Closes #14775 from shivaram/sparkr-java-api. (cherry picked from commit 736a791) Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>

shivaram added 5 commits August 22, 2016 12:37

First cut of making JVM functions public

472072a

Improve documentation for JVM methods

d772285

Merge branch 'master' of https://github.com/apache/spark into sparkr-…

5f4b53a

…java-api

Merge branch 'master' of https://github.com/apache/spark into sparkr-…

5f29361

…java-api

Add unit tests for JVM API

d267f2f

felixcheung reviewed Aug 23, 2016
View reviewed changes

shivaram added 2 commits August 27, 2016 15:03

Create wrappers for JVM calls.

0959208

Address code review comments

Update bug report URL and date in DESCRIPTION

448de0c

felixcheung reviewed Aug 28, 2016
View reviewed changes

Address code review comments

d1ec80b

asfgit closed this in 736a791 Aug 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16581][SPARKR] Make JVM backend calling functions public #14775

[SPARK-16581][SPARKR] Make JVM backend calling functions public #14775

shivaram commented Aug 23, 2016

shivaram commented Aug 23, 2016

SparkQA commented Aug 23, 2016

felixcheung Aug 23, 2016

felixcheung Aug 23, 2016

shivaram Aug 27, 2016

felixcheung Aug 28, 2016

shivaram Aug 29, 2016

felixcheung commented Aug 23, 2016 •

edited

felixcheung Aug 23, 2016

shivaram Aug 27, 2016

shivaram commented Aug 27, 2016

SparkQA commented Aug 27, 2016

SparkQA commented Aug 27, 2016

felixcheung Aug 28, 2016

shivaram Aug 28, 2016

felixcheung Aug 28, 2016

shivaram Aug 29, 2016

felixcheung commented Aug 28, 2016

shivaram commented Aug 29, 2016

SparkQA commented Aug 29, 2016

felixcheung commented Aug 29, 2016

[SPARK-16581][SPARKR] Make JVM backend calling functions public #14775

[SPARK-16581][SPARKR] Make JVM backend calling functions public #14775

Conversation

shivaram commented Aug 23, 2016

What changes were proposed in this pull request?

How was this patch tested?

shivaram commented Aug 23, 2016

SparkQA commented Aug 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung commented Aug 23, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivaram commented Aug 27, 2016

SparkQA commented Aug 27, 2016

SparkQA commented Aug 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixcheung commented Aug 28, 2016

shivaram commented Aug 29, 2016

SparkQA commented Aug 29, 2016

felixcheung commented Aug 29, 2016

felixcheung commented Aug 23, 2016 •

edited