Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Peng-Lei wants to merge 1 commit into from
Closed

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

Peng-Lei wants to merge 1 commit into from

Conversation

@Peng-Lei
Copy link
Contributor

@Peng-Lei Peng-Lei commented Jan 17, 2022 •
edited by cloud-fan
Loading

What changes were proposed in this pull request?

Quote the column name if needed instead of always.

Why are the changes needed?

#comments

Does this PR introduce any user-facing change?

Yes，It will change the result that users get the schema
eg:

"STRUCT<`_c0`: STRING, `_c1`: INT>"  => "STRUCT<_c0: STRING, _c1: INT>"

At now. for end-user. I learn about the 3 way to get schema directly

  1. the function: eg
schema_of_json
schema_of_csv
  1. table schema
    df.schema or show create table
  2. call toDDL for StructType or StructField.

How was this patch tested?

existed testcase.

@github-actions github-actions bot added the SQL label Jan 17, 2022
@Peng-Lei Peng-Lei force-pushed the Quote-Column branch 2 times, most recently from 4ad25a8 to ec172e4 Compare January 18, 2022 03:09
@Peng-Lei
Copy link
Contributor Author

Peng-Lei commented Jan 18, 2022

@cloud-fan Could you take a look ? Thank you very much.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 18, 2022 •
edited
Loading

Yes， when end-user get the table metadata， if the columns does not need quote. it will be a normal string.

Let's be specific here. This only affects schema DDL string AFAIK, while "get the table metadata" is low-level operation and it's scarying to change it.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 18, 2022

looks fine and I can't think of any breaking cases. cc @HyukjinKwon @viirya as well

@Peng-Lei
Copy link
Contributor Author

Peng-Lei commented Jan 18, 2022

Let's be specific here. This only affects schema DDL string AFAIK, while "get the table metadata" is low-level operation and it's scarying to change it.

I found that the toDDL first introduce by #21803. and also for show create table at the beginning.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jan 20, 2022

thanks, merging to master!

@cloud-fan cloud-fan closed this in 15464e3 Jan 20, 2022
@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jan 20, 2022

Yeah, I think this is fine.

@cloud-fan cloud-fan changed the title [SPARK-37931][SQL] Quote the column name if neededQuote the column name if needed [SPARK-37931][SQL] Quote the column name if needed Jan 20, 2022
cloud-fan
cloud-fan reviewed Jan 27, 2022
View reviewed changes
sql/core/src/test/resources/sql-tests/results/charvarchar.sql.out
`c` CHAR(5),
`v` VARCHAR(6))
c CHAR(5),
v VARCHAR(6))
Copy link
Contributor

@cloud-fan cloud-fan Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the view test in this file still has quotes. ShowCreateTableCommand has a branch for view, and we need to update the quoting logic in that branch as well. @Peng-Lei

Copy link
Contributor Author

@Peng-Lei Peng-Lei Jan 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I left this out. I will do it by a follow up.

@Peng-Lei Peng-Lei mentioned this pull request Jan 28, 2022
Closed
cloud-fan pushed a commit that referenced this pull request Jan 28, 2022
@Peng-Lei @cloud-fan
[SPARK-37931][SQL][FOLLOWUP] Quote the column name of view if needed
d150b7e 
### What changes were proposed in this pull request?
Quote the column name of view when `SHOW CREATE TABLE` for view.

### Why are the changes needed?
follow up the [#PR](#35227). Keep the consistent between table and view when `SHOW CREATE TABLE`.

### Does this PR introduce _any_ user-facing change?
Yes，It will change the result of `SHOW CREATE TABLE` for view.
eg:
```
"STRUCT<`_c0`, `_c1`>"  => "STRUCT<_c0, _c1>"
```

### How was this patch tested?
existed testcase.

Closes #35351 from Peng-Lei/view-quote-columns.

Authored-by: PengLei <peng.8lei@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
senthh pushed a commit to senthh/spark-1 that referenced this pull request Feb 3, 2022
@Peng-Lei @senthh
[SPARK-37931][SQL][FOLLOWUP] Quote the column name of view if needed
5705d2a 
### What changes were proposed in this pull request?
Quote the column name of view when `SHOW CREATE TABLE` for view.

### Why are the changes needed?
follow up the [#PR](apache#35227). Keep the consistent between table and view when `SHOW CREATE TABLE`.

### Does this PR introduce _any_ user-facing change?
Yes，It will change the result of `SHOW CREATE TABLE` for view.
eg:
```
"STRUCT<`_c0`, `_c1`>"  => "STRUCT<_c0, _c1>"
```

### How was this patch tested?
existed testcase.

Closes apache#35351 from Peng-Lei/view-quote-columns.

Authored-by: PengLei <peng.8lei@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@gatorsmile
Copy link
Member

gatorsmile commented Feb 8, 2022

This is a user-facing breaking change. The third party libraries (esp. SQL clients) might rely on the current format. We should avoid making such a change.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Feb 9, 2022

Hm, I think it's less likely that this breaks something because it removes the backquotes when they are unnecessary, meaning that it's still able to read and parse it back with lower versions of Spark.

@cloud-fan
Copy link
Contributor

cloud-fan commented Feb 9, 2022

I think we should clearly document the display-like commands. Their output is not structured and we only expect humans to read it, not programs.

@yikf yikf mentioned this pull request Sep 18, 2022
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan left review comments

Assignees

No one assigned

Labels

CORE PYTHON R SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Peng-Lei @cloud-fan @HyukjinKwon @gatorsmile