-
Notifications
You must be signed in to change notification settings - Fork 28k
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-10683][SPARK-16510][SPARKR] Move SparkR include jar test to SparkSubmitSuite #14243
Conversation
This helps us remove the binary jar from the R package and solves both the CRAN warnings and the lack of source being available for this jar.
cc @felixcheung |
Test build #62441 has finished for PR 14243 at commit
|
Test build #62443 has finished for PR 14243 at commit
|
cc @sun-rui |
@@ -16,17 +16,17 @@ | |||
# | |||
library(SparkR) | |||
|
|||
sparkR.session() | |||
sc <- sparkR.session() |
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.
probably left behind from merge - sc
was not referenced and was removed.
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.
If we remove sc
then it prints a line
Java ref type org.apache.spark.sql.SparkSession id 1
It doesn't matter in a test case but I wonder if we should return something else / print something better for users who might call this.
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.
we could change that in print.jobj
? since we don't require specifying SparkSession in calls, we don't really need to return this from sparkR.session()
?
Looks good, interesting approach - this won't run in CRAN since this is part of Scala tests? |
Will this test be run always no matter if the "sparkr" profile is specified or not? In other words, does R need to installed for all spark tests to pass? |
@sun-rui Thats a good point. Right now the test will be canceled if R is not installed (from the line
@felixcheung Yeah thats the idea. CRAN will only run the R tests and not the Scala ones. |
I guess it is harder to tell if the "R tests" are passing if it's embedded in Scala test, but that's an existing problem we have.. |
Test build #62474 has finished for PR 14243 at commit
|
better to add the SparkR installation check to the existing disable test? My expected solution would be setting an env var when the "sparkr" profile is specifed, and run SparkR related tests when the env var is detected. However, lets go with current way for now. |
Added the check to the |
Test build #62542 has finished for PR 14243 at commit
|
LGTM |
Thanks @sun-rui - Merging this to master and branch-2.0 |
…arkSubmitSuite ## What changes were proposed in this pull request? This change moves the include jar test from R to SparkSubmitSuite and uses a dynamically compiled jar. This helps us remove the binary jar from the R package and solves both the CRAN warnings and the lack of source being available for this jar. ## How was this patch tested? SparkR unit tests, SparkSubmitSuite, check-cran.sh Author: Shivaram Venkataraman <shivaram@cs.berkeley.edu> Closes #14243 from shivaram/sparkr-jar-move. (cherry picked from commit fc23263) Signed-off-by: Shivaram Venkataraman <shivaram@cs.berkeley.edu>
What changes were proposed in this pull request?
This change moves the include jar test from R to SparkSubmitSuite and uses a dynamically compiled jar. This helps us remove the binary jar from the R package and solves both the CRAN warnings and the lack of source being available for this jar.
How was this patch tested?
SparkR unit tests, SparkSubmitSuite, check-cran.sh