Skip to content

[SPARK-16883][SparkR]:SQL decimal type is not properly cast to number when collecting SparkDataFrame#14613

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

Closed
wangmiao1981 wants to merge 6 commits into from
Closed

[SPARK-16883][SparkR]:SQL decimal type is not properly cast to number when collecting SparkDataFrame #14613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f55569
add a type check helper
wangmiao1981 Aug 11, 2016
4f88f47
update unit tests
wangmiao1981 Aug 12, 2016
6d19711
move helper function to types.R and add comments
wangmiao1981 Aug 15, 2016
63b9d06
address review comments, adjust return value, change tests
wangmiao1981 Aug 23, 2016
4d6d048
address review comments
wangmiao1981 Sep 1, 2016
4e9e403
remove switch
wangmiao1981 Sep 2, 2016
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
13 changes: 12 additions & 1 deletion R/pkg/R/DataFrame.R
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -397,7 +397,11 @@ setMethod("coltypes",
}

if (is.null(type)) {
stop(paste("Unsupported data type: ", x))
specialtype <- specialtypeshandle(x)

@felixcheungfelixcheungAug 17, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here it looks like we are expecting type to be R native type, but seems like specialtypeshandle is returning double which is not a R native type? (per se - for historical reasons)

@felixcheungfelixcheungAug 17, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

or rather, numeric is better since it matches the mapping in PRIMITIVE_TYPES?

@wangmiao1981wangmiao1981Aug 17, 2016•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

numeric is not the key of PRIMITIVE_TYPE map. is.null(PRIMITIVE_TYPES[[colType]]) is NULL if mapped to numeric type.

@felixcheungfelixcheungAug 18, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

as line 383
type <- PRIMITIVE_TYPES[[x]]
here in line 399
specialtype <- specialtypeshandle(x)
type <- specialtype

numeric is one of the value from PRIMITIVE_TYPE, double, returned by specialtypeshandle, is not

@felixcheungfelixcheungAug 18, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and then from my comment below, line 1059
colType <- dtypes[[colIndex]][[2]]
colType is from dtypes which is the JVM type.
specialtype <- specialtypeshandle(colType)
so seem like we are mixing in both cases.

if (is.null(specialtype)) {
stop(paste("Unsupported data type: ", x))
}
type <- PRIMITIVE_TYPES[[specialtype]]
}
}
type
Expand DownExpand Up@@ -1063,6 +1067,13 @@ setMethod("collect",
df[[colIndex]] <- col
} else {
colType <- dtypes[[colIndex]][[2]]
if (is.null(PRIMITIVE_TYPES[[colType]])) {
specialtype <- specialtypeshandle(colType)
if (!is.null(specialtype)) {
colType <- specialtype

@felixcheungfelixcheungAug 17, 2016•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in this case colType is supposed to be JVM type, so we are mixing in what we are using the function for?

@wangmiao1981wangmiao1981Aug 19, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@felixcheung You are right. Here is a mistake. The goal of this function is to change the Key for the dictionary PRIMITIVE_TYPES. The issue is because backend returns decimal(10,0) as the Key. Let me review the whole PR again and make changes as you suggested. Thanks!

}
}

# Note that "binary" columns behave like complex types.
if (!is.null(PRIMITIVE_TYPES[[colType]]) && colType != "binary") {
vec <- do.call(c, col)
Expand Down
16 changes: 16 additions & 0 deletions R/pkg/R/types.R
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -67,3 +67,19 @@ rToSQLTypes <- as.environment(list(
"double" = "double",
"character" = "string",
"logical" = "boolean"))

# Helper function of coverting decimal type. When backend returns column type in the
# format of decimal(,) (e.g., decimal(10, 0)), this function coverts the column type
# as double type. This function converts backend returned types that are not the key
# of PRIMITIVE_TYPES, but should be treated as PRIMITIVE_TYPES.
# @param A type returned from the JVM backend.
# @return A type is the key of the PRIMITIVE_TYPES.
specialtypeshandle <- function(type) {
returntype <- NULL
m <- regexec("^decimal(.+)$", type)
matchedStrings <- regmatches(type, m)
if (length(matchedStrings[[1]]) >= 2) {
returntype <- "double"
}
returntype
}
22 changes: 22 additions & 0 deletions R/pkg/inst/tests/testthat/test_sparkSQL.R
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -526,6 +526,17 @@ test_that(
expect_is(newdf, "SparkDataFrame")
expect_equal(count(newdf), 1)
dropTempView("table1")

createOrReplaceTempView(df, "dfView")
sqlCast <- collect(sql("select cast('2' as decimal) as x from dfView limit 1"))
out <- capture.output(sqlCast)
expect_true(is.data.frame(sqlCast))
expect_equal(names(sqlCast)[1], "x")
expect_equal(nrow(sqlCast), 1)
expect_equal(ncol(sqlCast), 1)
expect_equal(out[1], " x")
expect_equal(out[2], "1 2")
dropTempView("dfView")
})

test_that("test cache, uncache and clearCache", {
Expand DownExpand Up@@ -2089,6 +2100,9 @@ test_that("Method coltypes() to get and set R's data types of a DataFrame", {
# Test primitive types
DF <- createDataFrame(data, schema)
expect_equal(coltypes(DF), c("integer", "logical", "POSIXct"))
createOrReplaceTempView(DF, "DFView")
sqlCast <- sql("select cast('2' as decimal) as x from DFView limit 1")
expect_equal(coltypes(sqlCast), "numeric")

# Test complex types
x <- createDataFrame(list(list(as.environment(
Expand DownExpand Up@@ -2132,6 +2146,14 @@ test_that("Method str()", {
"setosa\" \"setosa\" \"setosa\" \"setosa\""))
expect_equal(out[7], " $ col : logi TRUE TRUE TRUE TRUE TRUE TRUE")

createOrReplaceTempView(irisDF2, "irisView")

sqlCast <- sql("select cast('2' as decimal) as x from irisView limit 1")
castStr <- capture.output(str(sqlCast))
expect_equal(length(castStr), 2)
expect_equal(castStr[1], "'SparkDataFrame': 1 variables:")
expect_equal(castStr[2], " $ x: num 2")

# A random dataset with many columns. This test is to check str limits
# the number of columns. Therefore, it will suffice to check for the
# number of returned rows
Expand Down