[SPARK-26263][SQL] Validate partition values with user provided schema #23215

gengliangwang · 2018-12-04T09:46:28Z

What changes were proposed in this pull request?

Currently if user provides data schema, partition column values are converted as per it. But if the conversion failed, e.g. converting string to int, the column value is null.

This PR proposes to throw exception in such case, instead of converting into null value silently:

These null partition column values doesn't make sense to users in most cases. It is better to show the conversion failure, and then users can adjust the schema or ETL jobs to fix it.
There are always exceptions on such conversion failure for non-partition data columns. Partition columns should have the same behavior.

We can reproduce the case above as following:

/tmp/testDir
├── p=bar
└── p=foo

If we run:

val schema = StructType(Seq(StructField("p", IntegerType, false)))
spark.read.schema(schema).csv("/tmp/testDir/").show()

We will get:

+----+
|   p|
+----+
|null|
|null|
+----+

How was this patch tested?

Unit test

gengliangwang · 2018-12-04T09:46:51Z

@cloud-fan

cloud-fan · 2018-12-04T11:42:33Z

I think this new behavior makes more sense, but we need to add a migration guide.

SparkQA · 2018-12-04T13:23:37Z

Test build #99657 has finished for PR 23215 at commit 7060e12.

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

SparkQA · 2018-12-04T19:36:47Z

Test build #99671 has finished for PR 23215 at commit 4719765.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2018-12-04T19:36:53Z

Test build #99672 has finished for PR 23215 at commit 272bb1d.

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

SparkQA · 2018-12-04T22:13:38Z

Test build #99681 has finished for PR 23215 at commit 4060c30.

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

…to specific type

SparkQA · 2018-12-06T08:05:01Z

Test build #99755 has finished for PR 23215 at commit ce2db28.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-12-06T08:05:02Z

Test build #99754 has finished for PR 23215 at commit b24134a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-12-06T08:05:02Z

Test build #99753 has finished for PR 23215 at commit 8e1653b.

This patch fails due to an unknown error code, -9.
This patch does not merge cleanly.
This patch adds no public classes.

gengliangwang · 2018-12-06T08:23:52Z

retest this please.

HyukjinKwon · 2018-12-06T09:02:27Z

+1 from me too. It makes sense to me and having configuration sounds good.

viirya · 2018-12-06T09:54:18Z

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

+        val columnValue = columnValueLiteral.eval()
+        val castedValue = Cast(columnValueLiteral, dataType, Option(timeZone.getID)).eval()
+        if (validatePartitionColumns && columnValue != null && castedValue == null) {
+          throw new RuntimeException(s"Failed to cast partition value `$columnValue` to $dataType")


Can we also show columnName in this exception message? So it is easier to know which partition column has such error.

Good suggestion. Thanks.

viirya · 2018-12-06T09:58:47Z

Sounds good to me too. As there is a config, it is good that we can still disable it.

SparkQA · 2018-12-06T12:14:35Z

Test build #99761 has finished for PR 23215 at commit ce2db28.

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

cloud-fan · 2018-12-06T13:33:34Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/FileIndexSuite.scala

@@ -95,6 +95,31 @@ class FileIndexSuite extends SharedSQLContext {
    }
  }

+  test("SPARK-26263: Throw exception when partition value can't be converted to specific type") {


can't be casted to user-specified type

cloud-fan · 2018-12-06T13:34:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .doc("When this option is set to true, partition column values will be validated with " +
+        "provided schema. If the validation fails, a runtime exception is thrown." +
+        "When this option is set to false, the partition column value will be converted to null " +
+        "if it can not be converted to corresponding provided schema.")


... can not be casted to ...

viirya · 2018-12-06T13:56:10Z

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

+        if (validatePartitionColumns && columnValue != null && castedValue == null) {
+          throw new RuntimeException(s"Failed to cast value `$columnValue` to `$dataType` " +
+            s"for partition column `$columnName`")
+        }
        Literal.create(castedValue, userSpecifiedDataTypes(columnName))


Literal.create(castedValue, dataType)

SparkQA · 2018-12-06T15:39:23Z

Test build #99770 has finished for PR 23215 at commit 6f4e652.

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

gengliangwang · 2018-12-06T16:08:30Z

retest this please.

SparkQA · 2018-12-06T16:11:48Z

Test build #99766 has finished for PR 23215 at commit 25f7039.

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

SparkQA · 2018-12-06T17:22:50Z

Test build #99769 has finished for PR 23215 at commit e13147a.

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

SparkQA · 2018-12-06T19:45:05Z

Test build #99776 has finished for PR 23215 at commit 6f4e652.

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

cloud-fan · 2018-12-07T03:13:44Z

thanks, merging to master!

gengliangwang · 2018-12-07T04:27:48Z

Thank you @cloud-fan @viirya @HyukjinKwon .

## What changes were proposed in this pull request? Currently if user provides data schema, partition column values are converted as per it. But if the conversion failed, e.g. converting string to int, the column value is null. This PR proposes to throw exception in such case, instead of converting into null value silently: 1. These null partition column values doesn't make sense to users in most cases. It is better to show the conversion failure, and then users can adjust the schema or ETL jobs to fix it. 2. There are always exceptions on such conversion failure for non-partition data columns. Partition columns should have the same behavior. We can reproduce the case above as following: ``` /tmp/testDir ├── p=bar └── p=foo ``` If we run: ``` val schema = StructType(Seq(StructField("p", IntegerType, false))) spark.read.schema(schema).csv("/tmp/testDir/").show() ``` We will get: ``` +----+ | p| +----+ |null| |null| +----+ ``` ## How was this patch tested? Unit test Closes apache#23215 from gengliangwang/SPARK-26263. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

gengliangwang changed the title ~~[SPARK-26263][SQL] Throw exception when Partition column value can't be converted to user specified type~~ [SPARK-26263][SQL] Validate partition values with user provided schema Dec 4, 2018

gengliangwang force-pushed the SPARK-26263 branch from 4719765 to 272bb1d Compare December 4, 2018 15:33

gengliangwang added 4 commits December 6, 2018 13:33

SPARK-26263: Throw exception when partition value can't be converted …

db71b46

…to specific type

address comments

85f95f9

change conf name

f741f79

rename conf

b24134a

gengliangwang force-pushed the SPARK-26263 branch from 8e1653b to b24134a Compare December 6, 2018 05:37

revise

ce2db28

viirya reviewed Dec 6, 2018

View reviewed changes

include partition column in error message

25f7039

cloud-fan reviewed Dec 6, 2018

View reviewed changes

cloud-fan approved these changes Dec 6, 2018

View reviewed changes

address comments

e13147a

viirya approved these changes Dec 6, 2018

View reviewed changes

revise

6f4e652

HyukjinKwon approved these changes Dec 7, 2018

View reviewed changes

asfgit closed this in 5a140b7 Dec 7, 2018

[SPARK-26263][SQL] Validate partition values with user provided schema #23215

[SPARK-26263][SQL] Validate partition values with user provided schema #23215

Uh oh!

Conversation

gengliangwang commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gengliangwang commented Dec 4, 2018

Uh oh!

cloud-fan commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 4, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

gengliangwang commented Dec 6, 2018

Uh oh!

HyukjinKwon commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

gengliangwang Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

viirya commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

cloud-fan Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

viirya Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

gengliangwang commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

SparkQA commented Dec 6, 2018

Uh oh!

cloud-fan commented Dec 7, 2018

Uh oh!

gengliangwang commented Dec 7, 2018

Uh oh!

Uh oh!

gengliangwang commented Dec 4, 2018 •

edited

Loading

HyukjinKwon commented Dec 6, 2018 •

edited

Loading