[SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton #9192

felixcheung · 2015-10-21T04:43:10Z

Eliminate the need to pass sqlContext to method since it is a singleton - and we don't want to support multiple contexts in a R session.

Changes are done in a back compat way with deprecation warning added. Method signature for S3 methods are added in a concise, clean approach such that in the next release the deprecated signature can be taken out easily/cleanly (just delete a few lines per method).

Custom method dispatch is implemented to allow for multiple JVM reference types that are all 'jobj' in R and to avoid having to add 30 new exports.

shivaram · 2015-10-21T04:47:27Z

cc @davies

davies · 2015-10-21T05:47:20Z

@felixcheung This looks great overall, could you also update all the examples?

felixcheung · 2015-10-21T05:55:30Z

Rebased to master. Updated to fix the new as.DataFrame method.

@davies Thanks! Appreciated. I'd like to leave R doc update separated if that's ok by you - too many files to change and too many possible conflicts. It would be easier as a doc only PR.

SparkQA · 2015-10-21T06:22:37Z

Test build #44047 has finished for PR 9192 at commit b0348d7.

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

SparkQA · 2015-10-21T07:40:08Z

Test build #44042 has finished for PR 9192 at commit d8e91f3.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

sun-rui · 2015-10-21T09:29:53Z

R/pkg/R/SQLContext.R

wording: Temporary -> Dispatching ?

"reroute" was the term corresponding to "dispatch"
"temporary" was referring to the fact that we intend this to go away - please see my other answer regarding your question on this.

SparkQA · 2015-10-21T19:03:34Z

Test build #44080 has finished for PR 9192 at commit 2c16ca8.

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

sun-rui · 2015-10-22T09:11:42Z

R/pkg/R/jobj.R

print.jobj can be updated to use this method.

Could you address this comment?

SparkQA · 2015-10-22T09:53:38Z

Test build #44139 has finished for PR 9192 at commit 2f9e50d.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

felixcheung · 2015-10-31T21:33:30Z

Any more thought on this and #9185 ? It looks like we are cutting the 1.6.0 release very soon, and it will be good if API changes can go into a minor release change

rxin · 2016-05-20T00:45:00Z

Actually I'd love to have this...!

felixcheung · 2016-05-21T09:00:09Z

Would you like API without SQLContext (or SparkSession) parameter or, as what is in this PR, API that can be backward compatible with or without it?

rxin · 2016-05-22T06:23:46Z

I think maintaining backward compatibility would be useful. Do you have time to bring this up to date this week so we get it in for 2.0? It's pretty late for a change of this size, but given this is a huge usability improvement and R is experimental, it might be ok.

…tance in the env

Squashed commits: [2c16ca8] add getClassName from feedback [b0348d7] fix the new as.DataFrame method [d8e91f3] fix test [8b3141a] Change to method dispatch update more tests and add tests for back compat [fd3a835] update tests [fa50f78] Improve route logic [efedce5] Method dispatch to support omission of 'sqlContext' argument [612f7f3] Refractor SQLContext and DataFrame functions to lookup sqlContext instance in the env [9382244] fix test [943889f] Change to method dispatch update more tests and add tests for back compat [c5c41c2] update tests [a8e1ea6] Improve route logic [c779c9d] Method dispatch to support omission of 'sqlContext' argument

shivaram · 2016-05-23T21:33:46Z

Sorry I was busy last week and missed this -- but +1 to keeping backwards compatibility. BTW on that note will this also change the entry point in SparkR to be SparkSession (instead of SQL/SparkContext that is) ?

rxin · 2016-05-23T22:10:57Z

SparkSession definitely makes more sense, given DataFrame is the main API ...

SparkQA · 2016-05-23T23:57:46Z

Test build #59164 has finished for PR 9192 at commit 4884e56.

This patch fails some tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2016-05-23T23:58:56Z

Took me a while to rebase, and caught up with new changes.
This is WIP - I will need to check on roxygen2 doc, and look into SparkSession

SparkQA · 2016-05-25T08:28:52Z

Test build #59269 has finished for PR 9192 at commit 153d5e7.

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

SparkQA · 2016-05-25T09:36:56Z

Test build #59272 has finished for PR 9192 at commit f53b148.

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

shivaram · 2016-05-25T19:02:23Z

R/pkg/R/SQLContext.R


+#' Temporary function to reroute old S3 Method call to new
+#' We need to check the class of x to ensure it is SQLContext before dispatching
+dispatchFunc <- function(newFuncSig, x, ...) {


can we move this to utils.R. Also some function level comments on what the arguments mean would be useful (for example numFuncSig is only used to print the deprecation warning from what i see)

I think this is very specific to this file - this helper is implemented to specifically check & remove sqlContext parameter; I'll add more documentation on this.

shivaram · 2016-05-25T19:12:45Z

Thanks @felixcheung for the update. I left some minor comments inline.

It seems unfortunate that we need to do some amount of code duplication to get this to work (i.e. define read.df and read.df.default etc.) But I think thats fine for two reasons (a) this is an internal code issue and we can continue to clean it up (b) i dont think we are adding a lot of methods there -- in fact we should remove some of the unused ones.

sun-rui · 2016-05-26T02:52:59Z

R/pkg/R/SQLContext.R

-#' new_df <- tableToDF(sqlContext, "table")
+#' new_df <- tableToDF("table")
 #' }
+#' @note since 2.0.0


have we consistently added "since" for all new API methods in 2.0?

not yet, I don't want to keep checking which method is added in 2.0, so I'm adding it in this file for now, that would be the next PR.

sun-rui · 2016-05-26T03:08:23Z

LGTM except some minor comments.

SparkQA · 2016-05-26T03:19:52Z

Test build #59331 has finished for PR 9192 at commit 98e7ab9.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

felixcheung · 2016-05-26T03:22:15Z

@shivaram it's true there's some scaffolding we need to add (though I'm pretty we could codegen them on the fly instead). I think the idea is this is temporary and in the next release (2.1.0?) we could remove these very easily (just a few lines before and after the method, plus renaming x.default back to x) to stop supporting deprecated calls.

SparkQA · 2016-05-26T03:30:08Z

Test build #59335 has finished for PR 9192 at commit 640ffca.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-05-26T03:45:13Z

Test build #59337 has finished for PR 9192 at commit 90641a7.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

shivaram · 2016-05-26T04:11:03Z

Thanks for the update. LGTM. Will merge after Jenkins passes.

SparkQA · 2016-05-26T04:25:28Z

Test build #59341 has finished for PR 9192 at commit f67095e.

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

shivaram · 2016-05-26T18:19:20Z

Merging this to master and branch-2.0

…se a singleton Eliminate the need to pass sqlContext to method since it is a singleton - and we don't want to support multiple contexts in a R session. Changes are done in a back compat way with deprecation warning added. Method signature for S3 methods are added in a concise, clean approach such that in the next release the deprecated signature can be taken out easily/cleanly (just delete a few lines per method). Custom method dispatch is implemented to allow for multiple JVM reference types that are all 'jobj' in R and to avoid having to add 30 new exports. Author: felixcheung <[email protected]> Closes #9192 from felixcheung/rsqlcontext. (cherry picked from commit c76457c) Signed-off-by: Shivaram Venkataraman <[email protected]>

### What changes were proposed in this pull request? Add back the deprecated R APIs removed by #22843 and #22815. These APIs are - `sparkR.init` - `sparkRSQL.init` - `sparkRHive.init` - `registerTempTable` - `createExternalTable` - `dropTempTable` No need to port the function such as ```r createExternalTable <- function(x, ...) { dispatchFunc("createExternalTable(tableName, path = NULL, source = NULL, ...)", x, ...) } ``` because this was for the backward compatibility when SQLContext exists before assuming from #9192, but seems we don't need it anymore since SparkR replaced SQLContext with Spark Session at #13635. ### Why are the changes needed? Amend Spark's Semantic Versioning Policy ### Does this PR introduce any user-facing change? Yes The removed R APIs are put back. ### How was this patch tested? Add back the removed tests Closes #28058 from huaxingao/r. Authored-by: Huaxin Gao <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

### What changes were proposed in this pull request? Add back the deprecated R APIs removed by #22843 and #22815. These APIs are - `sparkR.init` - `sparkRSQL.init` - `sparkRHive.init` - `registerTempTable` - `createExternalTable` - `dropTempTable` No need to port the function such as ```r createExternalTable <- function(x, ...) { dispatchFunc("createExternalTable(tableName, path = NULL, source = NULL, ...)", x, ...) } ``` because this was for the backward compatibility when SQLContext exists before assuming from #9192, but seems we don't need it anymore since SparkR replaced SQLContext with Spark Session at #13635. ### Why are the changes needed? Amend Spark's Semantic Versioning Policy ### Does this PR introduce any user-facing change? Yes The removed R APIs are put back. ### How was this patch tested? Add back the removed tests Closes #28058 from huaxingao/r. Authored-by: Huaxin Gao <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit fd0b228) Signed-off-by: HyukjinKwon <[email protected]>

### What changes were proposed in this pull request? Add back the deprecated R APIs removed by apache#22843 and apache#22815. These APIs are - `sparkR.init` - `sparkRSQL.init` - `sparkRHive.init` - `registerTempTable` - `createExternalTable` - `dropTempTable` No need to port the function such as ```r createExternalTable <- function(x, ...) { dispatchFunc("createExternalTable(tableName, path = NULL, source = NULL, ...)", x, ...) } ``` because this was for the backward compatibility when SQLContext exists before assuming from apache#9192, but seems we don't need it anymore since SparkR replaced SQLContext with Spark Session at apache#13635. ### Why are the changes needed? Amend Spark's Semantic Versioning Policy ### Does this PR introduce any user-facing change? Yes The removed R APIs are put back. ### How was this patch tested? Add back the removed tests Closes apache#28058 from huaxingao/r. Authored-by: Huaxin Gao <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

sun-rui reviewed Oct 21, 2015
View reviewed changes

sun-rui reviewed Oct 22, 2015
View reviewed changes

felixcheung mentioned this pull request Jan 5, 2016

[SPARK-12629] [SparkR] Fixes for DataFrame saveAsTable method #10580

Closed

shivaram mentioned this pull request Apr 27, 2016

[SPARK-7264][ML] Parallel lapply for sparkR #12426

Closed

2 tasks

felixcheung added 3 commits May 22, 2016 17:08

Refractor SQLContext and DataFrame functions to lookup sqlContext ins…

e6edd18

…tance in the env

fix post rebase, add new wrapper for new API

d95288e

felixcheung added 2 commits May 23, 2016 16:33

fix tests

7fd5ed6

suppress warnings for tests

4884e56

felixcheung force-pushed the rsqlcontext branch from 2f9e50d to 4884e56 Compare May 23, 2016 23:56

felixcheung changed the title ~~[SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton~~ [WIP] [SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton May 23, 2016

fix roxygen2 doc gen

d9d72cf

fix hive context test failure more

f53b148

felixcheung changed the title ~~[WIP] [SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton~~ [SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton May 25, 2016

shivaram reviewed May 25, 2016
View reviewed changes

sun-rui reviewed May 26, 2016
View reviewed changes

review feedback

98e7ab9

fix bug

640ffca

left one line by accident

90641a7

more fix to test

f67095e

asfgit closed this in c76457c May 26, 2016

huaxingao mentioned this pull request Mar 30, 2020

[SPARK-31290][R] Add back the deprecated R APIs #28058

Closed

[SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton #9192

[SPARK-10903] [SPARKR] R - Simplify SQLContext method signatures and use a singleton #9192

Uh oh!

Conversation

felixcheung commented Oct 21, 2015

Uh oh!

shivaram commented Oct 21, 2015

Uh oh!

davies commented Oct 21, 2015

Uh oh!

felixcheung commented Oct 21, 2015

Uh oh!

SparkQA commented Oct 21, 2015

Uh oh!

SparkQA commented Oct 21, 2015

Uh oh!

sun-rui Oct 21, 2015

Choose a reason for hiding this comment

Uh oh!

felixcheung Oct 21, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 21, 2015

Uh oh!

sun-rui Oct 22, 2015

Choose a reason for hiding this comment

Uh oh!

sun-rui May 26, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 22, 2015

Uh oh!

felixcheung commented Oct 31, 2015

Uh oh!

rxin commented May 20, 2016

Uh oh!

felixcheung commented May 21, 2016

Uh oh!

rxin commented May 22, 2016

Uh oh!

shivaram commented May 23, 2016

Uh oh!

rxin commented May 23, 2016

Uh oh!

SparkQA commented May 23, 2016

Uh oh!

felixcheung commented May 23, 2016

Uh oh!

SparkQA commented May 25, 2016

Uh oh!

SparkQA commented May 25, 2016

Uh oh!

shivaram May 25, 2016

Choose a reason for hiding this comment

Uh oh!

felixcheung May 26, 2016

Choose a reason for hiding this comment

Uh oh!

shivaram commented May 25, 2016

Uh oh!

sun-rui May 26, 2016

Choose a reason for hiding this comment

Uh oh!

sun-rui May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixcheung May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sun-rui commented May 26, 2016

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

felixcheung commented May 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 26, 2016

Uh oh!

sun-rui May 26, 2016 •

edited

Loading

felixcheung May 26, 2016 •

edited

Loading

felixcheung commented May 26, 2016 •

edited

Loading