-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-10863][SPARKR] Method coltypes() to get R's data types of a DataFrame #8984
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.
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
Changes from 1 commit
461714d
e9e34b5
c65b682
cee871c
0851163
7a8e62a
de6d164
a346cc6
6c4dcbc
99e6304
30c5d26
4a92d99
a68f97a
360156c
0c2da6c
909e4e3
3cd2079
a7723d9
0a0b278
7e89935
21c0799
fee5a2e
e1056ab
75f5ced
908abf4
37bdc46
3b5c2d5
001884a
eaaf178
9a9618e
25faa4e
57a47a4
e5ab466
772de99
67b12a4
0bb39dc
8aa13ef
9b36955
95a8ece
462b1f1
cd033c0
ba091fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2102,6 +2102,7 @@ setMethod("as.data.frame", | |
| stop(paste("Unused argument(s): ", paste(list(...), collapse=", "))) | ||
| } | ||
| collect(x) | ||
| <<<<<<< HEAD | ||
| }) | ||
|
|
||
| #' The specified DataFrame is attached to the R search path. This means that | ||
|
|
@@ -2152,3 +2153,31 @@ setMethod("with", | |
| newEnv <- assignNewEnv(data) | ||
| eval(substitute(expr), envir = newEnv, enclos = newEnv) | ||
| }) | ||
|
|
||
| #' Returns the column types of a DataFrame. | ||
| #' | ||
| #' @name coltypes | ||
| #' @title Get column types of a DataFrame | ||
| #' @param x (DataFrame) | ||
| #' @return value (character) A character vector with the column types of the given DataFrame | ||
| #' @rdname coltypes | ||
| setMethod("coltypes", | ||
| signature(x = "DataFrame"), | ||
| function(x) { | ||
| # TODO: This may be moved as a global parameter | ||
| # These are the supported data types and how they map to | ||
| # R's data types | ||
| DATA_TYPES <- c("string"="character", | ||
| "double"="numeric", | ||
| "int"="integer", | ||
| "long"="integer", | ||
| "boolean"="long" | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You only handle primitive types here, but no complex types, like Array, Struct and Map. It would be better you can refactor the type mapping related code here and that in SerDe.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sun-rui For complex types (Array/Struct/Map), I can't think of any mapping to R types. Therefore, as agreed with @felixcheung and @shivaram, these will remain the same. For example: Original column types: ["string", "boolean", "map..."]
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @olarayej I think the fall back mechanism here is good. But @sun-rui makes another good point that it will be good to have one unified place where we do a mapping from R types to java types. Right now part of that is in serialize.R / deserialize.R Could you see if there is some refactoring we could do for this to not be duplicated ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sun-rui @shivaram In file serialize.R, method writeType (see below) turns the full data type into a one-character string. Then, method readTypedObject (see below), uses this one-character type to read accordingly. I suspect this is because complex types could be like map (String,String)? In my opinion, it would be better to use the full data type, as opposed to the first letter (which could be especially confusing since we support data types starting with the same letter Date/Double, String/Struct). Also, having the full data type would allow for centralizing the data types in one place, though this would require some major changes We could have mapping arrays: PRIMITIVE_TYPES <- c("string"="character", COMPLEX_TYPES <- c("map", "array", "struct", ...) DATA_TYPES <- c(PRIMITIVE_TYPES, COMPLEX_TYPES) And then we'd need to modify deserialize.R, serialize.R, and schema.R to acknowledge these accordingly. Thoughts? writeType <- function(con, class) { readTypedObject <- function(con, type) {
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The single character names are to reduce the amount of data serialized when we transfer these data types to the JVM. Its not meant to be remembered by anybody so I don't see it being a source of confusion. @sun-rui also added tests which ensure these mappings don't break. However I think having a list of primitive types, complex types and mapping in a common file (types.R ?) sounds good to me. |
||
|
|
||
| # Get the data types of the DataFrame by invoking dtypes() function. | ||
| # Some post-processing is needed. | ||
| types <- as.character(t(as.data.frame(dtypes(x))[2, ])) | ||
|
|
||
| # Map Spark data types into R's data types | ||
| as.character(DATA_TYPES[types]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you check for the case when it doesn't match the known types?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixcheung Yeah, that's a good point. I'm thinking coltypes() should always have an equivalent R data type for each column. We don't want method coltypes() to return NA's or throw an unsupported-type error cuz that would mean that the input DataFrame is inconsistent. Therefore, it'd be just a matter of putting in DATA_TYPES, the list all possible values returned by dtypes() (If I'm missing any). I couldn't find that in the docs. Could you point me to the list? Finally, I think the check for unsupported data types should be done instead in the coltypes()<- method and in the DataFrame initialization. coltypes() assumes the input DataFrame was assigned valid data types, which makes sense to me.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @felixcheung, @shivaram: Any thoughts on this one?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. http://spark.apache.org/docs/latest/sql-programming-guide.html#data-types is a list that might be helpful. Also I think it might make sense to try and map them to R types and if we fail to find a relevant one we fallback to the SparkSQL type
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shivaram I agree. I could use the mapping below (got the short types from schema.R:118): In any other case, I will use the same scala type. Sounds good?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. This sounds good. |
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1027,7 +1027,6 @@ setGeneric("weekofyear", function(x) { standardGeneric("weekofyear") }) | |
| #' @export | ||
| setGeneric("year", function(x) { standardGeneric("year") }) | ||
|
|
||
|
|
||
| #' @rdname glm | ||
| #' @export | ||
| setGeneric("glm") | ||
|
|
@@ -1047,3 +1046,7 @@ setGeneric("attach") | |
| #' @rdname with | ||
| #' @export | ||
| setGeneric("with") | ||
|
|
||
| #' @rdname coltypes | ||
| #' @export | ||
| setGeneric("coltypes", function(x) standardGeneric("coltypes")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: { standardGeneric("coltypes") } |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1460,13 +1460,15 @@ test_that("SQL error message is returned from JVM", { | |
| expect_equal(grepl("Table not found: blah", retError), TRUE) | ||
| }) | ||
|
|
||
| irisDF <- createDataFrame(sqlContext, iris) | ||
|
|
||
| test_that("Method as.data.frame as a synonym for collect()", { | ||
| irisDF <- createDataFrame(sqlContext, iris) | ||
| expect_equal(as.data.frame(irisDF), collect(irisDF)) | ||
| irisDF2 <- irisDF[irisDF$Species == "setosa", ] | ||
| expect_equal(as.data.frame(irisDF2), collect(irisDF2)) | ||
| }) | ||
|
|
||
| <<<<<<< HEAD | ||
| test_that("attach() on a DataFrame", { | ||
| df <- jsonFile(sqlContext, jsonPath) | ||
| expect_error(age) | ||
|
|
@@ -1496,6 +1498,10 @@ test_that("with() on a DataFrame", { | |
| expect_equal(nrow(sum2), 35) | ||
| }) | ||
|
|
||
| test_that("Method coltypes() to get R's data types of a DataFrame", { | ||
| expect_equal(coltypes(irisDF), c(rep("numeric", 4), "character")) | ||
| }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a test with some other types ? Also another one which runs into the |
||
|
|
||
| unlink(parquetPath) | ||
| unlink(jsonPath) | ||
| unlink(jsonPathNa) | ||
| unlink(jsonPathNa) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the style of function description to be more consistent with other existing ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change this when updating my PR #9218