You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2023/01/10 17:57:19 UTC
[GitHub] [spark] databricks-david-lewis opened a new pull request, #39488: WIP SparkPath
databricks-david-lewis opened a new pull request, #39488:
URL: https://github.com/apache/spark/pull/39488
WIP
<!--
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?
This PR proposes a SparkPath type for spark to internally represent paths.
Today, Spark uses Strings, which are sometimes uri encoded, and sometimes not. This PR aims to fix some of these issues.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1070796985
##########
core/src/main/scala/org/apache/spark/paths/SparkPath.scala:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.paths
+
+import java.net.URI
+
+import org.apache.hadoop.fs.{FileStatus, Path}
+
+/**
+ * A SparkPath is the result of a URI.toString call.
+ */
+case class SparkPath private (private val underlying: String) {
+ def uriEncoded: String = underlying
+ def toUri: URI = new URI(underlying)
+ def toPath: Path = new Path(toUri)
+ override def toString: String = underlying
+}
+
+object SparkPath {
+ def fromPathString(str: String): SparkPath = fromPath(new Path(str))
Review Comment:
I have revamped the comments in this class, please let me know what you think.
--
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 closed pull request #39488: WIP SparkPath
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis closed pull request #39488: WIP SparkPath
URL: https://github.com/apache/spark/pull/39488
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1073096122
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala:
##########
@@ -632,8 +632,8 @@ case class FileSourceScanExec(
}
}.groupBy { f =>
BucketingUtils
- .getBucketId(new Path(f.filePath).getName)
- .getOrElse(throw QueryExecutionErrors.invalidBucketFile(f.filePath))
+ .getBucketId(f.toPath.getName)
+ .getOrElse(throw QueryExecutionErrors.invalidBucketFile(f.urlEncodedPath))
Review Comment:
I'm wondering if we should still show the hadoop path string in the error message, as the url-encoded one is hard to read.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1073096406
##########
sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala:
##########
@@ -3924,18 +3925,18 @@ class Dataset[T] private[sql](
* @since 2.0.0
*/
def inputFiles: Array[String] = {
- val files: Seq[String] = queryExecution.optimizedPlan.collect {
+ val files: Seq[SparkPath] = queryExecution.optimizedPlan.collect {
case LogicalRelation(fsBasedRelation: FileRelation, _, _, _) =>
fsBasedRelation.inputFiles
case fr: FileRelation =>
fr.inputFiles
case r: HiveTableRelation =>
- r.tableMeta.storage.locationUri.map(_.toString).toArray
+ r.tableMeta.storage.locationUri.map(SparkPath.fromUri).toArray
case DataSourceV2ScanRelation(DataSourceV2Relation(table: FileTable, _, _, _, _),
_, _, _, _) =>
table.fileIndex.inputFiles
}.flatten
- files.toSet.toArray
+ files.iterator.map(_.urlEncoded).toSet.toArray
Review Comment:
did we also return url-encoded path string before?
--
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] JoshRosen commented on a diff in pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
JoshRosen commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1070402135
##########
core/src/main/scala/org/apache/spark/paths/SparkPath.scala:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.paths
+
+import java.net.URI
+
+import org.apache.hadoop.fs.{FileStatus, Path}
+
+/**
+ * A SparkPath is the result of a URI.toString call.
+ */
+case class SparkPath private (private val underlying: String) {
+ def uriEncoded: String = underlying
Review Comment:
Should this be `uriEncode` (with an 'I' ) or `urlEncoded` (with an 'L')? I think that "URL encoding" is the more common terminology (https://en.wikipedia.org/wiki/Percent-encoding)
##########
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##########
@@ -383,6 +383,7 @@ public static File createTempDir() throws IOException {
public static File createTempDir(String root, String namePrefix) throws IOException {
if (root == null) root = System.getProperty("java.io.tmpdir");
if (namePrefix == null) namePrefix = "spark";
+ namePrefix = namePrefix + " with a space";
Review Comment:
I like the methodology of ensuring that our test paths contain spaces or special characters.
Should we have a `Utils.isTesting` to flag this only for test code? Are there other methods for creating temporary files in tests that should also be updated?
##########
core/src/main/scala/org/apache/spark/paths/SparkPath.scala:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.paths
+
+import java.net.URI
+
+import org.apache.hadoop.fs.{FileStatus, Path}
+
+/**
+ * A SparkPath is the result of a URI.toString call.
+ */
+case class SparkPath private (private val underlying: String) {
+ def uriEncoded: String = underlying
+ def toUri: URI = new URI(underlying)
+ def toPath: Path = new Path(toUri)
+ override def toString: String = underlying
+}
+
+object SparkPath {
+ def fromPathString(str: String): SparkPath = fromPath(new Path(str))
Review Comment:
I think it would be a good idea to explain what a path string is (perhaps with a reference to the Hadoop docs at https://hadoop.apache.org/docs/stable/api/org/apache/hadoop/fs/Path.html#Path-java.lang.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] itholic commented on pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1378177976
Could we fill up the "Why are the changes needed?" and "How was this patch tested?" section in PR description.
(I know it's still in draft, just for reminding 😄 )
--
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] AmplabJenkins commented on pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1378040869
Can one of the admins verify this patch?
--
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] mridulm commented on pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
mridulm commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1382950980
Agree that invasive changes should not go into patch release.
Given the code freeze is Tue, and this is still WIP - 3.5 might be better vehicle for the change ?
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1070791606
##########
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##########
@@ -383,6 +383,7 @@ public static File createTempDir() throws IOException {
public static File createTempDir(String root, String namePrefix) throws IOException {
if (root == null) root = System.getProperty("java.io.tmpdir");
if (namePrefix == null) namePrefix = "spark";
+ namePrefix = namePrefix + " with a space";
Review Comment:
In my brief search I found that they all came to this method in the end (withTempDir, etc). I'm actually undoing this change for now, because it causes too many failures and I don't have the bandwidth to fix the tests right now.
Here is a PR in my branch where I made only this change: https://github.com/databricks-david-lewis/spark-opensource/pull/2.
I am hoping to fix the problems with spark's tests, and then propose that as a PR.
##########
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##########
@@ -383,6 +383,7 @@ public static File createTempDir() throws IOException {
public static File createTempDir(String root, String namePrefix) throws IOException {
if (root == null) root = System.getProperty("java.io.tmpdir");
if (namePrefix == null) namePrefix = "spark";
+ namePrefix = namePrefix + " with a space";
Review Comment:
In my brief search I found that they all came to this method in the end (withTempDir, etc). I'm actually undoing this change for now, because it causes too many failures and I don't have the bandwidth to fix the tests right now.
Here is a PR in my branch where I made only this change: https://github.com/databricks-david-lewis/spark-opensource/pull/2.
I am hoping to fix the problems with spark's tests, and then propose that as a PR, but I don't know when I will be able to.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1070790755
##########
core/src/main/scala/org/apache/spark/paths/SparkPath.scala:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.paths
+
+import java.net.URI
+
+import org.apache.hadoop.fs.{FileStatus, Path}
+
+/**
+ * A SparkPath is the result of a URI.toString call.
+ */
+case class SparkPath private (private val underlying: String) {
+ def uriEncoded: String = underlying
Review Comment:
I don't feel strongly. I went with "uri" because it was from a `java.net.URI`. I will change it to `urlEncoded`.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1067289016
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/FileScan.scala:
##########
@@ -163,7 +163,7 @@ trait FileScan extends Scan
}
if (splitFiles.length == 1) {
- val path = new Path(splitFiles(0).filePath)
Review Comment:
This was a bug. `filePath` is a uri-encoded 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] databricks-david-lewis commented on pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1383440531
Thanks for the comment @JoshRosen! You put it very well.
One of the other places I wanted to use `SparkPath` was in the streaming interfaces (`FileStreamSink`, `FileStreamSource`, `SinkFileStatus`, and maybe more).
I also wanted to standardize on the currently expected behavior of users providing hadoop-path-strings in SQL statements (table locations, SQLOnFiles, DataFrame-Reader/-Writer, etc). Right now they all (I think) end up in a new Path(_), but could benefit from type-safety.
@mridulm , this PR is ready for review I think. Please take a look. I think the sooner we can get it into a Spark minor version the better. Even if that means we are incrementally changing the internal interfaces in subsequent minor versions.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
URL: https://github.com/apache/spark/pull/39488
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1073681610
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala:
##########
@@ -632,8 +632,8 @@ case class FileSourceScanExec(
}
}.groupBy { f =>
BucketingUtils
- .getBucketId(new Path(f.filePath).getName)
- .getOrElse(throw QueryExecutionErrors.invalidBucketFile(f.filePath))
+ .getBucketId(f.toPath.getName)
+ .getOrElse(throw QueryExecutionErrors.invalidBucketFile(f.urlEncodedPath))
Review Comment:
I tend to agree, because users are always providing the path strings as input (I think?).
But that would be a behavior change I think. Since this is not a behavioral bug, I would prefer to leave that change to later.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1396347502
thanks, merging to master!
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1073097121
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala:
##########
@@ -70,7 +70,7 @@ import org.apache.spark.util.{HadoopFSUtils, ThreadUtils, Utils}
*
* @param paths A list of file system paths that hold data. These will be globbed before if
* the "__globPaths__" option is true, and will be qualified. This option only works
- * when reading from a [[FileFormat]].
+ * when reading from a [[FileFormat]]. These paths are expected to *not* be escaped.
Review Comment:
shall we mention that they should be Hadoop 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] databricks-david-lewis commented on a diff in pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1067285845
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileScanRDD.scala:
##########
@@ -140,14 +147,14 @@ class FileScanRDD(
private def updateMetadataRow(): Unit =
if (metadataColumns.nonEmpty && currentFile != null) {
updateMetadataInternalRow(metadataRow, metadataColumns.map(_.name),
- new Path(currentFile.filePath), currentFile.fileSize, currentFile.modificationTime)
+ currentFile.toPath, currentFile.fileSize, currentFile.modificationTime)
}
/**
* Create an array of constant column vectors containing all required metadata columns
*/
private def createMetadataColumnVector(c: ColumnarBatch): Array[ColumnVector] = {
- val path = new Path(currentFile.filePath)
Review Comment:
This was a bug, we were creating a Path from the uri encoded string. In the case of path with special characters, they were being escaped twice.
--
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 #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
databricks-david-lewis commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1377948537
Thanks @dongjoon-hyun , I have created a ticket. I assigned @JoshRosen as the Sheppard because he has some context already, though I don't know the proper process.
I will continue testing with the GitHub actions in my fork :)
--
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] dongjoon-hyun commented on pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #39488:
URL: https://github.com/apache/spark/pull/39488#issuecomment-1377949927
You can use here too~ :)
--
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] itholic commented on a diff in pull request #39488: [SPARK-41970] Introduce SparkPath for typesafety
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #39488:
URL: https://github.com/apache/spark/pull/39488#discussion_r1066525979
##########
mllib/src/main/scala/org/apache/spark/ml/source/image/ImageFileFormat.scala:
##########
@@ -71,8 +71,8 @@ private[image] class ImageFileFormat extends FileFormat with DataSourceRegister
if (!imageSourceOptions.dropInvalid && requiredSchema.isEmpty) {
Iterator(emptyUnsafeRow)
} else {
- val origin = file.filePath
- val path = new Path(origin)
+ val origin = file.uriEncodedPath
+ val path = file.hadoopPath // TODO (URI)
Review Comment:
Can we file a JIRA and add a link here so that we don't forget ?
e.g.
```scala
// TODO(SPARK-XXXXX): <add short summary for the TODO here would be great>
val path = file.hadoopPath
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/FileScan.scala:
##########
@@ -163,7 +163,7 @@ trait FileScan extends Scan
}
if (splitFiles.length == 1) {
- val path = new Path(splitFiles(0).filePath)
+ val path = splitFiles(0).hadoopPath // TODO (URI)
Review Comment:
ditto
--
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