You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "databricks-david-lewis (via GitHub)" <gi...@apache.org> on 2023/04/25 16:15:26 UTC
[GitHub] [spark] databricks-david-lewis opened a new pull request, #40947: [Spark-43284] Switch back to url-encoded strings
databricks-david-lewis opened a new pull request, #40947:
URL: https://github.com/apache/spark/pull/40947
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Update `_metadata.file_path` and `_metadata.file_name` to return url-encoded strings, rather than un-encoded strings. This was a regression introduced in Spark 3.4.0.
### Why are the changes needed?
This was an inadvertent behavior change.
### Does this PR introduce _any_ user-facing change?
Yes, fix regression!
### How was this patch tested?
New test added to validate that the `file_path` and `path_name` are returned as encoded strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40947:
URL: https://github.com/apache/spark/pull/40947#issuecomment-1530732071
`FileMetadataStructSuite.metadata struct (json): read partial/all metadata struct fields` fails, @databricks-david-lewis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1185662054
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,13 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile =>
+ // Use new Path(Path.toString).toString as a form of canonicalization
+ new Path(pf.filePath.toPath.toString).toString
Review Comment:
Whoops, this is what I get for doing all my development in the middle of the night 🤦
Sorry, I will put up a new PR soon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1177285829
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,8 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile => new Path(pf.filePath.urlEncoded).getName },
Review Comment:
This is interesting, for the same file, `Path` may return different result in `getName` depending on how the `Path` is constructed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on PR #40947:
URL: https://github.com/apache/spark/pull/40947#issuecomment-1534989116
Updated @cloud-fan. How do we merge this to the 3.4 branch as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #40947: [SPARK-43284] Switch back to url-encoded strings
URL: https://github.com/apache/spark/pull/40947
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1178034396
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,10 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile =>
+ pf.filePath.toUri.getRawPath.split("/").lastOption.getOrElse("")
Review Comment:
I prefer this way as I think ideally `new Path(String)` should take the unencoded path string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40947:
URL: https://github.com/apache/spark/pull/40947#issuecomment-1534179161
@databricks-david-lewis let's just update the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1185661489
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,13 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile =>
+ // Use new Path(Path.toString).toString as a form of canonicalization
+ new Path(pf.filePath.toPath.toString).toString
Review Comment:
I think we should generate the path string from a URI if we want it to be encoded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1177212533
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,8 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile => new Path(pf.filePath.urlEncoded).getName },
Review Comment:
is this different from `pf.toPath.getName`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1177456529
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,8 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile => new Path(pf.filePath.toUri.getRawPath).getName },
Review Comment:
is `pf.filePath.toUri.getRawPath.split("/").last` better?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1185671119
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,13 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile =>
+ // Use new Path(Path.toString).toString as a form of canonicalization
+ new Path(pf.filePath.toPath.toString).toString
Review Comment:
https://github.com/apache/spark/pull/41054
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40947:
URL: https://github.com/apache/spark/pull/40947#issuecomment-1535620328
thanks, merging to master/3.4!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1184672520
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,10 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
Review Comment:
I tried this
```
scala> sql("create table p1 using parquet location 'file:/tmp/path'")
res2: org.apache.spark.sql.DataFrame = []
scala> sql("create table p2 using parquet location 'file:///tmp/path'")
res3: org.apache.spark.sql.DataFrame = []
scala> sql("select _metadata.file_path from p1").show
+--------------------+
| file_path|
+--------------------+
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
+--------------------+
scala> sql("select _metadata.file_path from p2").show
+--------------------+
| file_path|
+--------------------+
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
|file:/tmp/path/pa...|
+--------------------+
```
To avoid regression I think we should do normalization here as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1177391819
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,8 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile => new Path(pf.filePath.urlEncoded).getName },
Review Comment:
Just using `Path.getName` returns the unencoded string (i.e. `some file.txt`).
The new way will return the encoded path name: `some%20file.txt`.
It's not exactly right with URI parameters though... I just updated it to
`new Path(pf.filePath.toUri.getRawPath).getName`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on PR #40947:
URL: https://github.com/apache/spark/pull/40947#issuecomment-1534129513
Oof. This is because of the following:
```
val p1 = new Path("file:///a")
val p2 = new Path("file:/a")
p1 == p2
p1.toString == p2.toString
p1.toUri == p2.toUri
// but...
p1.toUri.toString != p2.toUri.toString // "file:///a" != "file:/a"
```
Before any of my changes, we were doing `new Path(uriString).toString` which would "normalize" the uri string.
Should we do the same here? Seems silly...
I could also update the test to explicitly pass in `file:/...` ...
Thoughts @cloud-fan ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1185656071
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,13 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile =>
+ // Use new Path(Path.toString).toString as a form of canonicalization
+ new Path(pf.filePath.toPath.toString).toString
Review Comment:
oh wait, does it return encoded path string? The test only checks file_name but not file_path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1185663013
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,13 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile =>
+ // Use new Path(Path.toString).toString as a form of canonicalization
+ new Path(pf.filePath.toPath.toString).toString
Review Comment:
It's OK, 3.4.1 won't be released soon and we have time for a followup PR. In the followup PR let's also test `file_path` as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40947: [SPARK-43284] Switch back to url-encoded strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1184669124
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,10 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
Review Comment:
what's the behavior in Spark 3.3? Does Spark normalize the path string regarding `file:/...` and `file:///...`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] databricks-david-lewis commented on a diff in pull request #40947: [Spark-43284] Switch back to url-encoded strings
Posted by "databricks-david-lewis (via GitHub)" <gi...@apache.org>.
databricks-david-lewis commented on code in PR #40947:
URL: https://github.com/apache/spark/pull/40947#discussion_r1178014586
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileFormat.scala:
##########
@@ -265,8 +265,8 @@ object FileFormat {
* fields of the [[PartitionedFile]], and do have entries in the file's metadata map.
*/
val BASE_METADATA_EXTRACTORS: Map[String, PartitionedFile => Any] = Map(
- FILE_PATH -> { pf: PartitionedFile => pf.toPath.toString },
- FILE_NAME -> { pf: PartitionedFile => pf.toPath.getName },
+ FILE_PATH -> { pf: PartitionedFile => pf.filePath.urlEncoded },
+ FILE_NAME -> { pf: PartitionedFile => new Path(pf.filePath.toUri.getRawPath).getName },
Review Comment:
Hmm, maybe. I think that's basically what the path object is doing, but `split` is probably lighter weight. If you think that's safer I'm for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org