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

[wangmiao1981]

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

registerTempTable(createDataFrame(iris), "iris")
str(collect(sql("select cast('1' as double) as x, cast('2' as decimal) as y from iris limit 5")))

'data.frame': 5 obs. of 2 variables:
$ x: num 1 1 1 1 1
$ y:List of 5
..$ : num 2
..$ : num 2
..$ : num 2
..$ : num 2
..$ : num 2

The problem is that spark returns decimal(10, 0) col type, instead of decimal. Thus, decimal(10, 0) is not handled correctly. It should be handled as "double".

As discussed in JIRA thread, we can have two potential fixes:
1). Scala side fix to add a new case when writing the object back; However, I can't use spark.sql.types._ in Spark core due to dependency issues. I don't find a way of doing type case match;

2). SparkR side fix: Add a helper function to check special type like "decimal(10, 0)" and replace it with double, which is PRIMITIVE type. This special helper is generic for adding new types handling in the future.

I open this PR to discuss pros and cons of both approaches. If we want to do Scala side fix, we need to find a way to match the case of DecimalType and StructType in Spark Core.

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

Manual test:

str(collect(sql("select cast('1' as double) as x, cast('2' as decimal) as y from iris limit 5")))
'data.frame': 5 obs. of 2 variables:
$ x: num 1 1 1 1 1
$ y: num 2 2 2 2 2
R Unit tests

[SparkQA]

Test build #63648 has finished for PR 14613 at commit e95f557.

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

[shivaram]

@wangmiao1981 Thanks for the PR. Could we add a couple of test cases for this ? It'll also help me understand what is the expected behavior -- one of them could be for collect with decimals and another one could be for str on a Spark DatatFrame which contains decimals.

[wangmiao1981]

@shivaram Sure. I will add unit tests.

[SparkQA]

Test build #63710 has finished for PR 14613 at commit b333cda.

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

[felixcheung]

maybe this should go into types.R?
Could you add some documentation comment what this is doing and why?

[wangmiao1981]

I will add comments. Thanks!

[SparkQA]

Test build #63775 has finished for PR 14613 at commit 61b7a48.

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

[felixcheung]

since this is the collect DataFrame test, perhaps you want to move these to the sql test or the createOrReplaceTempView test?

[wangmiao1981]

@felixcheung Let me think about your comments and I will get back soon.

[wangmiao1981]

@felixcheung I changed the tests according to your comments. For the specialtypeshandle, it should return the key in PRIMITIVE_TYPES based on the original backend return type. In the caller, I did adjustment on the types to make sure it is consistent with R native type.

[SparkQA]

Test build #64297 has finished for PR 14613 at commit abce7ff.

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

[felixcheung]

should this go `"^decimal(.+)$"?

[felixcheung]

why do we switch on the first character? why don't we regex on the full string?

[wangmiao1981]

In case, there are other types that we want to handle in the future. Right?

[felixcheung]

I'd just simplify this at this point. It would be easy to add the additional checks later

[felixcheung]

@sun-rui@shivaram thought?

[SparkQA]

Test build #64808 has finished for PR 14613 at commit 4d6d048.

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

[SparkQA]

Test build #64834 has finished for PR 14613 at commit 4e9e403.

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