[SPARK-33876][SQL] Add length-check for reading char/varchar from tables w/ a external location #30882

yaooqinn · 2020-12-22T04:22:43Z

What changes were proposed in this pull request?

This PR adds the length check to the existing ApplyCharPadding rule. Tables will have external locations when users execute
SET LOCATION or CREATE TABLE ... LOCATION. If the location contains over length values we should FAIL ON READ.

Why are the changes needed?

spark-sql> INSERT INTO t2 VALUES ('1', 'b12345');
Time taken: 0.141 seconds
spark-sql> alter table t set location '/tmp/hive_one/t2';
Time taken: 0.095 seconds
spark-sql> select * from t;
1 b1234

the above case should fail rather than implicitly applying truncation

Does this PR introduce any user-facing change?

no

How was this patch tested?

new tests

…les w/ a external location

yaooqinn · 2020-12-22T04:26:56Z

cc @cloud-fan @maropu @HyukjinKwon thanks for checking this

SparkQA · 2020-12-22T05:24:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37792/

SparkQA · 2020-12-22T06:01:03Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37792/

cloud-fan · 2020-12-22T08:12:52Z

...n/scala/org/apache/spark/sql/execution/datasources/PaddingAndLengthCheckForCharVarChar.scala

 */
-object ApplyCharTypePadding extends Rule[LogicalPlan] {
+object PaddingAndLengthCheckForCharVarChar extends Rule[LogicalPlan] {


CharVarChar -> CharVarchar

sql/core/src/test/scala/org/apache/spark/sql/execution/command/CharVarcharDDLTestBase.scala

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala

…les w/ a external location

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala

SparkQA · 2020-12-22T08:55:45Z

Test build #133194 has finished for PR 30882 at commit 4c92503.

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

SparkQA · 2020-12-22T09:25:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37799/

SparkQA · 2020-12-22T09:28:55Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37798/

SparkQA · 2020-12-22T09:53:37Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37799/

SparkQA · 2020-12-22T10:01:23Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37798/

SparkQA · 2020-12-22T10:16:47Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37801/

SparkQA · 2020-12-22T10:21:28Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37801/

SparkQA · 2020-12-22T10:31:33Z

Test build #133200 has finished for PR 30882 at commit 85bf81d.

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

maropu · 2020-12-22T11:48:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/CharVarcharUtils.scala

+   *
+   * For a CHAR(N) column/field and the length of string value is M
+   * If M > N, raise runtime error
+   * If M <= N, the value should be right-padded to N characters.


nit: M <= N -> M < N?

<= looks more specific with the 'right-padded to N' and the padding is actually applied when =

maropu · 2020-12-22T11:53:35Z

lgtm

SparkQA · 2020-12-22T13:08:14Z

Test build #133201 has finished for PR 30882 at commit 59b6900.

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

cloud-fan · 2020-12-22T13:19:39Z

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala

+    Seq("char", "varchar").foreach { typ =>
+      withTempPath { dir =>
+        withTable("t") {
+          sql("SELECT '12' as c0").write.option("path", dir.toString).save()


can we write sql("SELECT '12' as c0").write.format(format).save(dir.toString) to be more robust?

cloud-fan · 2020-12-22T13:20:29Z

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala

+    Seq("char", "varchar").foreach { typ =>
+      withTempPath { dir =>
+        withTable("t") {
+          sql("SELECT '123456' as c0").write.option("path", dir.toString).save()


cloud-fan · 2020-12-22T13:21:13Z

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala

+      withTempPath { dir =>
+        withTable("t") {
+          sql("SELECT '12' as c0").write.option("path", dir.toString).save()
+          sql(s"CREATE TABLE t (c0 $typ(2)) using $format LOCATION '$dir'")


there is only one column, seems col is a better name.

SparkQA · 2020-12-22T14:05:08Z

Test build #133202 has finished for PR 30882 at commit b48071e.

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

SparkQA · 2020-12-22T14:20:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37817/

cloud-fan · 2020-12-22T14:23:36Z

The last commit is a small update to the CharVarcharTestSuite and I've verified it locally. I'm merging it to master/3.1, thanks!

…les w/ a external location ### What changes were proposed in this pull request? This PR adds the length check to the existing ApplyCharPadding rule. Tables will have external locations when users execute SET LOCATION or CREATE TABLE ... LOCATION. If the location contains over length values we should FAIL ON READ. ### Why are the changes needed? ```sql spark-sql> INSERT INTO t2 VALUES ('1', 'b12345'); Time taken: 0.141 seconds spark-sql> alter table t set location '/tmp/hive_one/t2'; Time taken: 0.095 seconds spark-sql> select * from t; 1 b1234 ``` the above case should fail rather than implicitly applying truncation ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? new tests Closes #30882 from yaooqinn/SPARK-33876. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 6da5cdf) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

SparkQA · 2020-12-22T14:56:09Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37817/

SparkQA · 2020-12-22T18:33:39Z

Test build #133219 has finished for PR 30882 at commit 8e0184e.

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

AmplabJenkins · 2020-12-22T18:35:39Z

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/133219/

[SPARK-33876][SQL] Add length-check for reading char/varchar from tab…

aedea78

…les w/ a external location

github-actions bot added the SQL label Dec 22, 2020

nit

4c92503

cloud-fan reviewed Dec 22, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/CharVarcharDDLTestBase.scala Outdated Show resolved Hide resolved

cloud-fan approved these changes Dec 22, 2020

View reviewed changes

addr comments

85bf81d

cloud-fan reviewed Dec 22, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 22, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala Outdated Show resolved Hide resolved

[SPARK-33876][SQL] Add length-check for reading char/varchar from tab…

59b6900

…les w/ a external location

cloud-fan reviewed Dec 22, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala Outdated Show resolved Hide resolved

nit

b48071e

maropu reviewed Dec 22, 2020

View reviewed changes

cloud-fan reviewed Dec 22, 2020

View reviewed changes

nit

8e0184e

cloud-fan closed this in 6da5cdf Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-33876][SQL] Add length-check for reading char/varchar from tables w/ a external location #30882

[SPARK-33876][SQL] Add length-check for reading char/varchar from tables w/ a external location #30882

yaooqinn commented Dec 22, 2020 •

edited

yaooqinn commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

maropu Dec 22, 2020

yaooqinn Dec 22, 2020 •

edited

maropu commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan Dec 22, 2020

yaooqinn Dec 22, 2020

cloud-fan Dec 22, 2020

cloud-fan Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

AmplabJenkins commented Dec 22, 2020

[SPARK-33876][SQL] Add length-check for reading char/varchar from tables w/ a external location #30882

[SPARK-33876][SQL] Add length-check for reading char/varchar from tables w/ a external location #30882

Conversation

yaooqinn commented Dec 22, 2020 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

yaooqinn commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan Dec 22, 2020

Choose a reason for hiding this comment

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

maropu Dec 22, 2020

Choose a reason for hiding this comment

yaooqinn Dec 22, 2020 • edited

Choose a reason for hiding this comment

maropu commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan Dec 22, 2020

Choose a reason for hiding this comment

yaooqinn Dec 22, 2020

Choose a reason for hiding this comment

cloud-fan Dec 22, 2020

Choose a reason for hiding this comment

cloud-fan Dec 22, 2020

Choose a reason for hiding this comment

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

cloud-fan commented Dec 22, 2020

SparkQA commented Dec 22, 2020

SparkQA commented Dec 22, 2020

AmplabJenkins commented Dec 22, 2020

yaooqinn commented Dec 22, 2020 •

edited

yaooqinn Dec 22, 2020 •

edited