[SPARK-13866][SQL] Handle decimal type in CSV inference at CSV data source. #11724

HyukjinKwon · 2016-03-15T10:03:09Z

What changes were proposed in this pull request?

https://issues.apache.org/jira/browse/SPARK-13866

This PR adds the support to infer DecimalType.
Here are the rules between IntegerType, LongType and DecimalType.

Infering Types

IntegerType and then LongTypeare tried first.

Int.MaxValue => IntegerType
Long.MaxValue => LongType

If it fails, try DecimalType.
```
(Long.MaxValue + 1) => DecimalType(20, 0)
```
This does not try to infer this as DecimalType when scale is less than 0.

if it fails, try DoubleType

0.1 => DoubleType // This is failed to be inferred as `DecimalType` because it has the scale, 1.

Compatible Types (Merging Types)

For merging types, this is the same with JSON data source. If DecimalType is not capable, then it becomes DoubleType

How was this patch tested?

Unit tests were used and ./dev/run_tests for code style test.

HyukjinKwon · 2016-03-15T10:03:43Z

cc @rxin @falaki

HyukjinKwon · 2016-03-15T10:06:30Z

There should be a conflict with #11550.

I will resolve the conflict as soon as either this one or that one is merged.

SparkQA · 2016-03-15T12:03:49Z

Test build #53187 has finished for PR 11724 at commit ed1d499.

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

SparkQA · 2016-03-15T14:11:00Z

Test build #53194 has finished for PR 11724 at commit c144797.

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

falaki · 2016-03-16T00:32:41Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVInferSchema.scala

        case IntegerType => tryParseInteger(field)
        case LongType => tryParseLong(field)
        case DoubleType => tryParseDouble(field)
+        case _: DecimalType => tryParseDecimal(field)


To be consistent:

case DecimalType => tryPraseDecimal(field)

I added the _ because DecimalType looks referencing the companion object. I tried that expression before but this emits the compilation error below.

Error:(89, 14) pattern type is incompatible with expected type; found : org.apache.spark.sql.types.DecimalType.type required: org.apache.spark.sql.types.DataType Note: if you intended to match against the class, try `case DecimalType(_,_)` case DecimalType => tryParseDecimal(field) ^

SparkQA · 2016-03-16T02:28:37Z

Test build #53256 has finished for PR 11724 at commit 978da28.

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

SparkQA · 2016-03-16T02:42:59Z

Test build #53254 has finished for PR 11724 at commit 439de08.

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

SparkQA · 2016-03-16T02:51:57Z

Test build #53257 has finished for PR 11724 at commit 6ad04cc.

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

SparkQA · 2016-03-16T08:31:35Z

Test build #53292 has finished for PR 11724 at commit fa42c1d.

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

HyukjinKwon · 2016-03-28T03:38:55Z

@falaki Could you take a look at this please?

SparkQA · 2016-03-28T05:15:09Z

Test build #54295 has finished for PR 11724 at commit 6de6e63.

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

HyukjinKwon · 2016-04-22T04:10:24Z

@rxin I am willing to close this one if you are not sure of this one.

SparkQA · 2016-05-02T02:41:30Z

Test build #57495 has finished for PR 11724 at commit bdaac7c.

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

rxin · 2016-05-03T08:18:32Z

@HyukjinKwon unfortunately this is too confusing. Can you precisely describe the inference rule in the pr description, and create (unit - not end to end) test cases for the rules?

HyukjinKwon · 2016-05-03T08:21:58Z

@rxin Sure I will add a more explicit description and some more unit tests for this. Thanks.

HyukjinKwon · 2016-05-03T11:19:03Z

@rxin I added some more commits for unit tests in CSVInferSchemaSuite. It's okay to hear that it is still confusing or looks not covered enough if it is. I can improve them more.

SparkQA · 2016-05-03T12:30:40Z

Test build #57627 has finished for PR 11724 at commit 0fdd796.

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

SparkQA · 2016-05-03T12:43:03Z

Test build #57629 has finished for PR 11724 at commit adb8747.

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

rxin · 2016-05-03T18:57:11Z

I actually worry that we are inferring things directly as decimals for floating point numbers, because a lot of formats and tools don't necessarily handle decimals well.

It seems like the problem here is only for large ints. Is it possible to only use decimal if they are integers, and otherwise prefer floating point numbers?

HyukjinKwon · 2016-05-04T00:08:19Z

@rxin I see. Thank you. Let me fix this up and change the description as well with some rules for LongType, DoubleType and DecimalType.

SparkQA · 2016-05-04T02:08:13Z

Test build #57698 has finished for PR 11724 at commit a2bf0c7.

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

SparkQA · 2016-05-04T02:17:44Z

Test build #57701 has finished for PR 11724 at commit 9593ae3.

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

HyukjinKwon · 2016-05-13T03:58:44Z

@rxin Do you mind if I ask a quick look again?

rxin · 2016-05-13T04:53:38Z

cc @davies can you review this?

davies · 2016-05-13T05:30:50Z

LGTM

davies · 2016-05-13T05:30:54Z

Merging this into master and 2.0, thanks!

…source. ## What changes were proposed in this pull request? https://issues.apache.org/jira/browse/SPARK-13866 This PR adds the support to infer `DecimalType`. Here are the rules between `IntegerType`, `LongType` and `DecimalType`. #### Infering Types 1. `IntegerType` and then `LongType`are tried first. ```scala Int.MaxValue => IntegerType Long.MaxValue => LongType ``` 2. If it fails, try `DecimalType`. ```scala (Long.MaxValue + 1) => DecimalType(20, 0) ``` This does not try to infer this as `DecimalType` when scale is less than 0. 3. if it fails, try `DoubleType` ```scala 0.1 => DoubleType // This is failed to be inferred as `DecimalType` because it has the scale, 1. ``` #### Compatible Types (Merging Types) For merging types, this is the same with JSON data source. If `DecimalType` is not capable, then it becomes `DoubleType` ## How was this patch tested? Unit tests were used and `./dev/run_tests` for code style test. Author: hyukjinkwon <gurwls223@gmail.com> Author: Hyukjin Kwon <gurwls223@gmail.com> Closes #11724 from HyukjinKwon/SPARK-13866. (cherry picked from commit 51841d7) Signed-off-by: Davies Liu <davies.liu@gmail.com>

HyukjinKwon added 3 commits March 15, 2016 18:55

Support for inferring DecimalType.

1d3e45e

Remove an unused temp variable

9415495

Update comments

ed1d499

Update comments

c144797

falaki reviewed Mar 16, 2016
View reviewed changes

HyukjinKwon added 3 commits March 16, 2016 10:10

Update test data and change some comments

439de08

Add a cast for scale test in inferring tests

978da28

Fix typo

6ad04cc

Resolve conflicts

fa42c1d

Try DecimalType first.

6de6e63

Resolve conflicts

bdaac7c

HyukjinKwon added 3 commits May 3, 2016 19:43

Fetch upstream

605e275

Add some more tests in CSVInferSchemaSuite

0fdd796

Add some comments in merging types

adb8747

HyukjinKwon added 7 commits May 4, 2016 09:46

Infer decimals only for values bigger than longs.

0f3285e

Add removed test by mistake

ebab61a

Remove unused import

a405d07

Leave the comparison for double as they were

fd68521

Indentation as well

a2bf0c7

Leave indentation

f28cf92

Let's do not touch other tests

9593ae3

asfgit closed this in 51841d7 May 13, 2016

HyukjinKwon deleted the SPARK-13866 branch January 2, 2018 03:40

[SPARK-13866][SQL] Handle decimal type in CSV inference at CSV data source. #11724

[SPARK-13866][SQL] Handle decimal type in CSV inference at CSV data source. #11724

Uh oh!

Conversation

HyukjinKwon commented Mar 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Infering Types

Compatible Types (Merging Types)

How was this patch tested?

Uh oh!

HyukjinKwon commented Mar 15, 2016

Uh oh!

HyukjinKwon commented Mar 15, 2016

Uh oh!

SparkQA commented Mar 15, 2016

Uh oh!

SparkQA commented Mar 15, 2016

Uh oh!

falaki Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Mar 16, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 16, 2016

Uh oh!

SparkQA commented Mar 16, 2016

Uh oh!

SparkQA commented Mar 16, 2016

Uh oh!

SparkQA commented Mar 16, 2016

Uh oh!

HyukjinKwon commented Mar 28, 2016

Uh oh!

SparkQA commented Mar 28, 2016

Uh oh!

HyukjinKwon commented Apr 22, 2016

Uh oh!

SparkQA commented May 2, 2016

Uh oh!

rxin commented May 3, 2016

Uh oh!

HyukjinKwon commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 3, 2016

Uh oh!

SparkQA commented May 3, 2016

Uh oh!

rxin commented May 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

HyukjinKwon commented May 13, 2016

Uh oh!

rxin commented May 13, 2016

Uh oh!

davies commented May 13, 2016

Uh oh!

davies commented May 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Mar 15, 2016 •

edited

Loading

HyukjinKwon commented May 3, 2016 •

edited

Loading

HyukjinKwon commented May 3, 2016 •

edited

Loading

rxin commented May 3, 2016 •

edited

Loading