[SPARK-49183][SQL] V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #47684

cloud-fan · 2024-08-09T09:45:22Z

What changes were proposed in this pull request?

Even if the table definition has a location, the table should be a managed table if PROP_IS_MANAGED_LOCATION is specified, in V2SessionCatalog.createTable.

Why are the changes needed?

It's a bug fix. A custom spark_catalog may generate custom location for managed table and delegate the actual table creation to V2SessionCatalog. The table should still be a managed table if PROP_IS_MANAGED_LOCATION is specified.

Does this PR introduce any user-facing change?

Yes, now users who use custom spark_catalog that generates custom location for managed table, can correctly create managed tables.

How was this patch tested?

a new test

Was this patch authored or co-authored using generative AI tooling?

no

cloud-fan · 2024-08-09T09:45:55Z

@yaooqinn @amaliujia

yaooqinn · 2024-08-09T09:54:10Z

Do we need to update the API doc to make it more precise?

 /**
   * A reserved property to indicate that the table location is managed, not user-specified.
   * If this property is "true", SHOW CREATE TABLE will not generate the LOCATION clause.
   */

yaooqinn · 2024-08-09T09:54:53Z

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

@@ -170,7 +170,8 @@ class V2SessionCatalog(catalog: SessionCatalog)
    val storage = DataSource.buildStorageFormatFromOptions(toOptions(tableProperties))
      .copy(locationUri = location.map(CatalogUtils.stringToURI))
    val isExternal = properties.containsKey(TableCatalog.PROP_EXTERNAL)
-    val tableType = if (isExternal || location.isDefined) {
+    val isManagedLocation = properties.containsKey(TableCatalog.PROP_IS_MANAGED_LOCATION)


do we need to test if the value is true or not?

I followed how PROP_EXTERNAL is handled.

cloud-fan · 2024-08-09T14:13:40Z

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

@@ -120,7 +120,7 @@ case class ShowCreateTableExec(
  private def showTableLocation(table: Table, builder: StringBuilder): Unit = {
    val isManagedOption = Option(table.properties.get(TableCatalog.PROP_IS_MANAGED_LOCATION))
    // Only generate LOCATION clause if it's not managed.
-    if (isManagedOption.forall(_.equalsIgnoreCase("false"))) {
+    if (isManagedOption.isEmpty || !isManagedOption.get.equalsIgnoreCase("true")) {


A small fix to match the document. true means effective, non true means not effective.

amaliujia · 2024-08-09T17:44:07Z

sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java

@@ -52,7 +52,8 @@ public interface TableCatalog extends CatalogPlugin {

  /**
   * A reserved property to indicate that the table location is managed, not user-specified.
-   * If this property is "true", SHOW CREATE TABLE will not generate the LOCATION clause.
+   * If this property is "true", it means it's a managed table even if it has a location. As an
+   * example, SHOW CREATE TABLE will not generate the LOCATION clause.


Do we need to also document non true case?

Is non true value invalid, or it is valid but ignored, etc.?

The non-true case is hard to document, as treating tables with location as external tables is a Hive-specific behavior.

amaliujia · 2024-08-09T17:45:21Z

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

+        sql(s"CREATE TABLE t (i INT) USING $v2Format")
+        val table = catalog(SESSION_CATALOG_NAME).asTableCatalog
+          .loadTable(Identifier.of(Array("default"), "t"))
+        assert(!table.properties().containsKey(TableCatalog.PROP_EXTERNAL))


do we need to also test SHOW CREATE TABLE or DESC TABLE that this is a managed table without a location?

v2 table property is already the source of truth.

amaliujia

LGTM

cloud-fan · 2024-08-12T03:18:55Z

thanks for the review, merging to master/3.5!

…S_MANAGED_LOCATION Even if the table definition has a location, the table should be a managed table if `PROP_IS_MANAGED_LOCATION` is specified, in `V2SessionCatalog.createTable`. It's a bug fix. A custom `spark_catalog` may generate custom location for managed table and delegate the actual table creation to `V2SessionCatalog`. The table should still be a managed table if `PROP_IS_MANAGED_LOCATION` is specified. Yes, now users who use custom `spark_catalog` that generates custom location for managed table, can correctly create managed tables. a new test no Closes #47684 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit ed04e16) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

ulysses-you · 2024-08-12T11:42:31Z

it seems the branch-3.5 CI failed..

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala:3401:24: type Map is not a member of package org.apache.spark.sql.connector.util
[error]       properties: util.Map[String, String]): Table = {
[error]                        ^
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala:3402:29: type HashMap is not a member of package org.apache.spark.sql.connector.util
[error]     val newProps = new util.HashMap[String, String]
[error]                             ^

cloud-fan · 2024-08-12T15:03:21Z

fixing at #47722

### What changes were proposed in this pull request? fix compilation error caused by #47684 . Seems scala 2.12 has some subtle differences in the import priority. ### Why are the changes needed? N/A ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? pass compilation ### Was this patch authored or co-authored using generative AI tooling? no Closes #47722 from cloud-fan/fix. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…S_MANAGED_LOCATION ### What changes were proposed in this pull request? Even if the table definition has a location, the table should be a managed table if `PROP_IS_MANAGED_LOCATION` is specified, in `V2SessionCatalog.createTable`. ### Why are the changes needed? It's a bug fix. A custom `spark_catalog` may generate custom location for managed table and delegate the actual table creation to `V2SessionCatalog`. The table should still be a managed table if `PROP_IS_MANAGED_LOCATION` is specified. ### Does this PR introduce _any_ user-facing change? Yes, now users who use custom `spark_catalog` that generates custom location for managed table, can correctly create managed tables. ### How was this patch tested? a new test ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#47684 from cloud-fan/table. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION

ea051dc

github-actions bot added the SQL label Aug 9, 2024

yaooqinn reviewed Aug 9, 2024

View reviewed changes

address comments

160211c

cloud-fan commented Aug 9, 2024

View reviewed changes

yaooqinn approved these changes Aug 9, 2024

View reviewed changes

amaliujia reviewed Aug 9, 2024

View reviewed changes

dongjoon-hyun approved these changes Aug 9, 2024

View reviewed changes

HyukjinKwon approved these changes Aug 11, 2024

View reviewed changes

amaliujia approved these changes Aug 12, 2024

View reviewed changes

cloud-fan closed this in ed04e16 Aug 12, 2024

cloud-fan mentioned this pull request Aug 12, 2024

[SPARK-49183][SQL][FOLLOWUP][3.5] Fix compilation #47722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-49183][SQL] V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #47684

[SPARK-49183][SQL] V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #47684

cloud-fan commented Aug 9, 2024

cloud-fan commented Aug 9, 2024

yaooqinn commented Aug 9, 2024

yaooqinn Aug 9, 2024 •

edited

Loading

cloud-fan Aug 9, 2024

cloud-fan Aug 9, 2024

amaliujia Aug 9, 2024

cloud-fan Aug 12, 2024

amaliujia Aug 9, 2024

cloud-fan Aug 12, 2024 •

edited

Loading

amaliujia left a comment

cloud-fan commented Aug 12, 2024

ulysses-you commented Aug 12, 2024

cloud-fan commented Aug 12, 2024

[SPARK-49183][SQL] V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #47684

[SPARK-49183][SQL] V2SessionCatalog.createTable should respect PROP_IS_MANAGED_LOCATION #47684

Conversation

cloud-fan commented Aug 9, 2024

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?

Was this patch authored or co-authored using generative AI tooling?

cloud-fan commented Aug 9, 2024

yaooqinn commented Aug 9, 2024

yaooqinn Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

cloud-fan Aug 9, 2024

Choose a reason for hiding this comment

cloud-fan Aug 9, 2024

Choose a reason for hiding this comment

amaliujia Aug 9, 2024

Choose a reason for hiding this comment

cloud-fan Aug 12, 2024

Choose a reason for hiding this comment

amaliujia Aug 9, 2024

Choose a reason for hiding this comment

cloud-fan Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

amaliujia left a comment

Choose a reason for hiding this comment

cloud-fan commented Aug 12, 2024

ulysses-you commented Aug 12, 2024

cloud-fan commented Aug 12, 2024

yaooqinn Aug 9, 2024 •

edited

Loading

cloud-fan Aug 12, 2024 •

edited

Loading