sarutak commented on code in PR #59947:
URL: https://github.com/apache/spark/pull/59947#discussion_r4422640225
##########
docs/sql-native-data-sources.md:
##########
@@ -325,12 +325,15 @@ 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 package
+that Spark cannot read or whose version it does not support, so that a session
does not keep using
Review Comment:
Fixed in 3c422f37c57. The Scaladoc of the `addArtifact` overloads and of
`addArtifacts`, and the docstring of `addArtifacts` in Python, now say that
they fail for a package that Spark cannot read, whose version Spark does not
support, or that has no library for the platform of the driver, and that the
session then does not get the package. "Upgrading from Spark SQL 4.3 to 4.4" in
`sql-migration-guide.md` now says that a file named `*.sparkpkg` is taken as a
package, also with `file=True`, `pyfile=True` or `archive=True` in Python
without Spark Connect, or with `file=True` from a Spark Connect client, and
rejected when Spark cannot use it; that renaming the file adds it as before;
and that setting `spark.sql.dataSource.native.enabled` to `false` adds it as a
file of the session without the check (in Python, still not as a Python file or
an archive). A new test checks that a session with native data sources disabled
adds a file that is not a package.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ffi/NativeDataSourcePackage.scala:
##########
@@ -132,6 +132,33 @@ object NativeDataSourcePackage extends Logging {
/** Reads the package in the given file. The result is cached until the file
changes. */
def read(file: File): NativeDataSourcePackage = tryRead(file).fold(p =>
throw p.error, identity)
+ /**
+ * Checks a package that a session adds with `spark.addArtifact`, in the
given file, before the
+ * session gets it: fails if Spark cannot read the package, or does not
support its version of
+ * the interface. The package that a session added last takes precedence,
and a session cannot
+ * remove an artifact, so Spark rejects such a package rather than ignore
it, as it ignores the
+ * configured ones, which would keep the session on the package that it
added before. The file
+ * is not the artifact itself, so the result is not cached, and errors call
it `path`.
+ */
+ def checkAdded(file: File, path: String): Unit = {
+ parse(file, path) match {
Review Comment:
Decision: fixed in 3c422f37c57, as suggested. The zip, manifest and
library-entry validation moved from `parse` to `readManifest`, with the same
`IOException` handling for the zip and the manifest. An I/O error while `parse`
then computes the checksum is now reported as "The package cannot be read",
with the data sources of the manifest, so a lookup of one of them reports it,
rather than as "not a valid zip file" without them. `parse` calls it and then
builds the package with its checksum for the lookup, and `checkAdded` calls
`readManifest` directly and builds the errors from `path` and the manifest, so
the check reads only the central directory and the manifest. (A local file is
now copied once before the check, following dongjoon-hyun's item 4, so that the
session gets the bytes that are checked; a staged file is only moved.)
Verification: the rejection test still gets the same conditions and
parameters for a package missing a listed library, a package with a newer
`abiVersion` (`version` = 2, `supported` = 1), and now a package without a
library for the platform, and the same condition and `path` for a file that is
not a zip. The lookup-time test of unusable configured packages is unchanged
and passes. The staged unsupported-version test from the third comment guards
the path that the error takes.
##########
docs/sql-native-data-sources.md:
##########
@@ -325,12 +325,15 @@ 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 package
+that Spark cannot read or whose version it does not support, so that a session
does not keep using
Review Comment:
Decision: fixed in 3c422f37c57, following your suggestion. The sentence is
now: "`spark.addArtifact` fails for a package that Spark cannot read, whose
version it does not support, or that has no library for the platform of the
driver, which loads the library to plan a query, instead of adding a package
that the session could not use: the session would ignore it and silently keep
using the package or library that it used before, or fail every query on its
data sources. After the failure, the session keeps the packages that it had,
and a call that adds several artifacts still adds the others, except in Python
without Spark Connect, where `spark.addArtifacts` adds the packages first, one
at a time, so a rejected package stops the rest of the call." (The platform
case is new in this round, following dongjoon-hyun's item 10.)
Verification: the rejection test asserts after each rejection that the
session has no added package and that queries still use the configured package.
##########
sql/connect/server/src/test/scala/org/apache/spark/sql/connect/NativeDataSourceConnectSuite.scala:
##########
@@ -92,6 +93,23 @@ class NativeDataSourceConnectSuite extends
SparkConnectServerTest {
}
}
+ nativeTest("reject a package that Spark cannot use when a session adds it") {
+ val invalid = new File(Utils.createTempDir(), "native_range.sparkpkg")
+ Files.writeString(invalid.toPath, "not a zip")
+ withNewSession { session =>
+ // The client gets the error of the server in the message of a
SparkException.
+ val e = intercept[SparkException](session.addArtifact(invalid.getPath))
+
assert(e.getMessage.contains("[INVALID_NATIVE_DATA_SOURCE_PACKAGE.INVALID_MANIFEST]"),
e)
+ // It names the artifact, rather than the file in which the server
staged it.
+ assert(e.getMessage.contains(s"package
files${File.separator}native_range.sparkpkg:"), e)
Review Comment:
Decision: fixed in 3c422f37c57, with both tests. This test now also adds a
package with `abiVersion` = 2 through the client and asserts that the message
contains "[INVALID_NATIVE_DATA_SOURCE_PACKAGE.UNSUPPORTED_ABI_VERSION]" and
"package files/<name>:". `NativeDataSourceSuite` also adds such a package from
bytes, with `addArtifact(bytes, "range-2.6.sparkpkg")`, and asserts that the
`path` parameter is `files/range-2.6.sparkpkg`.
Verification: with `checkAdded` building the unsupported-version error from
`file.getPath` instead of `path`, the Connect test fails on the new assertion,
and `NativeDataSourceSuite` fails already on its local case. With staged
packages named after the staged file instead of the artifact, the from-bytes
assertion in `NativeDataSourceSuite` fails: `"/.../range-2.6.sparkpkg" did not
equal "files/range-2.6.sparkpkg"`.
--
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]
