From 701980eebfeedc6fa629e939e7b1ae87c1023346 Mon Sep 17 00:00:00 2001 From: Hossein Date: Thu, 7 Nov 2019 14:47:15 -0800 Subject: [PATCH 1/4] Fixed typo --- core/src/main/scala/org/apache/spark/api/r/RRunner.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/api/r/RRunner.scala b/core/src/main/scala/org/apache/spark/api/r/RRunner.scala index 0327386b45ed..291fe7207983 100644 --- a/core/src/main/scala/org/apache/spark/api/r/RRunner.scala +++ b/core/src/main/scala/org/apache/spark/api/r/RRunner.scala @@ -127,7 +127,7 @@ private[spark] class RRunner[IN, OUT]( } } catch { case eof: EOFException => - throw new SparkException("R worker exited unexpectedly (cranshed)", eof) + throw new SparkException("R worker exited unexpectedly", eof) } } } From 51481fda45ac3907cc2823a7e880888b4480851a Mon Sep 17 00:00:00 2001 From: Hossein Date: Thu, 7 Nov 2019 14:48:32 -0800 Subject: [PATCH 2/4] Not skipping closure walkthrough for functions already visited --- R/pkg/R/utils.R | 4 ---- R/pkg/tests/fulltests/test_utils.R | 9 +++++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/R/pkg/R/utils.R b/R/pkg/R/utils.R index c3501977e64b..f3abf351d94a 100644 --- a/R/pkg/R/utils.R +++ b/R/pkg/R/utils.R @@ -545,10 +545,6 @@ processClosure <- function(node, oldEnv, defVars, checkedFuncs, newEnv) { found <- sapply(funcList, function(func) { ifelse(identical(func, obj), TRUE, FALSE) }) - if (sum(found) > 0) { - # If function has been examined, ignore. - break - } # Function has not been examined, record it and recursively clean its closure. assign(nodeChar, if (is.null(funcList[[1]])) { diff --git a/R/pkg/tests/fulltests/test_utils.R b/R/pkg/tests/fulltests/test_utils.R index b2b6f34aaa08..2217e6ae98a0 100644 --- a/R/pkg/tests/fulltests/test_utils.R +++ b/R/pkg/tests/fulltests/test_utils.R @@ -110,6 +110,15 @@ test_that("cleanClosure on R functions", { actual <- get("y", envir = env, inherits = FALSE) expect_equal(actual, y) + # Test for combination for nested and sequenctial functions in a closure + f1 <- function(x) x + 1 + f2 <- function(x) f1(x) + 2 + user_func <- function(x) { f1(x); f2(x) } + c_user_func_env <- environment(cleanClosure(user_func)) + expect_equal(length(c_user_func_env), 2) + inner_c_user_func_env <- environment(c_user_func_env$f2) + expect_equal(length(inner_c_user_func_env), 1) + # Test for function (and variable) definitions. f <- function(x) { g <- function(y) { y * 2 } From 10925bfb1537a86d4773e2788739037a164d6ed3 Mon Sep 17 00:00:00 2001 From: Hossein Date: Fri, 8 Nov 2019 23:22:02 -0800 Subject: [PATCH 3/4] checking for parent environment of checked functions --- R/pkg/R/utils.R | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/R/pkg/R/utils.R b/R/pkg/R/utils.R index f3abf351d94a..3306fc716bac 100644 --- a/R/pkg/R/utils.R +++ b/R/pkg/R/utils.R @@ -545,6 +545,13 @@ processClosure <- function(node, oldEnv, defVars, checkedFuncs, newEnv) { found <- sapply(funcList, function(func) { ifelse(identical(func, obj), TRUE, FALSE) }) + if (sum(found) > 0) { + # If function has been examined + if (identical(parent.env(environment(funcList[found][[1]])), func.env)) { + # If the parent environment is identical to current parent + break + } + } # Function has not been examined, record it and recursively clean its closure. assign(nodeChar, if (is.null(funcList[[1]])) { From 6ad35c93f4b66c63a7e2d173c2c695ee50665b83 Mon Sep 17 00:00:00 2001 From: Hossein Date: Thu, 14 Nov 2019 14:25:49 -0800 Subject: [PATCH 4/4] Addressed comments --- R/pkg/R/utils.R | 13 +++++++------ R/pkg/tests/fulltests/test_utils.R | 10 +++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/R/pkg/R/utils.R b/R/pkg/R/utils.R index 3306fc716bac..e495d230507f 100644 --- a/R/pkg/R/utils.R +++ b/R/pkg/R/utils.R @@ -543,14 +543,15 @@ processClosure <- function(node, oldEnv, defVars, checkedFuncs, newEnv) { funcList <- mget(nodeChar, envir = checkedFuncs, inherits = F, ifnotfound = list(list(NULL)))[[1]] found <- sapply(funcList, function(func) { - ifelse(identical(func, obj), TRUE, FALSE) + ifelse( + identical(func, obj) && + # Also check if the parent environment is identical to current parent + identical(parent.env(environment(func)), func.env), + TRUE, FALSE) }) if (sum(found) > 0) { - # If function has been examined - if (identical(parent.env(environment(funcList[found][[1]])), func.env)) { - # If the parent environment is identical to current parent - break - } + # If function has been examined ignore + break } # Function has not been examined, record it and recursively clean its closure. assign(nodeChar, diff --git a/R/pkg/tests/fulltests/test_utils.R b/R/pkg/tests/fulltests/test_utils.R index 2217e6ae98a0..c4fcbecee18e 100644 --- a/R/pkg/tests/fulltests/test_utils.R +++ b/R/pkg/tests/fulltests/test_utils.R @@ -113,11 +113,11 @@ test_that("cleanClosure on R functions", { # Test for combination for nested and sequenctial functions in a closure f1 <- function(x) x + 1 f2 <- function(x) f1(x) + 2 - user_func <- function(x) { f1(x); f2(x) } - c_user_func_env <- environment(cleanClosure(user_func)) - expect_equal(length(c_user_func_env), 2) - inner_c_user_func_env <- environment(c_user_func_env$f2) - expect_equal(length(inner_c_user_func_env), 1) + userFunc <- function(x) { f1(x); f2(x) } + cUserFuncEnv <- environment(cleanClosure(userFunc)) + expect_equal(length(cUserFuncEnv), 2) + innerCUserFuncEnv <- environment(cUserFuncEnv$f2) + expect_equal(length(innerCUserFuncEnv), 1) # Test for function (and variable) definitions. f <- function(x) {