[SPARK-22981][SQL] Fix incorrect results of Casting Struct to String #20176

maropu · 2018-01-07T06:37:09Z

What changes were proposed in this pull request?

This pr fixed the issue when casting structs into strings;

scala> val df = Seq(((1, "a"), 0), ((2, "b"), 0)).toDF("a", "b")
scala> df.write.saveAsTable("t")                                        
scala> sql("SELECT CAST(a AS STRING) FROM t").show
+-------------------+
|                  a|
+-------------------+
|[0,1,1800000001,61]|
|[0,2,1800000001,62]|
+-------------------+

This pr modified the result into;

+------+
|     a|
+------+
|[1, a]|
|[2, b]|
+------+

How was this patch tested?

Added tests in CastSuite.

maropu · 2018-01-07T06:39:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+      buildCast[InternalRow](_, row => {
+        val builder = new UTF8StringBuilder
+        builder.append("[")
+        if (row.numFields > 0) {


Probably, it seems we have no chance to hit row.numFields == 0 here though, I just leave this for strict checks.

SparkQA · 2018-01-07T08:05:01Z

Test build #85767 has finished for PR 20176 at commit 10285d0.

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

HyukjinKwon · 2018-01-07T08:37:45Z

retest this please

SparkQA · 2018-01-07T11:46:12Z

Test build #85770 has finished for PR 20176 at commit 10285d0.

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

maropu · 2018-01-08T23:40:06Z

@cloud-fan ping

cloud-fan · 2018-01-09T02:57:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+    val structToStringCode = st.zipWithIndex.map { case (ft, i) =>
+      val fieldToStringCode = castToStringCode(ft, ctx)
+      val funcName = ctx.freshName("fieldToString")
+      ctx.addNewFunction(funcName,


no need to create a function, it's called only once.

Actually we can create functions, for data types that appeared more than once among struct fields.

BTW here we may hit 64kb compile error if there are a lot of fields in this struct. We should use ctx.splitExpressions

ok, l'll update soon

SparkQA · 2018-01-09T07:57:05Z

Test build #85834 has finished for PR 20176 at commit 3dcbcc2.

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

SparkQA · 2018-01-09T08:05:01Z

Test build #85838 has finished for PR 20176 at commit 6f5b080.

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

cloud-fan · 2018-01-09T08:25:42Z

retest this please

SparkQA · 2018-01-09T11:32:41Z

Test build #85844 has finished for PR 20176 at commit 6f5b080.

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

## What changes were proposed in this pull request? This pr fixed the issue when casting structs into strings; ``` scala> val df = Seq(((1, "a"), 0), ((2, "b"), 0)).toDF("a", "b") scala> df.write.saveAsTable("t") scala> sql("SELECT CAST(a AS STRING) FROM t").show +-------------------+ | a| +-------------------+ |[0,1,1800000001,61]| |[0,2,1800000001,62]| +-------------------+ ``` This pr modified the result into; ``` +------+ | a| +------+ |[1, a]| |[2, b]| +------+ ``` ## How was this patch tested? Added tests in `CastSuite`. Author: Takeshi Yamamuro <[email protected]> Closes #20176 from maropu/SPARK-22981. (cherry picked from commit 2250cb7) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2018-01-09T14:01:25Z

LGTM, merging to master/2.3!

maropu · 2018-01-09T14:20:08Z

Thanks! Next, I'll fix showString though, one question; currently casting binary to string is different between Cast and showString. Which one is expected?
(I like the postgresql one because we can easily tell the difference between string and binary)

FYI: This casting behaviour is different between mysql and postgresql;

postgres=# create table t (a bytea);
CREATE TABLE
postgres=# insert into t values('abc');
INSERT 0 1
postgres=# SELECT CAST(a AS TEXT) from t;
    a     
----------
 \x616263

mysql> create table t(a blob);
Query OK, 0 rows affected (0.02 sec)

mysql> insert into t values('abc');
Query OK, 1 row affected (0.00 sec)

mysql> select CAST(a AS char) From t;
+-----------------+
| CAST(a AS char) |
+-----------------+
| abc             |
+-----------------+
1 row in set (0.00 sec)

maropu · 2018-01-09T15:02:03Z

hive one is the same with mysql one;

hive> create table t(a BINARY);
OK
hive> INSERT INTO t values('abc');
OK
hive> select CAST(a AS STRING) from t;
OK
abc

cloud-fan · 2018-01-10T02:25:02Z

show binary as string and cast binary to string seems different to me, let's stick with what it is. BTW it's pretty dangerous to change the behavior of cast to be different with Hive.

maropu · 2018-01-10T02:26:32Z

ok, I'll make a follow-up for showString later.

MaxGekk · 2020-07-29T19:47:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+    case StructType(fields) =>
+      buildCast[InternalRow](_, row => {
+        val builder = new UTF8StringBuilder
+        builder.append("[")


Are there any reasons for using the same brackets [] for Struct, Map and Array?

How about [] for arrays, {} for structs and <> for maps or somehow distinguish them. At least, it is not consistent with toHiveString which prints {} for structs and maps.

I think we have no strong reason. In this PR, I just followed the existing format. Looks like we can change it to { for consistency (But, we might need to update the migration doc cuz it change the casting behaivour). cc: @cloud-fan

I'm OK with the new format.

Here is the PR #29308 . Please, review it.

Cast structs to strings

10285d0

maropu commented Jan 7, 2018

View reviewed changes

cloud-fan reviewed Jan 9, 2018

View reviewed changes

split long exprs

6f5b080

maropu force-pushed the SPARK-22981 branch from 3dcbcc2 to 6f5b080 Compare January 9, 2018 06:09

asfgit closed this in 2250cb7 Jan 9, 2018

MaxGekk reviewed Jul 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-22981][SQL] Fix incorrect results of Casting Struct to String #20176

[SPARK-22981][SQL] Fix incorrect results of Casting Struct to String #20176

maropu commented Jan 7, 2018

maropu Jan 7, 2018

SparkQA commented Jan 7, 2018

HyukjinKwon commented Jan 7, 2018

SparkQA commented Jan 7, 2018

maropu commented Jan 8, 2018

cloud-fan Jan 9, 2018

cloud-fan Jan 9, 2018

cloud-fan Jan 9, 2018 •

edited

Loading

maropu Jan 9, 2018

SparkQA commented Jan 9, 2018

SparkQA commented Jan 9, 2018

cloud-fan commented Jan 9, 2018

SparkQA commented Jan 9, 2018

cloud-fan commented Jan 9, 2018

maropu commented Jan 9, 2018 •

edited

Loading

maropu commented Jan 9, 2018

cloud-fan commented Jan 10, 2018

maropu commented Jan 10, 2018

MaxGekk Jul 29, 2020

maropu Jul 29, 2020 •

edited

Loading

cloud-fan Jul 30, 2020

MaxGekk Jul 30, 2020

[SPARK-22981][SQL] Fix incorrect results of Casting Struct to String #20176

[SPARK-22981][SQL] Fix incorrect results of Casting Struct to String #20176

Conversation

maropu commented Jan 7, 2018

What changes were proposed in this pull request?

How was this patch tested?

maropu Jan 7, 2018

Choose a reason for hiding this comment

SparkQA commented Jan 7, 2018

HyukjinKwon commented Jan 7, 2018

SparkQA commented Jan 7, 2018

maropu commented Jan 8, 2018

cloud-fan Jan 9, 2018

Choose a reason for hiding this comment

cloud-fan Jan 9, 2018

Choose a reason for hiding this comment

cloud-fan Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

maropu Jan 9, 2018

Choose a reason for hiding this comment

SparkQA commented Jan 9, 2018

SparkQA commented Jan 9, 2018

cloud-fan commented Jan 9, 2018

SparkQA commented Jan 9, 2018

cloud-fan commented Jan 9, 2018

maropu commented Jan 9, 2018 • edited Loading

maropu commented Jan 9, 2018

cloud-fan commented Jan 10, 2018

maropu commented Jan 10, 2018

MaxGekk Jul 29, 2020

Choose a reason for hiding this comment

maropu Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

cloud-fan Jul 30, 2020

Choose a reason for hiding this comment

MaxGekk Jul 30, 2020

Choose a reason for hiding this comment

cloud-fan Jan 9, 2018 •

edited

Loading

maropu commented Jan 9, 2018 •

edited

Loading

maropu Jul 29, 2020 •

edited

Loading