[SPARK-17608][SPARKR]:Long type has incorrect serialization/deserialization

[wangmiao1981]

What changes were proposed in this pull request?

bigint is not supported in schema and the serialization is not Double.

Add bigint support in schema and serialized and deserialized as Double.

This fix is orthogonal to the precision problem in
https://issues.apache.org/jira/browse/SPARK-12360

How was this patch tested?

Add a new unit test.

[frreiss]

Overall, this looks like a sensible approach to a messy problem.
You might want to think about adding some overflow handling to the SQL-->R translation. That is, if a Dataframe contains a bigint value that cannot be expressed as a Double, it would be safer to convert that value to NaN instead of stripping the lower-order bits off the bigint. The bigint column in the source Dataframe could hold a unique identifier or a hash value.

[SparkQA]

Test build #75815 has finished for PR 17640 at commit 03b82ac.

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

[wangmiao1981]

I will some bound check and error handling. I think there should be some generic handling approach in the Scala side, because Double will also have the problem of overflow. Anyway, I will check the source code to understand the handling.

[wangmiao1981]

cc @felixcheung

[wangmiao1981]

If I use very big number, then sparkR shell will get the following output:

collect(df1)
a b c d
1 Inf 1 1 Inf

So the overflow problem has been taken care of in the Scala side. We don't have to add additional handling in R side.

[felixcheung]

What's the number you use for this big number?

[felixcheung]

Thanks @wangmiao1981
cc @shivaram@sun-rui

Does this address SPARK-12360 too?
Since this changes the serde, this affects more than dapply? Could you please add more test?

[wangmiao1981]

For Inf case, I used a very large number:

138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240138074279341524013807427934152401380742793415240

[wangmiao1981]

Based on my understanding, it does not directly solvethe 12360. This one just solves the serialization of a specific type bigint in struct field.

[shivaram]

+1 on what @felixcheung said -- It'll be good to have more tests in test_Serde.R. Other than that the change looks fine

[wangmiao1981]

I am adding more tests right now.

[felixcheung]

these are great tests to add...

thinking more about this, I think these are actually testing large numeric values, not large integer value, because in R, 1380742793415240 defaults to numeric, vs 1380742793415240L is integer.

though I'm not sure we could specify that as integer as R integer is only 32-bit. so we need a way to get bigint back from JVM side - and since we can't specify bigint on the R side (R integer is only 32-bit), should there be a SQL function test to create such value (and new column with the bigint type) from within the JVM?

[SparkQA]

Test build #75862 has finished for PR 17640 at commit da39bcf.

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

[felixcheung]

@wangmiao1981 do you want to get this into 2.2?

[wangmiao1981]

@felixcheung I just came back from vacation. I will make changes now. Thanks!

[SparkQA]

Test build #76121 has finished for PR 17640 at commit 331b781.

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

[felixcheung]

let me know if I'm mistaken, I think tests in here are only testing doubles/numerics instead of large int?

[wangmiao1981]

This one tests bigint

[felixcheung]

actually, I'm not sure.
Walking through the code, createDataFrame calls parallelize which eventually calls R's serialize. Since these big values are actually numeric, and not integer, serialize writes them in that way

[wangmiao1981]

I don't know how to specify in R console to enforce bigint type.

[wangmiao1981]

I did some google search. R can't specify bigint type. So, we can't directly test bigint type.

We can remove the tests above, as we added schema tests and scala API tests.

[felixcheung]

I think we might need step through this a bit more?

[felixcheung]

actually, I'm not sure.
Walking through the code, createDataFrame calls parallelize which eventually calls R's serialize. Since these big values are actually numeric, and not integer, serialize writes them in that way

[felixcheung]

and another thing, there is no bigint in R
so I'm not sure how we would hit this path

[wangmiao1981]

When specifying schema with bigint, we will hit the bigint path. Without this change, it will thrown an error of type mismatch. But as you said, we can't specify bigint type in R console.

[wangmiao1981]

If R doesn't have bigint type, we should remove all bigint related logic. I don't know the history of bigint mapping in the Types.R file. Why should we have it since every big number is numeric (Double in the backend)?

[wangmiao1981]

@felixcheung Any thoughts? Thanks!

[felixcheung]

if you are referring to https://github.com/apache/spark/blob/master/R/pkg/R/types.R#L25
like it says,

names(PRIMITIVE_TYPES) are Scala types whereas values are equivalent R types.

so bigint there is Scala type, not R type

[wangmiao1981]

I see. But as you mentioned, we don't know how to trigger the write path on the R side, because both bigint and double are numeric. I think we can just remove the test in the R side.

[felixcheung]

I think this is different though, for PRIMITIVE_TYPES, it is used when you create a schema with structField in R. In this case you can definitely define a column as bigint and then pass a R numeric value to it

[wangmiao1981]

When using createDataFrame, R uses serialize to send data to the backend. When taking an action, say, collect, scala side logic refers to the schema field and calls the readTypedObjects where the newly added read logic kicks in. When it returns back to R side, the newly added write logic kicks in and R side can interpret it due to the R side read logic. It seems that the write logic in R side is not called, because we don't have specific type bigint in R. Right?

[wangmiao1981]

For completeness purpose, I think we can keep the write logic in R side.

[SparkQA]

Test build #76306 has finished for PR 17640 at commit 26d7750.

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

[SparkQA]

Test build #76307 has finished for PR 17640 at commit 38fef1d.

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

[jiangxb1987]

@wangmiao1981 Are you still working on this?

[wangmiao1981]

@jiangxb1987 The original PR has some issues that are not correctly handled. I will open a new PR when I figure out the right fix. I intended to close this PR. Thanks for closing it.