Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
7 changes: 5 additions & 2 deletions R/pkg/R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -546,8 +546,11 @@ processClosure <- function(node, oldEnv, defVars, checkedFuncs, newEnv) {
ifelse(identical(func, obj), TRUE, FALSE)
})
if (sum(found) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline. The change would lead to dead loop. We should make sure newEnv contains the (cleaned) node.

Copy link
Member

Choose a reason for hiding this comment

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

Is the dead loop case when the same function with the same environments is recursively called in the closure?

# If function has been examined, ignore.
break
# 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,
Expand Down
9 changes: 9 additions & 0 deletions R/pkg/tests/fulltests/test_utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

BTW, I think naming c_user_func_env is not preferred (it was discussed here #17590 (comment)) before.
I think it should rather be cUserFuncEnv.

Google guide seems updated rapidly (https://google.github.io/styleguide/Rguide.html).. We will have to update the guide ...

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 }
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/api/r/RRunner.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Expand Down