Skip to content
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

Jump to bottom

[SPARK-22281][SPARKR] Handle R method breaking signature changes #19557

Closed
wants to merge 4 commits into from
Closed

[SPARK-22281][SPARKR] Handle R method breaking signature changes #19557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36af4c1
fix attach and glm
felixcheung Oct 23, 2017
cc51aca
no doc for attach generic
felixcheung Oct 24, 2017
f3dc1de
new signature
felixcheung Oct 24, 2017
1b41f73
fix help
felixcheung Oct 25, 2017
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
11 changes: 7 additions & 4 deletions R/pkg/R/DataFrame.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3236,7 +3236,7 @@ setMethod("as.data.frame",
#'
#' @family SparkDataFrame functions
#' @rdname attach
#' @aliases attach,SparkDataFrame-method
#' @aliases attach attach,SparkDataFrame-method
#' @param what (SparkDataFrame) The SparkDataFrame to attach
#' @param pos (integer) Specify position in search() where to attach.
#' @param name (character) Name to use for the attached SparkDataFrame. Names
Expand All @@ -3252,9 +3252,12 @@ setMethod("as.data.frame",
#' @note attach since 1.6.0
setMethod("attach",
signature(what = "SparkDataFrame"),
function(what, pos = 2, name = deparse(substitute(what)), warn.conflicts = TRUE) {
newEnv <- assignNewEnv(what)
attach(newEnv, pos = pos, name = name, warn.conflicts = warn.conflicts)
function(what, pos = 2L, name = deparse(substitute(what), backtick = FALSE),
warn.conflicts = TRUE) {
Copy link
Member Author

Choose a reason for hiding this comment

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

changing this to match the latest R-devel param signature

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is the only thing we might need to update if R changes something ? Also how does this keep working with the older R version as well

Copy link
Member Author

@felixcheung felixcheung Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this will need to be updated manually...
from my testing on r-release it's working against the older signature. I think for generic to work it just need to match the param names and order but not default value.

this does change the behavior a bit, from name = deparse(substitute(what)) to name = deparse(substitute(what), backtick = FALSE)

args <- as.list(environment()) # capture all parameters - this must be the first line
newEnv <- assignNewEnv(args$what)
args$what <- newEnv
do.call(attach, args)
Copy link
Member Author

Choose a reason for hiding this comment

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

calling attach this way to avoid it getting flagged...

})

#' Evaluate a R expression in an environment constructed from a SparkDataFrame
Expand Down
10 changes: 4 additions & 6 deletions R/pkg/R/generics.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,8 @@ setGeneric("as.data.frame",
standardGeneric("as.data.frame")
})

#' @rdname attach
# Do not document the generic because of signature changes across R versions
#' @noRd
#' @export
setGeneric("attach")

Expand Down Expand Up @@ -1569,12 +1570,9 @@ setGeneric("year", function(x) { standardGeneric("year") })
#' @export
setGeneric("fitted")

#' @param x,y For \code{glm}: logical values indicating whether the response vector
#' and model matrix used in the fitting process should be returned as
#' components of the returned value.
#' @inheritParams stats::glm
#' @rdname glm
# Do not carry stats::glm usage and param here, and do not document the generic
#' @export
#' @noRd
setGeneric("glm")

#' @param object a fitted ML model object.
Expand Down
1 change: 1 addition & 0 deletions R/pkg/R/mllib_regression.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ setMethod("spark.glm", signature(data = "SparkDataFrame", formula = "formula"),
#' 1.0.
#' @return \code{glm} returns a fitted generalized linear model.
#' @rdname glm
#' @aliases glm
#' @export
#' @examples
#' \dontrun{
Expand Down
6 changes: 3 additions & 3 deletions R/run-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ if [[ $FAILED != 0 || $NUM_TEST_WARNING != 0 ]]; then
echo -en "\033[0m" # No color
exit -1
else
# We have 2 NOTEs for RoxygenNote, attach(); and one in Jenkins only "No repository set"
# We have 2 NOTEs: for RoxygenNote and one in Jenkins only "No repository set"
# For non-latest version branches, one WARNING for package version
if [[ ($NUM_CRAN_WARNING != 0 || $NUM_CRAN_ERROR != 0 || $NUM_CRAN_NOTES -gt 3) &&
($HAS_PACKAGE_VERSION_WARN != 1 || $NUM_CRAN_WARNING != 1 || $NUM_CRAN_ERROR != 0 || $NUM_CRAN_NOTES -gt 2) ]]; then
if [[ ($NUM_CRAN_WARNING != 0 || $NUM_CRAN_ERROR != 0 || $NUM_CRAN_NOTES -gt 2) &&
($HAS_PACKAGE_VERSION_WARN != 1 || $NUM_CRAN_WARNING != 1 || $NUM_CRAN_ERROR != 0 || $NUM_CRAN_NOTES -gt 1) ]]; then
cat $CRAN_CHECK_LOG_FILE
echo -en "\033[31m" # Red
echo "Had CRAN check errors; see logs."
Expand Down