[SPARK-28040][SPARK-28070][R] Write type object s3

[MichaelChirico]

Re-filing stale #24888

What changes were proposed in this pull request?

Migrate write* internals to use an S3 generic

Why are the changes needed?

Robustness -- old version was missing some cases like glue that would be treated properly as character by S3 dispatch

Does this PR introduce any user-facing change?

How was this patch tested?

[MichaelChirico]

@HyukjinKwon oh! actually the issue is not transient.

The repository 'https://cloud.r-project.org/bin/linux/ubuntu bionic-cran35/ Release

that file doesn't exist anymore because the 35 folder is no longer the "head" of R -- it's migrated to 40:

https://cloud.r-project.org/bin/linux/ubuntu/bionic-cran40/

[HyukjinKwon]

Thanks for pointing it out. I just opened a PR :-)

[SparkQA]

Test build #121942 has finished for PR 28379 at commit 8e26641.

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

[SparkQA]

Test build #121944 has finished for PR 28379 at commit 27f15ab.

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

[HyukjinKwon]

cc also @felixcheung and @shivaram

[SparkQA]

Test build #121948 has finished for PR 28379 at commit 38aa3e9.

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

[SparkQA]

Test build #121954 has finished for PR 28379 at commit 9a1ac4f.

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

[SparkQA]

Test build #121992 has finished for PR 28379 at commit f1aac7f.

• This patch fails R style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #122110 has finished for PR 28379 at commit 94fe31f.

• This patch fails R style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #122111 has finished for PR 28379 at commit 7fccb3a.

• This patch fails R style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[MichaelChirico]

@HyukjinKwon 7fccb3a failed linting on Jenkins, but not on GH Actions. The error:

inst/worker/worker.R:81:62: style: Opening curly braces should never go on their own line and should always be followed by a new line.
    arrow = SparkR:::writeArrowSerialize(output, outputCon), { # brace here for linter...
                                                             ^
lintr checks failed.
[error] running /home/jenkins/workspace/SparkPullRequestBuilder/dev/lint-r ; received return code 1

Two things:

1. Is the lintr version on Jenkins up to date? I see there was something added to lintr in Sep '19 about this, so if the Jenkins version is <= 1.0.3, that could be the source
2. Is it necessary to have both Jenkins and GitHub running the linting check?

For now, I have nolinted this section; have also filed this issue: r-lib/lintr#487

[SparkQA]

Test build #122112 has finished for PR 28379 at commit f4757da.

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

[SparkQA]

Test build #122115 has finished for PR 28379 at commit 3e3f68a.

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

[SparkQA]

Test build #122121 has finished for PR 28379 at commit 232be9c.

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

[MichaelChirico]

Alright, completely and totally stuck now.

Could go for an extra pair of eyes / advice on how to further debug @HyukjinKwon .

Have done extensive regression testing, see https://gist.github.com/MichaelChirico/58327b30aa865fd8bb234b7b98fa6df9

Two types of test (1) test a bunch of common objects to be sure writeObject is producing the same bits on master and this branch (2) ran pkg/run-tests.sh where I added some code to writeObject to save() the input of all instances of writeObject encountered during the tests, then do the old-v-new comparison on all of those.

Both are successful, so as near as I can tell, the new and old writeObject are always producing the same output.

But still we're failing (it seems a timeout error??)

I'm also not able to reproduce the same errors as Jenkins or Appveyor (which have different errors) locally, so it's hard to know how to proceed.

I'm also not able to run pkg/run-tests.sh on master locally either, FWIW (Spark itself builds under ./build/mvn -DskipTests clean package), further complicating the dev process...

[MichaelChirico]

For the Jenkins lintr, I see now it's 2.0.0 installed from GitHub; this can probably be handled by CRAN now right? 2.0.1 is on CRAN

[MichaelChirico]

New & old writeObject differ for this input:

x = c(1:2, NA)

# old
rc = rawConnection(raw(), 'rb')
writeObject(rc, x)
rawConnectionValue(rc)
# 6169000000030000000100000002
close(rc)

# new
rc = rawConnection(raw(), 'rb')
writeObject(x, rc)
rawConnectionValue(rc)
# 616900000003000000010000000280000000
close(rc)

However, the old code throws a warning in this case, so I doubt that such input was observed previously.

[felixcheung]

I didn’t review but I recall something similar this to support glue like 2 or 3 years ago? This serde is very sensitive - we probably need (to add) a lot more tests on this (existing tests are not very comprehensive)

[MichaelChirico]

Thanks @felixcheung indeed that was me filing that issue about glue 😅

This PR is intended to be a much more general solution to that issue -- a glue object would be passed to writeObject.character under this PR.

There are already a bunch of tests you could basically drop in from the gist above.

My suspicion at the moment is that the usage in worker.R is the issue, but I'm having trouble debugging that. Any suggestion for debugging the executor process?

[SparkQA]

Test build #126420 has finished for PR 28379 at commit ed5f19b.

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

[danzafar]

why not just coerce inputs to class character?

SparkR:::sql.default <- function (sqlQuery) {
    sparkSession <- getSparkSession()
    sdf <- callJMethod(sparkSession, "sql", as.character(sqlQuery))
    dataFrame(sdf)
}

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!