Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Feb 4, 2020

What changes were proposed in this pull request?

This is a follow-up for #22787. In #22787 we disallowed empty strings for json parser except for string and binary types. This follow-up adds a legacy config for restoring previous behavior of allowing empty string.

Why are the changes needed?

Adding a legacy config to make migration easy for Spark users.

Does this PR introduce any user-facing change?

Yes. If set this legacy config to true, the users can restore previous behavior prior to Spark 3.0.0.

How was this patch tested?

Unit test.

@viirya
Copy link
Member Author

viirya commented Feb 4, 2020

cc @cloud-fan @dongjoon-hyun

@viirya
Copy link
Member Author

viirya commented Feb 4, 2020

Thanks @MaxGekk!

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you, @viirya . +1, LGTM (with a few minor comments.)
cc @gatorsmile

@SparkQA
Copy link

SparkQA commented Feb 4, 2020

Test build #117852 has finished for PR 27456 at commit ec01677.

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

@SparkQA
Copy link

SparkQA commented Feb 4, 2020

Test build #117854 has finished for PR 27456 at commit 506c550.

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

@SparkQA
Copy link

SparkQA commented Feb 5, 2020

Test build #117857 has finished for PR 27456 at commit fd62b4b.

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

@dongjoon-hyun
Copy link
Member

Ur, it's interesting.

* creating vignettes ... ERROR
Error: processing vignette 'sparkr-vignettes.Rmd' failed with diagnostics:
package ���htmltools��� was installed by an R version with different internals; it needs to be reinstalled for use with this R version
Execution halted

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 5, 2020

The master branch seems to be okay.

* creating vignettes ... OK
* checking for LF line-endings in source and make files
* checking for empty or unneeded directories
* building ���SparkR_3.0.0.tar.gz���

BTW, the error is irrelevant to this PR.

@HyukjinKwon
Copy link
Member

@dongjoon-hyun, I think Shane upgraded testthat to 2.0.0 and this seems causing this new problem. Let me try to take a quick stab.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 5, 2020

@shaneknapp, can you take a look?

test_includePackage.R:31: error: include inside function
package or namespace load failed for ���plyr���:
 package ���plyr��� was installed by an R version with different internals; it needs to be reinstalled for use with this R version

Seems it's a package installation issue. Looks like plyr has to be re-installed.

Some other tests looks related to environment variables:

test_sparkSQL.R:499: warning: SPARK-17811: can create DataFrame containing NA as date and time
Your system is mis-configured: ���/etc/localtime��� is not a symlink

test_sparkSQL.R:499: warning: SPARK-17811: can create DataFrame containing NA as date and time
It is strongly recommended to set envionment variable TZ to ���America/Los_Angeles��� (or equivalent)

test_sparkSQL.R:1814: error: string operators
unable to find an inherited method for function ���startsWith��� for signature ���"character"���
1: expect_true(startsWith("Hello World", "Hello")) at /home/jenkins/workspace/SparkPullRequestBuilder@2/R/pkg/tests/fulltests/test_sparkSQL.R:1814
2: quasi_label(enquo(object), label)
3: eval_bare(get_expr(quo), get_env(quo))
4: startsWith("Hello World", "Hello")
5: (function (classes, fdef, mtable) 
   {
       methods <- .findInheritedMethods(classes, fdef, mtable)
       if (length(methods) == 1L) 
           return(methods[[1L]])
       else if (length(methods) == 0L) {
           cnames <- paste0("\"", vapply(classes, as.character, ""), "\"", collapse = ", ")
           stop(gettextf("unable to find an inherited method for function %s for signature %s", 
               sQuote(fdef@generic), sQuote(cnames)), domain = NA)
       }
       else stop("Internal error in finding inherited methods; didn't return a unique method", 
           domain = NA)
   })(list("character"), new("nonstandardGenericFunction", .Data = function (x, prefix) 
   {
       standardGeneric("startsWith")
   }, generic = structure("startsWith", package = "SparkR"), package = "SparkR", group = list(), 
       valueClass = character(0), signature = c("x", "prefix"), default = NULL, skeleton = (function (x, 
           prefix) 
       stop("invalid call in method dispatch to 'startsWith' (no default method)", domain = NA))(x, 
           prefix)), <environment>)
6: stop(gettextf("unable to find an inherited method for function %s for signature %s", 
       sQuote(fdef@generic), sQuote(cnames)), domain = NA)

@dongjoon-hyun
Copy link
Member

Thank you so much, @HyukjinKwon !

@dongjoon-hyun
Copy link
Member

It seems that only PRBuilder is affected so far. Then, I'll merge this PR.
Merged to master/3.0. Thank you, @viirya, @HyukjinKwon , and @MaxGekk .

dongjoon-hyun pushed a commit that referenced this pull request Feb 5, 2020
…ings for certain types in json parser

### What changes were proposed in this pull request?

This is a follow-up for #22787. In #22787 we disallowed empty strings for json parser except for string and binary types. This follow-up adds a legacy config for restoring previous behavior of allowing empty string.

### Why are the changes needed?

Adding a legacy config to make migration easy for Spark users.

### Does this PR introduce any user-facing change?

Yes. If set this legacy config to true, the users can restore previous behavior prior to Spark 3.0.0.

### How was this patch tested?

Unit test.

Closes #27456 from viirya/SPARK-25040-followup.

Lead-authored-by: Liang-Chi Hsieh <[email protected]>
Co-authored-by: Liang-Chi Hsieh <[email protected]>
Signed-off-by: Dongjoon Hyun <[email protected]>
(cherry picked from commit 7631275)
Signed-off-by: Dongjoon Hyun <[email protected]>
@HyukjinKwon
Copy link
Member

+1 for the decision. Let me take an action for the tests for now.

@viirya
Copy link
Member Author

viirya commented Feb 5, 2020

Thanks all!

@HyukjinKwon
Copy link
Member

Made a PR at #27460

@viirya viirya deleted the SPARK-25040-followup branch December 27, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants