dongjoon-hyun commented on code in PR #59947:
URL: https://github.com/apache/spark/pull/59947#discussion_r4449200788
##########
sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala:
##########
@@ -204,6 +215,100 @@ class ArtifactManager(session: SparkSession) extends
AutoCloseable with Logging
// Convert the normalized string back to a Path object
Paths.get(normalizedPathString).normalize()
}
+
+ /**
+ * Returns true if a file artifact with the same content is at `target`
already, which makes
+ * adding it idempotent, and fails with ARTIFACT_ALREADY_EXISTS if another
file is there: an
+ * artifact is not overwritten.
+ */
+ private def artifactAlreadyAdded(
+ normalizedRemoteRelativePath: Path,
+ serverLocalStagingPath: Path,
+ target: Path): Boolean = {
+ if (Files.exists(target)) {
+ if (Utils.contentEquals(target.toFile, serverLocalStagingPath.toFile)) {
+ return true
+ }
+ throw artifactAlreadyExistsError(normalizedRemoteRelativePath)
+ }
+ false
+ }
+
+ private def artifactAlreadyExistsError(normalizedRemoteRelativePath: Path):
Throwable = {
+ new SparkRuntimeException(
+ "ARTIFACT_ALREADY_EXISTS",
+ Map("normalizedRemoteRelativePath" ->
normalizedRemoteRelativePath.toString)
+ )
+ }
+
+ /**
+ * Transfers a native data source package to `target` if Spark can use it,
see
+ * [[NativeDataSourcePackage.checkAdded]], so that it never is at the path
of an artifact
+ * otherwise. Returns false if the session has the same file already, and
fails with
+ * ARTIFACT_ALREADY_EXISTS if it has another one, as artifactAlreadyAdded
does, or with the error
+ * of the check. The file is compared with the artifact first, so that
adding the same file again
+ * neither copies nor checks it. A file of the user is then copied to a
temporary directory,
+ * outside of the artifacts, which the file server serves and a clone of the
session copies, and
+ * the copy is checked and then moved, so that the session gets the bytes
that are checked. Errors
+ * name the file of the user, or the artifact if Spark staged the file for
it, such as for a Spark
+ * Connect client.
+ */
+ private def addNativeDataSourcePackage(
+ normalizedRemoteRelativePath: Path,
+ serverLocalStagingPath: Path,
+ target: Path,
+ deleteStagedFile: Boolean,
+ sourceName: Option[String]): Boolean = {
+ // Returns true if the same file was added already, and fails with
ARTIFACT_ALREADY_EXISTS if
+ // another one was, as artifactAlreadyAdded does.
+ def alreadyAdded(source: Path): Boolean = {
+ // Utils.contentEquals fails for a directory, which cannot be the same
as a package anyway,
+ // so a directory with the name of an artifact, or an artifact that is a
directory, such
+ // as files/x.sparkpkg after files/x.sparkpkg/y was added, fails as an
existing artifact.
+ if (Files.exists(target) && (Files.isDirectory(source) ||
Files.isDirectory(target))) {
+ throw artifactAlreadyExistsError(normalizedRemoteRelativePath)
+ }
+ artifactAlreadyAdded(normalizedRemoteRelativePath, source, target)
+ }
+ // ARTIFACT_ALREADY_EXISTS comes before the check, which is then not
needed for the same file.
+ if (alreadyAdded(serverLocalStagingPath)) {
+ return false
+ }
+ val copyDir = if (deleteStagedFile) None else Some(Utils.createTempDir())
+ Utils.tryWithSafeFinally {
+ val staged = copyDir.map { dir =>
+ // Fails as for the other artifacts if the file does not exist or
cannot be read.
+ val copy = dir.toPath.resolve(normalizedRemoteRelativePath.getFileName)
+ Files.copy(serverLocalStagingPath, copy)
Review Comment:
**14. [Low] On Windows with Java 25, a rejected read-only package leaves its
temporary copy**
On Windows, `Files.copy` uses `CopyFileEx`, which also copies the attributes
of the file, `FILE_ATTRIBUTE_READONLY` included. So when the file of the user
is read-only, for example in a Perforce workspace or after `attrib +R`, this
copy is read-only too. When the copy is not moved, for example because the
package is rejected, `deleteTemporaryDirectory` cannot delete it on Java 25:
- `Utils.deleteQuietly` deletes each file with `File.delete`.
- Since [JDK-8355954](https://bugs.openjdk.org/browse/JDK-8355954), in Java
25, `File.delete` does not delete a read-only file on Windows unless
`jdk.io.File.allowDeleteReadOnlyFiles` is true. Java 17 and 21 cleared the
attribute before they deleted the file.
So each rejected add leaves a copy of the package in `%TEMP%`, logs the
warning at
[L306](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L306),
and keeps the directory registered for the shutdown hook, which fails on it
too. Native data sources need Java 22 or later, so Java 25 is the version that
a Windows user of this feature runs. I did not run this on Windows. It comes
from the Windows code of `WinNTFileSystem` and `WindowsFileCopy` in OpenJDK 21
and 25.
Could the copy be made writable right after `Files.copy`, for example with
`copy.toFile.setWritable(true)`? OpenJDK's own test of this change makes a
read-only file writable before it deletes it. A file that is added stays
read-only at the path of its artifact, as before this PR, so only the temporary
copy is new.
##########
sql/api/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -569,7 +569,10 @@ abstract class SparkSession extends Serializable with
Closeable {
* Ivy repositories local to the client.
*
* Since 4.4.0, it also supports native data source packages: local files
with the extension
- * .sparkpkg, which make the native data sources that they contain available
to the session.
+ * .sparkpkg, which make the native data sources that they contain available
to the session. It
Review Comment:
**19. [Nit] The API docs do not say that the check needs native data
sources**
This sentence, its copies in the other overloads and in `addArtifacts`, and
the Python docstring say that the call fails for a file that is not a valid
package, without a condition. With `spark.sql.dataSource.native.enabled` set to
false, though, `isCheckedPackage` ([ArtifactManager.scala
L170-L173](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L170-L173))
skips the check, and such a file is added as a file of the session, as the
test `add any file with the extension when native data sources are disabled`
checks
([L1692-L1701](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala#L1692-L1701)).
The guide recommends false on a shared server
([L790-L792](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/docs/sql-native-data-s
ources.md?plain=1#L790-L792)), but only the migration guide says what it does
to `.sparkpkg` files. The doc of the configuration ([StaticSQLConf.scala
L467-L477](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala#L467-L477))
and its row in the guide
([L778](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/docs/sql-native-data-sources.md?plain=1#L778))
only say whether Spark loads native data sources. Could the configuration say
that, when it is false, `spark.addArtifact` adds such files as files of the
session without the check?
##########
docs/sql-native-data-sources.md:
##########
@@ -325,12 +325,27 @@ A session finds the packages in two places, and the
executors get them automatic
The packages of a session take precedence over the installed libraries, and
the package that a
session added last over its other packages, also the configured ones: a
session cannot remove an
-artifact, or add a file with the same name again, so it upgrades a package by
adding the new
-version under another file name, such as `rust_range-1.1.sparkpkg`. Distinct
file names are also
-needed because the sessions that are not isolated share the files they add. A
query fails if more
-than one configured package provides its data source. Spark ignores, and logs,
a package that it
-cannot read or whose version it does not support, unless its manifest lists
the data source that a
-query uses and no other package or installed library provides it.
+artifact, or add another file with the name of one that it added, so it
upgrades a package by adding
+the new version under another file name, such as `rust_range-1.1.sparkpkg`.
Distinct file names are
+also needed because the sessions that are not isolated share the files they
add. A query fails if
+more than one configured package provides its data source. `spark.addArtifact`
fails for a file that
Review Comment:
**22. [Nit] The paragraph about packages could be split**
This paragraph
([L326-L348](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/docs/sql-native-data-sources.md?plain=1#L326-L348))
grew from about 130 to about 390 words, and the new text sits between the two
sentences about configured packages: "A query fails if more than one configured
package provides its data source" (L330-L331) and "Spark ignores, and logs, a
configured package that it cannot read ..." (L346-L348). So a reader who looks
for what happens to a broken configured package reads through the whole check
of `spark.addArtifact` first, and the contrast that this PR draws, between a
package that a session adds, which is rejected, and a configured one, which is
ignored and logged, ends up at the two ends of the paragraph. Without changing
the wording, it could be four paragraphs:
- the precedence and the file names,
- the two sentences about configured packages,
- the check of `spark.addArtifact` and what a call does after a rejection,
with the files that cannot be read, and
- what the check does not cover (L340-L345).
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala:
##########
@@ -1555,6 +1559,147 @@ class NativeDataSourceSuite extends QueryTest with
SharedSparkSession {
assert(logged.forall(_.contains("for the data source native_range")),
logged)
}
+ nativeTest("reject a package that a session adds when Spark cannot use it") {
+ // The package that a session added last takes precedence, so Spark
rejects a package that it
+ // cannot use when the session adds it, rather than keep using what the
session used before.
+ val session = newSession(defaultPackage)
+ def ids(): DataFrame = range(session, "end" -> "3").select("id")
+ val dir = Utils.createTempDir()
+ def check(pkg: File, subClass: String, params: Map[String, String]): Unit
= {
+ // Also when the same file is added again.
+ (1 to 2).foreach { _ =>
+ val e =
intercept[SparkRuntimeException](session.addArtifact(pkg.getPath))
+ assert(e.getCondition ==
s"INVALID_NATIVE_DATA_SOURCE_PACKAGE.$subClass")
+ assert(e.getMessageParameters.get("path") == pkg.getPath)
+ params.foreach { case (key, value) =>
assert(e.getMessageParameters.get(key) == value) }
+ }
+ // The session does not get it, and keeps using the configured package.
+ assert(session.artifactManager.getNativeDataSourcePackages._1.isEmpty)
+ assert(!session.artifactManager.hasFileArtifact(pkg.getName))
+ assert(!session.sparkContext.listFiles().exists(_.endsWith("/" +
pkg.getName)))
+ checkRows(ids(), Seq(Row(0L), Row(1L), Row(2L)))
+ }
+ check(createPackage(renamedNegatingLibrary, dataSources(""), dir = dir,
+ fileName = "range-2.0.sparkpkg", abiVersion = 2),
+ "UNSUPPORTED_ABI_VERSION", Map("version" -> "2", "supported" -> "1"))
+ check(createZip(dir, "range-2.1.sparkpkg",
+ NativeDataSourcePackage.MANIFEST_NAME -> manifestJson(
+ dataSources(""), Map(NativePlatform.current ->
"lib.so")).getBytes(UTF_8)),
+ "INVALID_MANIFEST", Map("reason" -> "The package does not contain the
library lib.so."))
+ // ZipFile.getEntry("lib") also finds the directory entry "lib/", as for
the manifest below.
+ check(createZip(dir, "range-2.7.sparkpkg",
+ NativeDataSourcePackage.MANIFEST_NAME -> manifestJson(
+ dataSources(""), Map(NativePlatform.current ->
"lib")).getBytes(UTF_8),
+ "lib/" -> Array.emptyByteArray, "lib/x" -> "x".getBytes(UTF_8)),
+ "INVALID_MANIFEST", Map("reason" -> "The package does not contain the
library lib."))
+ check(createZip(dir, "range-2.8.sparkpkg",
+ s"${NativeDataSourcePackage.MANIFEST_NAME}/" -> Array.emptyByteArray),
+ "INVALID_MANIFEST", Map("reason" ->
+ s"The package does not contain the manifest
${NativeDataSourcePackage.MANIFEST_NAME}."))
+ val notAZip = new File(dir, "range-2.2.sparkpkg")
+ Files.writeString(notAZip.toPath, "not a zip")
+ check(notAZip, "INVALID_MANIFEST", Map.empty)
Review Comment:
**15. [Nit] The new `check` does not pin the reason of a file that is not a
zip**
`check` compares only the parameters that it is given
([L1571-L1574](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala#L1571-L1574)),
so this call checks no `reason`. If a file that is not a zip got another
reason, such as "The package does not contain the manifest ..." or "The package
cannot be read", this test would still pass, and no other test pins "The
package is not a valid zip file", the distinction that item 6 of my first
review was about. The checks of `ARTIFACT_ALREADY_EXISTS` below
([L1646-L1670](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala#L1646-L1670))
also check only the condition, so they would not notice if
`normalizedRemoteRelativePath` named the file of the user or the temporary copy
instead of the arti
fact.
Could `check` use `checkError`, as the one of `invalid packages` does
([L1712-L1717](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala#L1712-L1717))?
```scala
checkError(
exception =
intercept[SparkRuntimeException](session.addArtifact(pkg.getPath)),
condition = s"INVALID_NATIVE_DATA_SOURCE_PACKAGE.$subClass",
parameters = Map("path" -> pkg.getPath) ++ params)
```
The message of the `ZipException` names no file, so this call could pass
`Map("reason" -> s"The package is not a valid zip file:
${intercept[ZipException](new ZipFile(notAZip))}")`, or use
`checkErrorMatchPVals`. The checks of `ARTIFACT_ALREADY_EXISTS` could pass
`Map("normalizedRemoteRelativePath" -> Paths.get("files",
"range-2.0.sparkpkg").toString)`.
##########
python/pyspark/sql/session.py:
##########
@@ -2275,7 +2275,13 @@ def addArtifacts(
----------
*path : tuple of str
Artifact's URIs to add. A native data source package (a
``.sparkpkg`` file) makes
- the native data sources it contains available to the session.
+ the native data sources it contains available to the session. It
fails for a
Review Comment:
**21. [Nit] "It" in the docstring reads as the package**
The sentence before this one is about "A native data source package", so "It
fails for a file that is not a valid package" reads as if the package failed.
The copies in the Scaladoc read fine, since the sentence before them is about
the method or about the extensions. Could this one say "The call fails for a
file ..."? That also matches "stops the rest of the call" below. The Spark
Connect session uses this docstring too.
##########
docs/sql-migration-guide.md:
##########
@@ -30,6 +30,7 @@ license: |
- Since Spark 4.4, Data Source V2 file scans correctly evaluate deterministic
Python UDF filter predicates that reference only partition columns. Previously,
these predicates could be dropped, returning rows that did not satisfy the
filter (SPARK-60486). Data derived from affected query results may need to be
recomputed.
- Since Spark 4.4, quoted Variant path keys decode `\\`, `\/`, `\b`, `\f`,
`\n`, `\r`, `\t`, `\uXXXX` (including valid surrogate pairs), and escapes of
the enclosing quote (`\'` or `\"`). Previously, backslashes were literal, so
existing paths can match different keys or become invalid. For example, use
`$['a\\']` instead of `$['a\']`. Unknown escapes and unpaired surrogate escapes
stay literal. Dot-notation keys are unchanged. To keep the old behavior, set
`spark.sql.legacy.variantPathBackslashAsLiteral` to `true` before creating the
query.
+- Since Spark 4.4, `spark.addArtifact` and `spark.addArtifacts` take a file
whose name ends with `.sparkpkg` as a [native data source
package](sql-native-data-sources.html), also when it is added with `file=True`,
`pyfile=True` or `archive=True` in Python without Spark Connect, or with
`file=True` from a Spark Connect client, and fail with
`INVALID_NATIVE_DATA_SOURCE_PACKAGE` for such a file that is not a valid
package, or whose version Spark does not support, or that has no library for
the platform of the driver. Previously, it was added as a file, a Python file
or an archive. To add such a file as before, rename it. To add it as a file of
the session without the check, set the static configuration
`spark.sql.dataSource.native.enabled` to `false`, which disables native data
sources; in Python, it is then still not added as a Python file or an archive.
Review Comment:
**20. [Nit] The migration guide leaves out classic Python without a flag**
"Previously, it was added as a file, a Python file or an archive" covers the
calls with a flag. Without `file`, `pyfile` or `archive`, classic Python
ignored the path in 4.3: `addArtifacts` only compared it with the files of
`SparkFiles` and added nothing. Now the same `spark.addArtifacts("x.sparkpkg")`
adds the file as a package, or fails with `INVALID_NATIVE_DATA_SOURCE_PACKAGE`.
As far as I can see, it is the only call that worked in 4.3 and can fail now
that the entry leaves out: Scala, Java and the Spark Connect clients rejected
the extension in 4.3, except Python with `file=True`, which the entry covers.
The description mentions it in parentheses. Could the entry say it too, for
example "Previously, it was added as a file, a Python file or an archive, or
ignored without a flag in Python without Spark Connect."?
##########
sql/core/src/test/scala/org/apache/spark/sql/artifact/ArtifactManagerSuite.scala:
##########
@@ -431,6 +434,30 @@ class ArtifactManagerSuite extends SharedSparkSession {
}
}
+ test("Add local artifacts through the public addArtifact") {
+ // A subclass that overrides addArtifact, for example to audit what a
session adds, sees the
Review Comment:
**18. [Nit] An override of `addArtifact` does not see a local package**
This follows item 11 of my third review
([thread](https://github.com/apache/spark/pull/59947#discussion_r4433063716)).
This comment says that an override sees "the local files that addLocalArtifacts
adds". As your reply there says, a local `.sparkpkg` goes to the private
overload instead ([ArtifactManager.scala
L457-L464](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L457-L464))
while native data sources are enabled, which is the default. So an override
that audits what a session adds sees `spark.addArtifact(bytes, "x.sparkpkg")`
and a package from a Spark Connect client, but not
`spark.addArtifact("/x/x.sparkpkg")`. Today only the description says so. Could
this comment say "other than a native data source package", and the Scaladoc of
the public `addArtifact`
([L312-L320](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/
org/apache/spark/sql/artifact/ArtifactManager.scala#L312-L320)) mention it for
whoever overrides it?
A minor point: the override runs on the test thread, so a
`mutable.ArrayBuffer` would do instead of `CopyOnWriteArrayList` and `asScala`.
##########
python/pyspark/sql/tests/test_artifact.py:
##########
@@ -134,6 +150,15 @@ def new_file(name):
spark.addArtifact("my_source:1.sparkpkg")
finally:
os.chdir(cwd)
+ # A file that is not a valid package is rejected.
+ invalid = os.path.join(d, "invalid.sparkpkg")
+ with open(invalid, "wb") as f:
+ f.write(b"not a zip")
+ with self.assertRaises(SparkRuntimeException) as e:
+ spark.addArtifact(invalid)
+ self.assertEqual(
Review Comment:
**16. [Nit] The Python test does not check which file the error names**
This checks only the condition. For a local package, the error names the
file of the user, which the Scala test checks
([L1573](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourceSuite.scala#L1573)).
In Python, a change that staged the package first, for example as bytes, would
name `files/invalid.sparkpkg` and still pass here. Could it also check `path`?
The JVM resolves the path to its canonical form, so on macOS a temporary
directory under `/var` becomes `/private/var`:
```python
self.assertEqual(e.exception.getMessageParameters()["path"],
os.path.realpath(invalid))
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourcePackage.scala:
##########
@@ -177,18 +233,43 @@ object NativeDataSourcePackage extends Logging {
packages.asMap().keySet().asScala.exists(_.path == canonicalPath)
}
- private def invalidManifestError(file: File, reason: String, cause:
Throwable = null) = {
+ private def invalidManifestError(path: String, reason: String, cause:
Throwable = null) = {
QueryExecutionErrors.invalidNativeDataSourcePackageError(
- file.getPath, "INVALID_MANIFEST", Map("reason" -> reason), cause)
+ path, "INVALID_MANIFEST", Map("reason" -> reason), cause)
}
+ /** Reads the package in the given file. */
private def parse(file: File): ReadResult = {
+ readManifest(file, file.getPath).flatMap { manifest =>
+ try {
+ Right(NativeDataSourcePackage(file.getPath, sha256(file), manifest))
+ } catch {
+ case e: IOException => Left(InvalidNativeDataSourcePackage(
Review Comment:
**23. [Nit] The error "The package cannot be read" is built twice**
This builds the same error as `tryRead`
([L201-L202](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourcePackage.scala#L201-L202)),
with the data sources of the manifest instead of `Nil`, and no test checks the
reason of either. A small helper next to `invalidManifestError` would keep the
two in sync:
```scala
private def cannotRead(
file: File,
e: IOException,
dataSources: Seq[String] = Nil): InvalidNativeDataSourcePackage = {
InvalidNativeDataSourcePackage(
invalidManifestError(file.getPath, s"The package cannot be read: $e",
e), dataSources)
}
```
Also, the new doc of `parse`
([L241](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourcePackage.scala#L241))
repeats the first sentence of `read`
([L132](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourcePackage.scala#L132)).
It could say what differs: it reads the manifest and computes the checksum,
without the cache and the log of `tryRead`.
##########
sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala:
##########
@@ -334,6 +454,14 @@ class ArtifactManager(session: SparkSession) extends
AutoCloseable with Logging
artifacts.foreach { artifact =>
try {
artifact.storage match {
+ case d: Artifact.LocalFile if isCheckedPackage(artifact.path) =>
+ // Its errors name the file of the user.
Review Comment:
**17. [Nit] Only the errors of the check name the file of the user**
This comment, the doc of the private `addArtifact`
([L330-L334](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L330-L334))
and the doc of `addNativeDataSourcePackage`
([L252-L254](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L252-L254))
say that the errors name the file of the user. On this path, though,
`ARTIFACT_ALREADY_EXISTS` names the artifact, since it comes from
`artifactAlreadyExistsError(normalizedRemoteRelativePath)`
([L232](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L232),
[L269](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L269)),
and only t
he check uses `sourceName`
([L284-L285](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L284-L285)).
For example, `spark.addArtifact("/home/u/rs.sparkpkg")` on a session that has
another `files/rs.sparkpkg` fails with `ARTIFACT_ALREADY_EXISTS` for
`files/rs.sparkpkg`. Naming the artifact is right for that error, so only the
wording is off. Could these three say "the errors of the check"?
Also, "a file artifact" in the doc of `artifactAlreadyAdded`
([L220](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L220))
means an artifact under `files/` elsewhere in this class, as in
`hasFileArtifact`, but
[L404](https://github.com/apache/spark/blob/3517d4f733ae8fbf6e46f6deb2e3ccf434ce5879/sql/core/src/main/scala/org/apache/spark/sql/artifact/ArtifactManager.scala#L404)
also uses it for jars, Python files and archives, so "an artifact" would be
more accurate.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
