[SPARK-34955][SQL] ADD JAR command cannot add jar files which contains whitespaces in the path #32052
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This PR fixes an issue that
ADD JAR
command can't add jar files which contain whitespaces in the path thoughADD FILE
andADD ARCHIVE
work with such files.If we have
/some/path/test file.jar
and execute the following command:The following exception is thrown.
This is because
HiveSessionStateBuilder
andSessionStateBuilder
don't check whether the form of the path is URI or plain path and it always regards the path as URI form.Whitespces should be encoded to
%20
so/some/path/test file.jar
is rejected.We can resolve this part by checking whether the given path is URI form or not.
Unfortunatelly, if we fix this part, another problem occurs.
When we execute
ADD JAR
command, Hive'sADD JAR
command is executed inHiveClientImpl.addJar
andAddResourceProcessor.run
is transitively invoked.In
AddResourceProcessor.run
, the command line is just split bys+
and the path is also split into/some/path/test
andfile.jar
and passed toss.add_resources
.https://github.com/apache/hive/blob/f1e87137034e4ecbe39a859d4ef44319800016d7/ql/src/java/org/apache/hadoop/hive/ql/processors/AddResourceProcessor.java#L56-L75
So, the command still fails.
Even if we convert the form of the path to URI like
file:/some/path/test%20file.jar
and execute the following command:The following exception is thrown.
The reason is
Utilities.realFile
invoked inSessionState.validateFiles
returnsnull
as the result offs.exists(path)
isfalse
.https://github.com/apache/hive/blob/f1e87137034e4ecbe39a859d4ef44319800016d7/ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java#L1052-L1064
fs.exists
checks the existence of the given path by comparing the string representation of Hadoop'sPath
.The string representation of
Path
is similar to URI but it's actually different.Path
doesn't encode the given path.For example, the URI form of
/some/path/jar file.jar
isfile:/some/path/jar%20file.jar
but thePath
form of it isfile:/some/path/jar file.jar
. Sofs.exists
returns false.So the solution I come up with is removing Hive's
ADD JAR
fromHiveClientimpl.addJar
.I think Hive's
ADD JAR
was used to add jar files to the class loader for metadata and isolate the class loader from the one for execution.https://github.com/apache/spark/pull/6758/files#diff-cdb07de713c84779a5308f65be47964af865e15f00eb9897ccf8a74908d581bbR94-R103
But, as of SPARK-10810 and SPARK-10902 (#8909) are resolved, the class loaders for metadata and execution seem to be isolated with different way.
https://github.com/apache/spark/pull/8909/files#diff-8ef7cabf145d3fe7081da799fa415189d9708892ed76d4d13dd20fa27021d149R635-R641
In the current implementation, such class loaders seem to be isolated by
SharedState.jarClassLoader
andIsolatedClientLoader.classLoader
.https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala#L173-L188
https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala#L956-L967
So I wonder we can remove Hive's
ADD JAR
fromHiveClientImpl.addJar
.Why are the changes needed?
This is a bug.
Does this PR introduce any user-facing change?
How was this patch tested?