You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "vicennial (via GitHub)" <gi...@apache.org> on 2023/12/01 13:50:44 UTC
[PR] [SPARK-46202][CONNECT] Expose new ArtifactManager API to support in-memory artifacts and sub-directory structure [spark]
vicennial opened a new pull request, #44109:
URL: https://github.com/apache/spark/pull/44109
<!--
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?
Adds a new client API to the Spark Connect Scala Client:`def addArtifact(target: URI, bytes: Array[Byte]): Unit`
### Why are the changes needed?
Currently, without the use of a REPL/Class finder, there is no API to support adding in-memory artifacts to the remote Spark Connect session.
Further, there is currently no API to preserve/impose a sub-directory structure on the files we send over the wire.
### Does this PR introduce _any_ user-facing change?
Yes.
Users can do the following for classfiles and jars:
```scala
spark.addArtifact(new URI("file:/sub/package/dummyClassFile.class"), bytes)
```
This would preserve the directory structure in the remote server. In this case, the file would be stored under the directory:
`$ROOT_SESSION_ARTIFACT_DIRECTORY/classes/sub/package/dummyClassFile.class`
### 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.
-->
Unit test
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No.
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1423124563
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -590,6 +590,48 @@ class SparkSession private[sql] (
@Experimental
def addArtifact(uri: URI): Unit = client.addArtifact(uri)
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
+ * specified by `target` under the session's working directory of that particular file
+ * extension.
+ *
+ * Supported target file extensions are .jar and .class.
+ *
+ * ==Example==
+ * {{{
+ * addArtifact(bytesBar, "foo/bar.class")
+ * addArtifact(bytesFlat, "flat.class")
+ * // Directory structure of the session's working directory for class files would look like:
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/flat.class
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/foo/bar.class
+ * }}}
+ *
+ * @since 4.0.0
+ */
+ @Experimental
+ def addArtifact(bytes: Array[Byte], target: String): Unit = client.addArtifact(bytes, target)
+
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
Review Comment:
Can you update the docs?
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on PR #44109:
URL: https://github.com/apache/spark/pull/44109#issuecomment-1847013631
I've updated the APIs to be simpler to make them simpler to use (String-based source/target). It is now easier to reason about what the directory-strcuture would look like.
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1423132850
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -108,6 +104,56 @@ class ArtifactManager(
*/
def addArtifact(uri: URI): Unit = addArtifacts(parseArtifacts(uri))
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
+ * specified by `target` under the session's working directory of that particular file
+ * extension.
+ *
+ * Supported target file extensions are .jar and .class.
+ *
+ * ==Example==
+ * {{{
+ * addArtifact(bytesBar, "foo/bar.class")
+ * addArtifact(bytesFlat, "flat.class")
+ * // Directory structure of the session's working directory for class files would look like:
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/flat.class
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/foo/bar.class
+ * }}}
+ */
+ def addArtifact(bytes: Array[Byte], target: String): Unit = {
Review Comment:
It's not used at the moment. We do, however, use a similar variation of this (without a custom `target`) in the `AmmoniteClassFinder`. This API would let user's do the same if they wish 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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell closed pull request #44109: [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories
URL: https://github.com/apache/spark/pull/44109
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on PR #44109:
URL: https://github.com/apache/spark/pull/44109#issuecomment-1847023164
cc @hvanhovell @nija-at
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager API to support in-memory artifacts and sub-directory structure [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1413236630
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -408,14 +432,28 @@ object Artifact {
jars.map(p => Paths.get(p)).map(path => newJarArtifact(path.getFileName, new LocalFile(path)))
}
+ private def concatenatePaths(basePath: Path, otherPath: Path): Path = {
+ // We avoid using the `.resolve()` method here to ensure that we're concatenating the two
+ // paths even if `otherPath` is absolute.
+ val concatenatedPath = Paths.get(basePath.toString + "/" + otherPath.toString)
+ // Note: The normalized resulting path may still reference parent directories if the
+ // `otherPath` contains sufficient number of parent operators (i.e "..").
+ // Example: `basePath` = "/base", `otherPath` = "subdir/../../file.txt"
+ // Then, `concatenatedPath` = "/base/subdir/../../file.txt"
+ // and `normalizedPath` = "/base/file.txt".
+ val normalizedPath = concatenatedPath.normalize()
+ // Verify that the prefix of the `normalizedPath` starts with `basePath/`.
+ require(normalizedPath != basePath && normalizedPath.startsWith(s"$basePath/"))
Review Comment:
```suggestion
require(normalizedPath != basePath && normalizedPath.startsWith(s"$basePath${File.separator}"))
```
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "vicennial (via GitHub)" <gi...@apache.org>.
vicennial commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1423128412
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -590,6 +590,48 @@ class SparkSession private[sql] (
@Experimental
def addArtifact(uri: URI): Unit = client.addArtifact(uri)
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
+ * specified by `target` under the session's working directory of that particular file
+ * extension.
+ *
+ * Supported target file extensions are .jar and .class.
+ *
+ * ==Example==
+ * {{{
+ * addArtifact(bytesBar, "foo/bar.class")
+ * addArtifact(bytesFlat, "flat.class")
+ * // Directory structure of the session's working directory for class files would look like:
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/flat.class
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/foo/bar.class
+ * }}}
+ *
+ * @since 4.0.0
+ */
+ @Experimental
+ def addArtifact(bytes: Array[Byte], target: String): Unit = client.addArtifact(bytes, target)
+
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
Review Comment:
Oops, fixed, good eye!
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager API to support in-memory artifacts and sub-directory structure [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1413237071
##########
connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -590,6 +590,16 @@ class SparkSession private[sql] (
@Experimental
def addArtifact(uri: URI): Unit = client.addArtifact(uri)
+ /**
+ * Add a single in-memory artifact to the client session with a specified target URI.
+ *
+ * Currently it supports files with extensions .jar and .class.
Review Comment:
In API documentation, we should better remove things like "Currently". Should say sth like "Supported file extensions are `.jar` and `.class`".
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager API to support in-memory artifacts and sub-directory structure [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1413236346
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -408,14 +432,28 @@ object Artifact {
jars.map(p => Paths.get(p)).map(path => newJarArtifact(path.getFileName, new LocalFile(path)))
}
+ private def concatenatePaths(basePath: Path, otherPath: Path): Path = {
+ // We avoid using the `.resolve()` method here to ensure that we're concatenating the two
+ // paths even if `otherPath` is absolute.
+ val concatenatedPath = Paths.get(basePath.toString + "/" + otherPath.toString)
Review Comment:
```suggestion
val concatenatedPath = Paths.get(basePath.toString, otherPath.toString)
```
--
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
Re: [PR] [SPARK-46202][CONNECT] Expose new ArtifactManager APIs to support custom target directories [spark]
Posted by "hvanhovell (via GitHub)" <gi...@apache.org>.
hvanhovell commented on code in PR #44109:
URL: https://github.com/apache/spark/pull/44109#discussion_r1423125577
##########
connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ArtifactManager.scala:
##########
@@ -108,6 +104,56 @@ class ArtifactManager(
*/
def addArtifact(uri: URI): Unit = addArtifacts(parseArtifacts(uri))
+ /**
+ * Add a single in-memory artifact to the session while preserving the directory structure
+ * specified by `target` under the session's working directory of that particular file
+ * extension.
+ *
+ * Supported target file extensions are .jar and .class.
+ *
+ * ==Example==
+ * {{{
+ * addArtifact(bytesBar, "foo/bar.class")
+ * addArtifact(bytesFlat, "flat.class")
+ * // Directory structure of the session's working directory for class files would look like:
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/flat.class
+ * // ${WORKING_DIR_FOR_CLASS_FILES}/foo/bar.class
+ * }}}
+ */
+ def addArtifact(bytes: Array[Byte], target: String): Unit = {
Review Comment:
Where is this used? I am not against it, just trying to understand?
--
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