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

[SPARK-37878][SQL] Migrate SHOW CREATE TABLE to use v2 command by default #35204

Closed

Peng-Lei wants to merge 1 commit into apache:master from Peng-Lei:SPARK-37878

Contributor

Peng-Lei commented Jan 14, 2022 •

edited

Loading

What changes were proposed in this pull request?

Add quoted(identifier: TableIdentifier) to quoted the table name of V1 command(SHOW CREATE TABLE[AS SERDE]) to match V2 behavior. It just work when quoteIfNeeded
Change addV2TableProperties of V1Table. Just when external == true, we will add location property.
Change V1Table.Schema, re-construct the original schema from the string.
Use V2 command as default for SHOW CRATE TABLE
Change V2 behavior ShowTablePartitions to match V1 behavior.

Why are the changes needed?

It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands.

Does this PR introduce any user-facing change?

use V2 command as default for show create table
if LEGACY_USE_V1_COMMAND == true
will use V1 command

How was this patch tested?

build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *ShowCreateTableSuite"

github-actions bot added the SQL label

Peng-Lei force-pushed the SPARK-37878 branch from b779f03 to 82cd6c8 Compare

January 14, 2022 04:05

Contributor Author

Peng-Lei commented Jan 14, 2022

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

cloud-fan reviewed

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/V1Table.scala Outdated

    
            @@ -58,7 +59,7 @@ private[sql] case class V1Table(v1Table: CatalogTable) extends Table {
          
                override lazy val properties: util.Map[String, String] = addV2TableProperties(v1Table).asJava

                override lazy val schema: StructType = v1Table.schema

                override lazy val schema: StructType = CharVarcharUtils.getRawSchema(v1Table.schema, SQLConf.get)

Contributor

cloud-fan Jan 14, 2022

why is this change?

Contributor Author

Peng-Lei Jan 14, 2022

when v2 command work with v1 table. when we resolve the table. we load the table by V2SessionCatalog, then create V1Table(sessionCatalog.getTableMetadata), here when getTableMetadata, we will replace char/varchar with string in schema. So V1Table(t) t is alway with string type although type is char/varchar. So re-construct the schema of V1Table.

Contributor Author

Peng-Lei Jan 14, 2022

maybe we can remove #L496

Contributor

cloud-fan Jan 17, 2022

I think we should restore the raw table schema in SHOW CREATE TABLE, not here.

Contributor Author

Peng-Lei Jan 17, 2022

other v2 command works v1 table. maybe encounter this problem. but I will update as you said. thank you very much.

Contributor Author

Peng-Lei Jan 17, 2022

done.

cloud-fan reviewed

View reviewed changes

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

    
            @@ -1054,6 +1054,15 @@ trait ShowCreateTableCommandBase {
          
                private def showViewText(metadata: CatalogTable, builder: StringBuilder): Unit = {

                  builder ++= metadata.viewText.mkString("AS ", "", "\n")

                }

                def quoted(identifier: TableIdentifier): String = {

Contributor

cloud-fan Jan 14, 2022

is it simply identifier.asMultiPartIdentifier.quoted?

Contributor Author

Peng-Lei Jan 14, 2022

TableIdentifier could not asMultiPartIdentifier, Identifier can asMultiPartIdentifier

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala Show resolved Hide resolved

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala Outdated

    
                    table.partitioning.foreach(t => transforms += t.describe())

                    builder ++= s"PARTITIONED BY ${transforms.mkString("(", ", ", ")")}\n"

                  table match {

                    case V1Table(t) =>

Contributor

cloud-fan Jan 14, 2022

We don't need to special-case v1 table. We should recognize these special partition transforms (bucket and sorted_bucket) and generate the CLUSTERED BY syntax for them.

@huaxingao have we unified this part? We discussed this before and the parser should generate partition transforms for bucket spec already, instead of BucketSpec

Contributor

huaxingao Jan 14, 2022

Haven't changed this yet. Remember that
c.partitioning ++ c.tableSpec.bucketSpec.map(_.asTransform) didn't work inside AstBuilder because sortColumnNames needed to be empty. Now it should work. I can have a follow up to fix this.

Peng-Lei force-pushed the SPARK-37878 branch 3 times, most recently from 643673e to 3b8bde2 Compare

January 14, 2022 12:19

Peng-Lei requested a review from cloud-fan

January 14, 2022 12:23

Peng-Lei force-pushed the SPARK-37878 branch 3 times, most recently from 55fd5e9 to 7b52ca9 Compare

January 14, 2022 16:50

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala Outdated

    
                    // compatible with v1

                    bucketSpec.map { bucket =>

                      if (bucket.bucketColumnNames.nonEmpty) {

Contributor

cloud-fan Jan 17, 2022

nit: we can turn this if into an assert, as this can never happen

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala Outdated

    
                  val showProps = table.properties.asScala

                    .filterKeys(key => !CatalogV2Util.TABLE_RESERVED_PROPERTIES.contains(key)

                      && !key.startsWith(TableCatalog.OPTION_PREFIX)

                      && !tableOptions.contains(key))

                      && !tableOptions.contains(key)

                      && !key.equals(TableCatalog.PROP_EXTERNAL))

Contributor

cloud-fan Jan 17, 2022 •

edited

Loading

isn't it convered by TABLE_RESERVED_PROPERTIES?

Contributor Author

Peng-Lei Jan 17, 2022

As PROP_EXTERNA is not include by TABLE_RESERVED_PROPERTIES. do you think it should be included by TABLE_RESERVED_PROPERTIES ?

Contributor

cloud-fan Jan 17, 2022

yea it should, it's indeed a reserved property

Contributor Author

Peng-Lei Jan 17, 2022

done.

Peng-Lei force-pushed the SPARK-37878 branch from 7b52ca9 to 402c82c Compare

January 17, 2022 08:14

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala Outdated

@@ @@ -57,7 +60,7 @@ case class ShowCreateTableExec( @@
                 }
                 private def showTableDataColumns(table: Table, builder: StringBuilder): Unit = {
-                  val columns = table.schema().fields.map(_.toDDL)
+                  val columns = CharVarcharUtils.getRawSchema(table.schema(), SQLConf.get).fields.map(_.toDDL)

Contributor

cloud-fan Jan 17, 2022

Suggested change

      
                val columns = CharVarcharUtils.getRawSchema(table.schema(), SQLConf.get).fields.map(_.toDDL)
          
                val columns = CharVarcharUtils.getRawSchema(table.schema(), conf).fields.map(_.toDDL)

We are within a physical plan, we can access conf here.

cloud-fan reviewed

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ShowCreateTableExec.scala Outdated

@@ @@ -71,8 +74,9 @@ case class ShowCreateTableExec( @@
                     builder: StringBuilder,
                     tableOptions: Map[String, String]): Unit = {
                   if (tableOptions.nonEmpty) {
-                    val props = tableOptions.toSeq.sortBy(_._1).map { case (key, value) =>
-                      s"'${escapeSingleQuotedString(key)}' = '${escapeSingleQuotedString(value)}'"
+                    val props = SQLConf.get.redactOptions(tableOptions).toSeq.sortBy(_._1).map {

Contributor

cloud-fan Jan 17, 2022

ditto

cloud-fan reviewed

View reviewed changes

sql/core/src/test/resources/sql-tests/results/show-create-table.sql.out

    
            @@ -257,7 +257,7 @@ SHOW CREATE TABLE tbl
          
              -- !query schema

              struct<createtab_stmt:string>

              -- !query output

              CREATE TABLE `default`.`tbl` (

              CREATE TABLE default.tbl (

                `a` FLOAT,

Contributor

cloud-fan Jan 17, 2022

to be consistent, shall we also only quote the column name if needed? This is can be done in a new PR, as it's not related to v2 command migration.

Contributor Author

Peng-Lei Jan 17, 2022

ok. I will do it.

cloud-fan approved these changes

View reviewed changes

Peng-Lei force-pushed the SPARK-37878 branch from 402c82c to 49c8672 Compare

January 17, 2022 08:35

Peng-Lei mentioned this pull request

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

Closed

Peng-Lei force-pushed the SPARK-37878 branch from 49c8672 to 317ec09 Compare

January 17, 2022 11:29

github-actions bot added the DOCS label

Contributor Author

Peng-Lei commented Jan 17, 2022

@cloud-fan Update the PR. As the testcase failed #testcase. Maybe "Add the external to TABLE_RESERVED_PROPERTIES" in separate PR is more suitable?

Contributor

cloud-fan commented Jan 17, 2022

do you know why the test failed?

Contributor Author

Peng-Lei commented Jan 17, 2022 •

edited

Loading

do you know why the test failed?

without filterNot(_ == PROP_EXTERNAL)

Contributor

cloud-fan commented Jan 17, 2022

@Peng-Lei can you be more specific? What's the expectation of the tests and how do we break it?

I see that we exclude COMMENT there, as we do want to allow users to specify the COMMENT property directly. Do we have the same expectation for EXTERNAL? If yes, where is the code?

Contributor Author

Peng-Lei commented Jan 17, 2022

@Peng-Lei can you be more specific? What's the expectation of the tests and how do we break it?

I see that we exclude COMMENT there, as we do want to allow users to specify the COMMENT property directly. Do we have the same expectation for EXTERNAL? If yes, where is the code?

expectation of the tests：

if LEGACY_PROPERTY_NON_RESERVED == false. That means we reserved table properties. so we could not use key of reserved properties for create/replace table options/tblproperties ... and set/unset tblproperties. so result is throw a ParseException.
if LEGACY_PROPERTY_NON_RESERVED == true. That means we can use reserved key for create/replace table options/tblproperties ... and set/unset tblproperties. but have no effect.

At first I was really confused why filter COMMENT and plan to do a new PR to fix it so that COMMENT and EXTERNAL are same with other reserved properties key. so modify the code of ASTBuilder.scala first. anthor reason I think EXTERNAL is a little special, as is should be consistent with whether the location is defined. But at now, I am wrong. Sorry. I should look more code to keep the same expectation with COMMENT. @cloud-fan

Contributor

cloud-fan commented Jan 17, 2022

@Peng-Lei We should update AstBuilder.cleanTableProperties, to make EXTERNAL a truly reserved property. Let's open a new PR for it as it's a breaking change.

Peng-Lei force-pushed the SPARK-37878 branch from 317ec09 to 6e8ff32 Compare

January 18, 2022 01:44


          add draft

7380a51

Peng-Lei force-pushed the SPARK-37878 branch from 6e8ff32 to 7380a51 Compare

January 18, 2022 01:50

Contributor Author

Peng-Lei commented Jan 18, 2022

@Peng-Lei We should update AstBuilder.cleanTableProperties, to make EXTERNAL a truly reserved property. Let's open a new PR for it as it's a breaking change.

The issue is created. I'm so sorry for spending a lot of your time on review this PR. Thank you very much for your patient code review.

cloud-fan approved these changes

View reviewed changes

Contributor

cloud-fan commented Jan 18, 2022

thanks, merging to master!

cloud-fan closed this in

2c825d1

cloud-fan pushed a commit that referenced this pull request


          [SPARK-37931][SQL] Quote the column name if neededQuote the column na…

15464e3

…me if needed

### What changes were proposed in this pull request?
Quote the column name just needed instead of anyway.

### Why are the changes needed?
[#comments](#35204 (comment))

### 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
```
2. table schema
    df.schema or show create table
3. call `toDDL` for StructType or StructField.

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

Closes #35227 from Peng-Lei/Quote-Column.

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

Peng-Lei mentioned this pull request

[SPARK-37950][SQL] Take EXTERNAL as a reserved table property #35268

Closed

cloud-fan pushed a commit that referenced this pull request


          [SPARK-37950][SQL] Take EXTERNAL as a reserved table property

8ff48b3

### What changes were proposed in this pull request?

Take `external` as a reserved table property. and do not allow use `external` for end-user when `spark.sql.legacy.notReserveProperties` == `false`.

### Why are the changes needed?

[#disscuss](#35204 (comment)).
keep it consistent with other properties like `location` `owner` `provider` and so on.

### Does this PR introduce _any_ user-facing change?
Yes. end-user could not use `external` as property key when create table with tblproperties and alter table set tblproperties.

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

Closes #35268 from Peng-Lei/SPARK-37950.

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

cloud-fan mentioned this pull request

[SPARK-37878][SQL][FOLLOWUP] V1Table should always carry the "location" property #36498

Closed

cloud-fan added a commit that referenced this pull request


          [SPARK-37878][SQL][FOLLOWUP] V1Table should always carry the "locatio…

fa2bda5

…n" property

### What changes were proposed in this pull request?

This is a followup of #35204 . #35204 introduced a potential regression: it removes the "location" table property from `V1Table` if the table is not external. The intention was to avoid putting the LOCATION clause for managed tables in `ShowCreateTableExec`. However, if we use the v2 DESCRIBE TABLE command by default in the future, this will bring a behavior change and v2 DESCRIBE TABLE command won't print the table location for managed tables.

This PR fixes this regression by using a different idea to fix the SHOW CREATE TABLE issue:
1. introduce a new reserved table property `is_managed_location`, to indicate that the location is managed by the catalog, not user given.
2. `ShowCreateTableExec` only generates the LOCATION clause if the "location" property is present and is not managed.

### Why are the changes needed?

avoid a potential regression

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

existing tests. We can add a test when we use v2 DESCRIBE TABLE command by default.

Closes #36498 from cloud-fan/regression.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan added a commit that referenced this pull request


          [SPARK-37878][SQL][FOLLOWUP] V1Table should always carry the "locatio…

8608baa

…n" property

### What changes were proposed in this pull request?

This is a followup of #35204 . #35204 introduced a potential regression: it removes the "location" table property from `V1Table` if the table is not external. The intention was to avoid putting the LOCATION clause for managed tables in `ShowCreateTableExec`. However, if we use the v2 DESCRIBE TABLE command by default in the future, this will bring a behavior change and v2 DESCRIBE TABLE command won't print the table location for managed tables.

This PR fixes this regression by using a different idea to fix the SHOW CREATE TABLE issue:
1. introduce a new reserved table property `is_managed_location`, to indicate that the location is managed by the catalog, not user given.
2. `ShowCreateTableExec` only generates the LOCATION clause if the "location" property is present and is not managed.

### Why are the changes needed?

avoid a potential regression

### Does this PR introduce _any_ user-facing change?

no

### How was this patch tested?

existing tests. We can add a test when we use v2 DESCRIBE TABLE command by default.

Closes #36498 from cloud-fan/regression.

Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit fa2bda5)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment