[SPARK-29777][SparkR] SparkR::cleanClosure aggressively removes a function required by user function

[falaki]

What changes were proposed in this pull request?

The implementation for walking through the user function AST and picking referenced variables and functions, had an optimization to skip a branch if it had already seen it. This runs into an interesting problem in the following example

df <- createDataFrame(data.frame(x=1))
f1 <- function(x) x + 1
f2 <- function(x) f1(x) + 2
dapplyCollect(df, function(x) { f1(x); f2(x) })

Results in error:

org.apache.spark.SparkException: R computation failed with
Error in f1(x) : could not find function "f1"
Calls: compute -> computeFunc -> f2

Why are the changes needed?

Bug fix

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests in test_utils.R

[falaki]

@mengxr can we cherry-pick this to 2.4.x?

[SparkQA]

Test build #113409 has finished for PR 26429 at commit 51481fd.

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

[mengxr]

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

[HyukjinKwon]

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

[SparkQA]

Test build #113493 has finished for PR 26429 at commit 10925bf.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

Retest this please.

[SparkQA]

Test build #113519 has finished for PR 26429 at commit 10925bf.

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

[falaki]

@shivaram and @felixcheung do you guys have any input on this?

[falaki]

@HyukjinKwon would you please take a look?

[HyukjinKwon]

retest this please

[HyukjinKwon]

LGTM. one question and one nit. Wait, I take it back. Let me take another look.

[SparkQA]

Test build #113738 has finished for PR 26429 at commit 10925bf.

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

[HyukjinKwon]

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 ...

[HyukjinKwon]

Looks fine except two nits: #26429 (comment) and #26429 (comment)

[falaki]

Thanks for reviewing @HyukjinKwon I addressed your comments.

[SparkQA]

Test build #113814 has finished for PR 26429 at commit 6ad35c9.

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

[HyukjinKwon]

retest this please

[SparkQA]

Test build #113827 has finished for PR 26429 at commit 6ad35c9.

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

[HyukjinKwon]

The test cases should be fixed as of 65a189c

[srowen]

Just needs a rebase?

[felixcheung]

Looks reasonable to me

…

________________________________

[falaki]

Rebased this. Sorry for the delay.

[SparkQA]

Test build #114031 has finished for PR 26429 at commit cc9c416.

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

[HyukjinKwon]

Let's give a try.
Merged to master.