Skip to content

[SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR #19551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR #19551

wants to merge 2 commits into from

Conversation

HyukjinKwon
Copy link
Member

What changes were proposed in this pull request?

This PR proposes to revive stringsAsFactors option in collect API, which was mistakenly removed in 71a138c.

Simply, it casts charactor to factor if it meets the condition, stringsAsFactors && is.character(vec) in primitive type conversion.

How was this patch tested?

Unit test in R/pkg/tests/fulltests/test_sparkSQL.R.

@HyukjinKwon
Copy link
Member Author

cc @falaki and @shivaram, I think I am not sure if it is the best way, so, I marked this as [WIP]. But, it looks at least fixing the issue though. Would you guys mind taking a look when you have some time?

@SparkQA
Copy link

SparkQA commented Oct 22, 2017

Test build #82958 has finished for PR 19551 at commit ffe751b.

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

@SparkQA
Copy link

SparkQA commented Oct 22, 2017

Test build #82959 has finished for PR 19551 at commit 145b60f.

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

falaki
falaki suggested changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@falaki falaki left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR.

R/pkg/tests/fulltests/test_sparkSQL.R
@@ -499,6 +499,12 @@ test_that("create DataFrame with different data types", {
expect_equal(collect(df), data.frame(l, stringsAsFactors = FALSE))
})

test_that("SPARK-17902: collect() with stringsAsFactors enabled", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please verify that factor orders are identical. I wonder if expect_equal really verifies order of values in a factor.

Copy link
Member Author

Choose a reason for hiding this comment

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

> # Ordered vs unordered
> or <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Lo", "Med", "Hi"), ordered=TRUE)
> or1 <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Lo", "Med", "Hi"), ordered=FALSE)
> expect_equal(or, or1)
error: `or` not equal to `or1`.
Attributes: < Component “class”: Lengths (2, 1) differ (string compare on first 1) >
Attributes: < Component “class”: 1 string mismatch >
> # level order mismatch
> or <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Hi", "Lo", "Med"))
> or1 <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Lo", "Med", "Hi"))
> expect_equal(or, or1)
error: `or` not equal to `or1`.
Attributes: < Component “levels”: 3 string mismatches >
# Data order mismatch
> or <- factor(c("Lo", "Hi", "Med", "Med", "Hi"), levels=c("Hi", "Lo", "Med"))
> or1 <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Hi", "Lo", "Med"))
> expect_equal(or, or1)
error: `or` not equal to `or1`.
4 string mismatches
> or <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Hi", "Lo", "Med"))
> or1 <- factor(c("Hi", "Med", "Med", "Hi", "Lo"), levels=c("Hi", "Lo", "Med"))
> expect_equal(or, or1)

Would this test address your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW: I think iris data frame all Species values are clustered together. That is why the test is passing (the new factor order ends up being identical to the existing order).

R/pkg/R/DataFrame.R Outdated
@@ -1191,6 +1191,9 @@ setMethod("collect",
vec <- do.call(c, col)
stopifnot(class(vec) != "list")
class(vec) <- PRIMITIVE_TYPES[[colType]]
if (stringsAsFactors && is.character(vec)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For performance maybe it is better to reverse the order of checks: is.character(vec) && stringsAsFactors

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, thanks.

@HyukjinKwon HyukjinKwon changed the title [WIP][SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR [SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR Oct 23, 2017
@SparkQA
Copy link

SparkQA commented Oct 23, 2017

Test build #82982 has finished for PR 19551 at commit f9e42d5.

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

@HyukjinKwon HyukjinKwon reopened this Oct 23, 2017
@falaki
Copy link
Contributor

falaki commented Oct 23, 2017

LGTM

felixcheung
felixcheung approved these changes Oct 24, 2017
View reviewed changes
Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

LGTM, added this to SPARK-21616

asfgit pushed a commit that referenced this pull request Oct 26, 2017
@HyukjinKwon
[SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR
24fe7cc 
## What changes were proposed in this pull request?

This PR proposes to revive `stringsAsFactors` option in collect API, which was mistakenly removed in 71a138c.

Simply, it casts `charactor` to `factor` if it meets the condition, `stringsAsFactors && is.character(vec)` in primitive type conversion.

## How was this patch tested?

Unit test in `R/pkg/tests/fulltests/test_sparkSQL.R`.

Author: hyukjinkwon <gurwls223@gmail.com>

Closes #19551 from HyukjinKwon/SPARK-17902.

(cherry picked from commit a83d8d5)
Signed-off-by: hyukjinkwon <gurwls223@gmail.com>
asfgit pushed a commit that referenced this pull request Oct 26, 2017
@HyukjinKwon
[SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR
aa023fd 
## What changes were proposed in this pull request?

This PR proposes to revive `stringsAsFactors` option in collect API, which was mistakenly removed in 71a138c.

Simply, it casts `charactor` to `factor` if it meets the condition, `stringsAsFactors && is.character(vec)` in primitive type conversion.

## How was this patch tested?

Unit test in `R/pkg/tests/fulltests/test_sparkSQL.R`.

Author: hyukjinkwon <gurwls223@gmail.com>

Closes #19551 from HyukjinKwon/SPARK-17902.

(cherry picked from commit a83d8d5)
Signed-off-by: hyukjinkwon <gurwls223@gmail.com>
@HyukjinKwon
Copy link
Member Author

Merged to master, branch-2.2 and branch-2.1.

@HyukjinKwon
Copy link
Member Author

Thank you for review @falaki and @felixcheung.

@asfgit asfgit closed this in a83d8d5 Oct 26, 2017
@HyukjinKwon HyukjinKwon deleted the SPARK-17902 branch January 2, 2018 03:41
MatthewRBruce pushed a commit to Shopify/spark that referenced this pull request Jul 31, 2018
@HyukjinKwon @MatthewRBruce
[SPARK-17902][R] Revive stringsAsFactors option for collect() in SparkR
b7d5106 
## What changes were proposed in this pull request?

This PR proposes to revive `stringsAsFactors` option in collect API, which was mistakenly removed in apache@71a138c.

Simply, it casts `charactor` to `factor` if it meets the condition, `stringsAsFactors && is.character(vec)` in primitive type conversion.

## How was this patch tested?

Unit test in `R/pkg/tests/fulltests/test_sparkSQL.R`.

Author: hyukjinkwon <gurwls223@gmail.com>

Closes apache#19551 from HyukjinKwon/SPARK-17902.

(cherry picked from commit a83d8d5)
Signed-off-by: hyukjinkwon <gurwls223@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@falaki falaki falaki requested changes

@felixcheung felixcheung felixcheung approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HyukjinKwon @SparkQA @falaki @felixcheung