[SPARK-49501][SQL] Fix double-escaping of table location #47976

cstavr · 2024-09-03T14:25:18Z

What changes were proposed in this pull request?

Change the implementation of createTable to avoid escaping of special chars in UnresolvedTableSpec.location. This field should contain the original user-provided path option and not the URI that is constructed by the buildStorageFormatFromOptions() call.

In addition this commit extends SparkFunSuite and SQLTestUtils to allow creating temporary directories with a custom prefix. This can be used to create temporary directories with special chars.

Why are the changes needed?

Bug fix. The following code would result in the creation of a table that is stored in /tmp/test%table instead of /tmp/test table:

spark.catalog.createTable("testTable", source = "parquet", schema = new StructType().add("id", "int"), description = "", options = Map("path" -> "/tmp/test table"))

Note that this was not consistent with the SQL API, e.g. create table testTable(id int) using parquet location '/tmp/test table'

Does this PR introduce any user-facing change?

Yes. The previous behaviour would result in table path be escaped. After this change the path will not be escaped.

How was this patch tested?

Updated existing test in CatalogSuite.

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

No

cloud-fan · 2024-09-05T13:44:36Z

sql/core/src/test/scala/org/apache/spark/sql/internal/CatalogSuite.scala


    withTable("t") {
-      withTempDir { dir =>
+      withTempDir(prefix = "test%prefix") { dir =>


I'm not sure we need this new util function with prefix parameter, we can just do

withTempDir { baseDir => val dir = new File(baseDir, "test%prefix")

That would also work, but I think is useful to have it since we should expand more on testing of paths with special chars. In the future we could also change the default value, so this helper will be needed to avoid the special chars.

It's orthogonal from this bug fix, isn't it? I'd recommend to proceed that test utility contribution as an independent PR, @cstavr . We can review separately.

That would also work, but I think is useful to have it since we should expand more on testing of paths with special chars. In the future we could also change the default value, so this helper will be needed to avoid the special chars.

ACK. I removed the test util.

cloud-fan

good catch!

cloud-fan · 2024-09-05T13:46:12Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

-      None
-    }
+
+    // The location in UnresolvedTableSpec should be the original user-provided path string.


just in case, shall we add a legacy config to restore to the old behavior?

I thought about this. I did not add a config because the change in behaviour is only for new tables. Existing tables should continue working. I am also suspecting that there is no real usage of this API for tables with special chars. But I don't have a strong opinion. WDYT?

OK let's keep it simple. double escaping is definitely a bug and no one should rely on it (existing tables won't be affected)

dongjoon-hyun

To @cstavr , we can merge your two independent contributions if you split this into two PRs. Let's not mix the improvement and bug fixes into a single PR.

dongjoon-hyun · 2024-09-05T15:20:04Z

sql/core/src/test/scala/org/apache/spark/sql/test/SQLTestUtils.scala


+  /**
+   * Creates a temporary directory with the provided prefix, which is then passed to `f` and will
+   * be deleted after `f` returns.


In a new test utility PR, could you revise this PR description? The AS-IS description looks like a copy of the description of super.withTempDir. However, this has more features, doesn't it?

dongjoon-hyun

+1, LGTM. Thank you, @cstavr .

cloud-fan · 2024-09-09T09:20:11Z

The docker test failure isn't related, thanks, merging to master/3.5!

Change the implementation of `createTable` to avoid escaping of special chars in `UnresolvedTableSpec.location`. This field should contain the original user-provided `path` option and not the URI that is constructed by the `buildStorageFormatFromOptions()` call. In addition this commit extends `SparkFunSuite` and `SQLTestUtils` to allow creating temporary directories with a custom prefix. This can be used to create temporary directories with special chars. Bug fix. The following code would result in the creation of a table that is stored in `/tmp/test%table` instead of `/tmp/test table`: ``` spark.catalog.createTable("testTable", source = "parquet", schema = new StructType().add("id", "int"), description = "", options = Map("path" -> "/tmp/test table")) ``` Note that this was not consistent with the SQL API, e.g. `create table testTable(id int) using parquet location '/tmp/test table'` Yes. The previous behaviour would result in table path be escaped. After this change the path will not be escaped. Updated existing test in `CatalogSuite`. No Closes #47976 from cstavr/location-double-escaping. Authored-by: Christos Stavrakakis <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit dc3333b) Signed-off-by: Wenchen Fan <[email protected]>

Change the implementation of `createTable` to avoid escaping of special chars in `UnresolvedTableSpec.location`. This field should contain the original user-provided `path` option and not the URI that is constructed by the `buildStorageFormatFromOptions()` call. In addition this commit extends `SparkFunSuite` and `SQLTestUtils` to allow creating temporary directories with a custom prefix. This can be used to create temporary directories with special chars. Bug fix. The following code would result in the creation of a table that is stored in `/tmp/test%table` instead of `/tmp/test table`: ``` spark.catalog.createTable("testTable", source = "parquet", schema = new StructType().add("id", "int"), description = "", options = Map("path" -> "/tmp/test table")) ``` Note that this was not consistent with the SQL API, e.g. `create table testTable(id int) using parquet location '/tmp/test table'` Yes. The previous behaviour would result in table path be escaped. After this change the path will not be escaped. Updated existing test in `CatalogSuite`. No Closes apache#47976 from cstavr/location-double-escaping. Authored-by: Christos Stavrakakis <[email protected]> (cherry picked from commit dc3333b) Signed-off-by: Wenchen Fan <[email protected]> Co-authored-by: Christos Stavrakakis <[email protected]>

github-actions bot added SQL STRUCTURED STREAMING CORE labels Sep 3, 2024

cstavr force-pushed the location-double-escaping branch 2 times, most recently from dc9997a to 95ece17 Compare September 4, 2024 09:53

Fix double-escaping of table location

83509fe

cstavr force-pushed the location-double-escaping branch from 95ece17 to 83509fe Compare September 5, 2024 11:15

cloud-fan changed the title ~~[SPARK-49501] Fix double-escaping of table location~~ [SPARK-49501][SQL] Fix double-escaping of table location Sep 5, 2024

cloud-fan reviewed Sep 5, 2024

View reviewed changes

cloud-fan approved these changes Sep 5, 2024

View reviewed changes

cloud-fan reviewed Sep 5, 2024

View reviewed changes

dongjoon-hyun requested changes Sep 5, 2024

View reviewed changes

dongjoon-hyun reviewed Sep 5, 2024

View reviewed changes

Remove withTempDir with prefix

61c83d1

github-actions bot removed STRUCTURED STREAMING CORE labels Sep 6, 2024

cstavr requested a review from dongjoon-hyun September 6, 2024 14:24

dongjoon-hyun approved these changes Sep 6, 2024

View reviewed changes

cloud-fan closed this in dc3333b Sep 9, 2024

[SPARK-49501][SQL] Fix double-escaping of table location #47976

[SPARK-49501][SQL] Fix double-escaping of table location #47976

Uh oh!

Conversation

cstavr commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cstavr commented Sep 3, 2024 •

edited

Loading