[SPARK-49152][SQL] V2SessionCatalog should use V2Command by amaliujia · Pull Request #47660 · apache/spark

amaliujia · 2024-08-08T02:57:28Z

What changes were proposed in this pull request?

V2SessionCatalog should use V2Command when possible.

Why are the changes needed?

This is because the session catalog can be overwritten thus the overwritten's catalog should use v2 commands, otherwise the V1Command will still call hive metastore or the built-in session catalog.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests.

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

NO

amaliujia · 2024-08-08T02:57:50Z

@cloud-fan

cloud-fan · 2024-08-12T01:51:19Z

  import DataSourceV2Implicits._
  import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._

+  lazy private val hadoopConf = session.sparkContext.hadoopConfiguration


this shouldn't be lazy val, we should call SessionState.newHadoopConf

…/ResolveSessionCatalog.scala Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>

cloud-fan · 2024-08-12T07:45:15Z

thanks, merging to master!

V2SessionCatalog should use V2Command when possible. This is because the session catalog can be overwritten thus the overwritten's catalog should use v2 commands, otherwise the V1Command will still call hive metastore or the built-in session catalog. No Existing tests. NO Closes apache#47660 from amaliujia/create_table_v2. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2024-08-14T09:33:02Z

  private def qualifyLocInTableSpec(tableSpec: TableSpec): TableSpec = {
-    tableSpec.withNewLocation(tableSpec.location.map(makeQualifiedDBObjectPath(_)))
+    tableSpec.withNewLocation(tableSpec.location.map(loc => CatalogUtils.makeQualifiedPath(
+      CatalogUtils.stringToURI(loc), hadoopConf).toString))


we should follow v1 command code path and call CatalogUtils.URIToString to get the path string.

fixing at #47759

…ath string ### What changes were proposed in this pull request? This is a followup of #47660 to restore the behavior change. The table location string should be Hadoop Path string instead of URL string which escapes all special chars. ### Why are the changes needed? restore the unintentional behavior change. ### Does this PR introduce _any_ user-facing change? No, it's not released yet ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #47759 from cloud-fan/fix. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…ath string This is a followup of apache#47660 to restore the behavior change. The table location string should be Hadoop Path string instead of URL string which escapes all special chars. restore the unintentional behavior change. No, it's not released yet new test no Closes apache#47759 from cloud-fan/fix. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…oop Path string ### What changes were proposed in this pull request? This is a followup of #47660 to restore the behavior change. The table location string should be Hadoop Path string instead of URL string which escapes all special chars. ### Why are the changes needed? restore the unintentional behavior change. ### Does this PR introduce _any_ user-facing change? No, it's not released yet ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #47765 from cloud-fan/fix. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…se V1 commands ### What changes were proposed in this pull request? This is a followup of #47660 . If users override `spark_catalog` with `DelegatingCatalogExtension`, we should still use v1 commands as `DelegatingCatalogExtension` forwards requests to HMS and there are still behavior differences between v1 and v2 commands targeting HMS. This PR also forces to use v1 commands for certain commands that do not have a v2 version. ### Why are the changes needed? Avoid introducing behavior changes to Spark plugins that implements `DelegatingCatalogExtension` to override `spark_catalog`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test case ### Was this patch authored or co-authored using generative AI tooling? No Closes #47995 from amaliujia/fix_catalog_v2. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Rui Wang <rui.wang@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…se V1 commands ### What changes were proposed in this pull request? This is a followup of #47660 . If users override `spark_catalog` with `DelegatingCatalogExtension`, we should still use v1 commands as `DelegatingCatalogExtension` forwards requests to HMS and there are still behavior differences between v1 and v2 commands targeting HMS. This PR also forces to use v1 commands for certain commands that do not have a v2 version. ### Why are the changes needed? Avoid introducing behavior changes to Spark plugins that implements `DelegatingCatalogExtension` to override `spark_catalog`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test case ### Was this patch authored or co-authored using generative AI tooling? No Closes #47995 from amaliujia/fix_catalog_v2. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Rui Wang <rui.wang@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit f7cfeb5) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…oop Path string ### What changes were proposed in this pull request? This is a followup of #47660 to restore the behavior change. The table location string should be Hadoop Path string instead of URL string which escapes all special chars. ### Why are the changes needed? restore the unintentional behavior change. ### Does this PR introduce _any_ user-facing change? No, it's not released yet ### How was this patch tested? new test ### Was this patch authored or co-authored using generative AI tooling? no Closes #47765 from cloud-fan/fix. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…se V1 commands ### What changes were proposed in this pull request? This is a followup of #47660 . If users override `spark_catalog` with `DelegatingCatalogExtension`, we should still use v1 commands as `DelegatingCatalogExtension` forwards requests to HMS and there are still behavior differences between v1 and v2 commands targeting HMS. This PR also forces to use v1 commands for certain commands that do not have a v2 version. ### Why are the changes needed? Avoid introducing behavior changes to Spark plugins that implements `DelegatingCatalogExtension` to override `spark_catalog`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? new test case ### Was this patch authored or co-authored using generative AI tooling? No Closes #47995 from amaliujia/fix_catalog_v2. Lead-authored-by: Wenchen Fan <wenchen@databricks.com> Co-authored-by: Rui Wang <rui.wang@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit f7cfeb5) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

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

cloud-fan reviewed Aug 8, 2024

View reviewed changes

Comment thread sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala Outdated

amaliujia changed the title ~~[SPARK-49152][SQL] V2SessionCatalog should use V2Command when possible~~ [SPARK-49152][SQL] V2SessionCatalog should use V2Command Aug 8, 2024

amaliujia force-pushed the create_table_v2 branch from e85a419 to 69614d1 Compare August 9, 2024 04:49