[SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table #35462

bozhang2820 · 2022-02-09T13:00:41Z

What changes were proposed in this pull request?

After #28527, we change to create table under the database location when the table location is relative. However the criteria to determine if a table location is relative/absolute is URI.isAbsolute, which basically checks if the table location URI has a scheme defined. So table URIs like /table/path are treated as relative and the scheme and authority of the database location URI are used to create the table. For example, when the database location URI is s3a://bucket/db, the table will be created at s3a://bucket/table/path, while it should be created under the file system defined in SessionCatalog.hadoopConf instead.

This change fixes that by treating table location as absolute when the first letter of its path is slash.

This also applies to alter table.

Why are the changes needed?

This is to fix the behavior described above.

Does this PR introduce any user-facing change?

Yes. When users try to create/alter a table with a location that starts with a slash but without a scheme defined, the table will be created under/altered to the file system defined in SessionCatalog.hadoopConf, instead of the one defined in the database location URI.

How was this patch tested?

Updated unit tests.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

zsxwing · 2022-02-11T08:03:50Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

@@ -388,6 +388,8 @@ class SessionCatalog(
  private def makeQualifiedTablePath(locationUri: URI, database: String): URI = {
    if (locationUri.isAbsolute) {
      locationUri
+    } else if (locationUri.getPath.startsWith("/")) {


Use new Path(locationUri).isAbsolute?

Thanks! Will change to follow your suggestion.

bozhang2820 · 2022-02-16T07:12:34Z

CC @yaooqinn @cloud-fan for a review.

cloud-fan · 2022-02-16T14:12:03Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala

@@ -53,15 +53,11 @@ import org.apache.spark.sql.types._
 import org.apache.spark.sql.v2.avro.AvroScan
 import org.apache.spark.util.Utils

-abstract class AvroSuite
+abstract class AvroSuiteBase


why do we need to touch this suite? The fix is for catalog stuff and is not related to the file source.

After running the test in CommonFileDataSourceSuite, org.apache.hadoop.fs.Path.getFileSystem() in other tests in the same suite will fail the check in FakeFileSystemRequiringDSOption, since they share the same SparkSession.

That's why I split the suites that extends CommonFileDataSourceSuite into separate ones in this change.

is it because org.apache.hadoop.fs.Path.getFileSystem() has cache?

It's because SparkContext.hadoopConfiguration is passed to org.apache.hadoop.fs.Path.getFileSystem() and that is shared across tests in the same suite.

but certain test case can reset the hadoop configs it sets to avoid polluting the shared environment, right?

The test in CommonFileDataSourceSuite actually uses withSQLConf which does reset the config. However SessionCatalog.hadoopConf are set and could not be changed unless we create a new SessionCatalog

cloud-fan

The fix LGTM, just one question for the test changes.

dongjoon-hyun

Hi, @bozhang2820 .
In general, FOLLOWUP-style PR are more proper when the original PR and the follow-up PR is in the same release. SPARK-31709 was released long time ago at 3.1.0 (Actually, on March 2, 2021, as 3.1.1). In addition, your contribution is worth of tracking by a separate JIRA issue instead of a follow-up style.

bozhang2820 · 2022-02-17T08:28:18Z

Thanks @dongjoon-hyun for the guidance. Created https://issues.apache.org/jira/browse/SPARK-38236 and updated the title.

This reverts commit 65b7b18.

This reverts commit 67c2318.

cloud-fan · 2022-02-18T15:19:53Z

external/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala

@@ -68,6 +68,8 @@ abstract class AvroSuite

  override protected def beforeAll(): Unit = {
    super.beforeAll()
+    // initialize SessionCatalog here so it has a clean hadoopConf
+    spark.sessionState.catalog


shall we do it in SharedSparkSessionBase.initializeSession?

I thought about it but figured that might need a separate PR due to the impact?

cloud-fan · 2022-02-21T03:30:26Z

thanks, merging to master!

cloud-fan · 2022-02-21T03:31:35Z

@bozhang2820 can you help to create backport PRs for 3.2 and 3.1?

…tter of its path is slash in create/alter table After apache#28527, we change to create table under the database location when the table location is relative. However the criteria to determine if a table location is relative/absolute is `URI.isAbsolute`, which basically checks if the table location URI has a scheme defined. So table URIs like `/table/path` are treated as relative and the scheme and authority of the database location URI are used to create the table. For example, when the database location URI is `s3a://bucket/db`, the table will be created at `s3a://bucket/table/path`, while it should be created under the file system defined in `SessionCatalog.hadoopConf` instead. This change fixes that by treating table location as absolute when the first letter of its path is slash. This also applies to alter table. This is to fix the behavior described above. Yes. When users try to create/alter a table with a location that starts with a slash but without a scheme defined, the table will be created under/altered to the file system defined in `SessionCatalog.hadoopConf`, instead of the one defined in the database location URI. Updated unit tests. Closes apache#35462 from bozhang2820/spark-31709. Authored-by: Bo Zhang <bo.zhang@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

bozhang2820 · 2022-02-21T05:17:28Z

Created #35591 for 3.1 and 3.2.

dongjoon-hyun · 2022-02-21T22:40:35Z

+1, late LGTM.

HyukjinKwon

LGTM2

Determine if table path is absolute by checking its first letter

a743482

github-actions bot added the SQL label Feb 9, 2022

github-actions bot added AVRO ML MLLIB labels Feb 10, 2022

gatorsmile reviewed Feb 10, 2022

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala Show resolved Hide resolved

Split out CommonFileDataSourceSuite

67c2318

bozhang2820 force-pushed the spark-31709 branch from c514c47 to 67c2318 Compare February 11, 2022 06:00

zsxwing reviewed Feb 11, 2022

View reviewed changes

bozhang2820 added 2 commits February 14, 2022 13:53

Address comments

afaabe8

Fix tests

65b7b18

cloud-fan reviewed Feb 16, 2022

View reviewed changes

cloud-fan approved these changes Feb 16, 2022

View reviewed changes

dongjoon-hyun requested changes Feb 17, 2022

View reviewed changes

bozhang2820 changed the title ~~[SPARK-31709][SQL][FOLLOWUP] Treat table location as absolute when the first letter of its path is slash in create/alter table~~ [SPARK-38236][SQL] Treat table location as absolute when the first letter of its path is slash in create/alter table Feb 17, 2022

yaooqinn approved these changes Feb 17, 2022

View reviewed changes

bozhang2820 added 3 commits February 18, 2022 22:54

Revert "Fix tests"

654b953

This reverts commit 65b7b18.

Revert "Split out CommonFileDataSourceSuite"

6782a23

This reverts commit 67c2318.

Fix tests by initialize SessionCatalog

cd16730

cloud-fan reviewed Feb 18, 2022

View reviewed changes

cloud-fan approved these changes Feb 21, 2022

View reviewed changes

cloud-fan closed this in b71b917 Feb 21, 2022

HyukjinKwon reviewed Feb 22, 2022

View reviewed changes

bozhang2820 deleted the spark-31709 branch February 22, 2022 03:14

bozhang2820 changed the title ~~[SPARK-38236][SQL] Treat table location as absolute when the first letter of its path is slash in create/alter table~~ [SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table #35462

[SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table #35462

bozhang2820 commented Feb 9, 2022 •

edited

Loading

zsxwing Feb 11, 2022

bozhang2820 Feb 14, 2022

bozhang2820 commented Feb 16, 2022

cloud-fan Feb 16, 2022

bozhang2820 Feb 17, 2022 •

edited

Loading

cloud-fan Feb 17, 2022

bozhang2820 Feb 17, 2022

cloud-fan Feb 17, 2022

bozhang2820 Feb 17, 2022

cloud-fan left a comment

dongjoon-hyun left a comment •

edited

Loading

bozhang2820 commented Feb 17, 2022

cloud-fan Feb 18, 2022

bozhang2820 Feb 19, 2022

cloud-fan commented Feb 21, 2022 •

edited

Loading

cloud-fan commented Feb 21, 2022

bozhang2820 commented Feb 21, 2022 •

edited

Loading

dongjoon-hyun commented Feb 21, 2022

HyukjinKwon left a comment

[SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table #35462

[SPARK-38236][SQL] Check if table location is absolute by "new Path(locationUri).isAbsolute" in create/alter table #35462

Conversation

bozhang2820 commented Feb 9, 2022 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozhang2820 commented Feb 16, 2022

Choose a reason for hiding this comment

bozhang2820 Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment • edited Loading

Choose a reason for hiding this comment

bozhang2820 commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Feb 21, 2022 • edited Loading

cloud-fan commented Feb 21, 2022

bozhang2820 commented Feb 21, 2022 • edited Loading

dongjoon-hyun commented Feb 21, 2022

HyukjinKwon left a comment

Choose a reason for hiding this comment

bozhang2820 commented Feb 9, 2022 •

edited

Loading

bozhang2820 Feb 17, 2022 •

edited

Loading

dongjoon-hyun left a comment •

edited

Loading

cloud-fan commented Feb 21, 2022 •

edited

Loading

bozhang2820 commented Feb 21, 2022 •

edited

Loading