[SPARK-24849][SPARK-24911][SQL] Converting a value of StructType to a DDL string #21803

MaxGekk · 2018-07-18T14:47:36Z

What changes were proposed in this pull request?

In the PR, I propose to extend the StructType/StructField classes by new method toDDL which converts a value of the StructType/StructField type to a string formatted in DDL style. The resulted string can be used in a table creation.

The toDDL method of StructField is reused in SHOW CREATE TABLE. In this way the PR fixes the bug of unquoted names of nested fields.

How was this patch tested?

I add a test for checking the new method and 2 round trip tests: fromDDL -> toDDL and toDDL -> fromDDL

maropu · 2018-07-18T14:59:51Z

How about the case where a column name has special characters that should be backquoted, e.g., 'aaa:bbb'?

MaxGekk · 2018-07-18T17:53:38Z

@maropu I added quoting of column names

SparkQA · 2018-07-18T18:40:21Z

Test build #93235 has finished for PR 21803 at commit 34511db.

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

SparkQA · 2018-07-18T21:53:29Z

Test build #93242 has finished for PR 21803 at commit f302777.

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

maropu · 2018-07-19T02:04:12Z

(As I described in the jira) What's this func is used for? Is this related to the other work?

maropu · 2018-07-19T02:10:55Z

sql/catalyst/src/main/scala/org/apache/spark/sql/types/StructType.scala

+   * `StructType(Seq(StructField("a", IntegerType)))` should be converted to `a int`
+   */
+  def toDDL(struct: StructType): String = {
+    struct.map(field => s"${quoteIdentifier(field.name)} ${field.dataType.sql}").mkString(",")


Can this also handle the special character ('\n', '\t', '', ...) that needs an escape?

We use similar code in SHOW CREATE TABLE:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

Line 999 in bac50aa

s"${quoteIdentifier(column.name)} ${column.dataType.catalogString}${comment.getOrElse("")}"

MaxGekk · 2018-07-19T06:53:30Z

@hvanhovell Could you look at the PR please.

MaxGekk · 2018-07-19T06:55:06Z

(As I described in the jira) What's this func is used for?

@maropu I answered in JIRA, please, look at it.

HyukjinKwon

@MaxGekk, is the purpose of this API is to have a int instead of struct<a: int>?

You can use struct.map(field => s"${quoteIdentifier(field.name)} ${field.dataType.sql}").mkString(",") in the application code or you could simply:

scala> DataType.fromDDL(StructType.fromDDL("a int").catalogString)
res4: org.apache.spark.sql.types.DataType = StructType(StructField(a,IntegerType,true))

HyukjinKwon · 2018-07-19T08:32:14Z

We should reduce APIs exposed .. there are already a hell of a lot..

MaxGekk · 2018-07-19T09:09:40Z

is the purpose of this API is to have a int instead of struct<a: int>

Basically, yes. All those methods simpleString(), catalogString(), sql() return struct< ... : ...> which is impossible to uses in a table creation because of : and struct

We should reduce APIs exposed .. there are already a hell of a lot..

I believe we should reduce user's/customer's pain first of all. I think the function which is opposite to fromDLL looks pretty natural here.

HyukjinKwon · 2018-07-19T09:16:15Z

Does the alternative code I posted above work in that case? If so, sounds we are adding an API just for consistency

MaxGekk · 2018-07-19T09:42:30Z

Does the alternative code I posted above work in that case?
You can use struct.map(field => s"${quoteIdentifier(field.name)} ${field.dataType.sql}").mkString(",") in the application code

It is actually the same as in the PR. For sure, it works ( I added tests for that ;-) )

or you could simply:
scala> DataType.fromDDL(StructType.fromDDL("a int").catalogString)

Sorry, I didn't catch the example. Just in case, I need to read one avro file from a folder, take a schema from it in DDL format and create table by using the string. This is the use case.

If so, sounds we are adding an API just for consistency ...

Not only, we are adding the method to cover the use case which I described above. And for consistency too. Having useful API, isn't it beautiful?

HyukjinKwon · 2018-07-19T14:30:20Z

Ah, I misunderstood then. Thing is, fromDDL was added because it's needed other APIs rather then it's own purpose. I wonder how commonly it will be used to be honest. I haven't seen such requests before frankly.

In my case, I usually leave mailing list or stackoverflow questions as references usually though. It's one liner code and I think we haven't added APIs when the workarounds are easy usually so far. That doesn't quite match to what I am used to personally at least.

MaxGekk · 2018-07-19T14:56:54Z

Thing is, fromDDL was added because it's needed other APIs rather then it's own purpose.

The toDDL function can be reused from SHOW CREATE TABLE too:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala

Line 999 in bac50aa

    
           s"${quoteIdentifier(column.name)} ${column.dataType.catalogString}${comment.getOrElse("")}"

I wonder how commonly it will be used to be honest. I haven't seen such requests before frankly.

Frankly speaking I cannot say there is high demand for the method from our users.

It's one liner code and I think we haven't added APIs when the workarounds are easy usually so far.

Do you think I should close the PR?

HyukjinKwon · 2018-07-19T15:02:25Z

To me yup, but if you are in doubt, I am perfectly okay with waiting some more days and see if other opinions arrive.

rxin · 2018-07-20T16:45:41Z

Should we do schema.toDDL, or StructType.toDDL(schema)?

gatorsmile · 2018-07-20T16:49:14Z

schema.toDDL is more friendly.

MaxGekk · 2018-07-20T19:18:54Z

Should we do schema.toDDL, or StructType.toDDL(schema)?
schema.toDDL is more friendly.

For sure, schema.toDDL looks more natural. I put toDDL to the StructType object only to have it closer to fromDDL and to do not pollute the namespace of the StructType class which already has inherited from DataType similar methods simpleString(), catalogString() and sql()

SparkQA · 2018-07-21T18:07:28Z

Test build #93387 has finished for PR 21803 at commit d542bf3.

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

MaxGekk · 2018-07-24T13:19:52Z

@gatorsmile @rxin I moved toDDL to the StructType class and reused it from SHOW CREATE TABLE (fixed the bug of unquoted field names). Please, have take a look at the PR.

gatorsmile · 2018-07-24T16:00:32Z

@MaxGekk Thanks for fixing the issue in SHOW CREATE TABLE! Could you open a JIRA about SHOW CREATE TABLE and add the JIRA number to this PR. Also, could you add the failed case in ShowCreateTableSuite?

SparkQA · 2018-07-25T02:53:02Z

Test build #93522 has finished for PR 21803 at commit 738e97c.

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

HyukjinKwon · 2018-07-25T03:07:16Z

I am okay now just for clarification if you guys feel strong on this ~

gatorsmile · 2018-07-25T03:29:24Z

It is nice to have. Actually, I believe we need to fix the bug in SHOW CREATE TABLE, which is widely used.

HyukjinKwon · 2018-07-25T03:49:49Z

yup the current change sounds okay.

gatorsmile · 2018-07-25T07:13:46Z

@MaxGekk Please include the test case for SHOW CREATE TABLE. Thanks!

SparkQA · 2018-07-25T14:37:17Z

Test build #93540 has finished for PR 21803 at commit 60f663d.

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

gatorsmile · 2018-07-25T18:08:36Z

LGTM

Thanks! Merged to master

… DDL string In the PR, I propose to extend the `StructType`/`StructField` classes by new method `toDDL` which converts a value of the `StructType`/`StructField` type to a string formatted in DDL style. The resulted string can be used in a table creation. The `toDDL` method of `StructField` is reused in `SHOW CREATE TABLE`. In this way the PR fixes the bug of unquoted names of nested fields. I add a test for checking the new method and 2 round trip tests: `fromDDL` -> `toDDL` and `toDDL` -> `fromDDL` Author: Maxim Gekk <maxim.gekk@databricks.com> Closes apache#21803 from MaxGekk/to-ddl.

MaxGekk added 3 commits July 18, 2018 16:33

New method - toDDL

38f905a

Simplification of a test

6e05093

New test for cases

34511db

quoting of column identifiers

f302777

maropu reviewed Jul 19, 2018

View reviewed changes

HyukjinKwon reviewed Jul 19, 2018

View reviewed changes

MaxGekk added 4 commits July 21, 2018 15:07

Moving toDDL from object to StructType class

d49316f

Support comments in fields

d92b546

Moving a part of toDDL to StructField

8101b4a

Reusing toDDL of StructField

d542bf3

Merge remote-tracking branch 'origin/master' into to-ddl

8f78386

MaxGekk changed the title ~~[SPARK-24849][SQL] Converting a value of StructType to a DDL string~~ [SPARK-24849][SPARK-24911][SQL] Converting a value of StructType to a DDL string Jul 24, 2018

Reuse toDDL

738e97c

Adding a test for SHOW CREATE TABLE

60f663d

asfgit closed this in 2f77616 Jul 25, 2018

MaxGekk deleted the to-ddl branch August 17, 2019 13:35

Peng-Lei mentioned this pull request Jan 18, 2022

[SPARK-37931][SQL] Quote the column name if needed #35227

Closed

[SPARK-24849][SPARK-24911][SQL] Converting a value of StructType to a DDL string #21803

[SPARK-24849][SPARK-24911][SQL] Converting a value of StructType to a DDL string #21803

Uh oh!

Conversation

MaxGekk commented Jul 18, 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!

maropu commented Jul 18, 2018

Uh oh!

MaxGekk commented Jul 18, 2018

Uh oh!

SparkQA commented Jul 18, 2018

Uh oh!

SparkQA commented Jul 18, 2018

Uh oh!

maropu commented Jul 19, 2018

Uh oh!

maropu Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Jul 19, 2018

Uh oh!

MaxGekk commented Jul 19, 2018

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Jul 19, 2018

Uh oh!

MaxGekk commented Jul 19, 2018

Uh oh!

HyukjinKwon commented Jul 19, 2018

Uh oh!

MaxGekk commented Jul 19, 2018

Uh oh!

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

Uh oh!

Uh oh!

MaxGekk commented Jul 19, 2018

Uh oh!

HyukjinKwon commented Jul 19, 2018

Uh oh!

rxin commented Jul 20, 2018

Uh oh!

gatorsmile commented Jul 20, 2018

Uh oh!

MaxGekk commented Jul 20, 2018

Uh oh!

SparkQA commented Jul 21, 2018

Uh oh!

MaxGekk commented Jul 24, 2018

Uh oh!

gatorsmile commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 25, 2018

Uh oh!

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

Uh oh!

Uh oh!

gatorsmile commented Jul 25, 2018

Uh oh!

HyukjinKwon commented Jul 25, 2018

Uh oh!

gatorsmile commented Jul 25, 2018

Uh oh!

SparkQA commented Jul 25, 2018

Uh oh!

gatorsmile commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

MaxGekk commented Jul 18, 2018 •

edited

Loading

HyukjinKwon commented Jul 19, 2018 •

edited

Loading

gatorsmile commented Jul 24, 2018 •

edited

Loading

HyukjinKwon commented Jul 25, 2018 •

edited

Loading