[SPARK-53073][CORE][SQL][YARN][SS] Support copyDirectory in SparkFileUtils and JavaUtils
#51786
Conversation
…ileUtils` and `JavaUtils`
dongjoon-hyun
force-pushed
the
SPARK-53073
branch
from
fe2cd0f to
0c8c10c
Compare
| } | ||
|
|
||
| /** Copy src to the target directory simply. File attribute times are not copied. */ | ||
| public static void copyDirectory(File src, File dst) throws IOException { |
There was a problem hiding this comment.
If src is a File, it seems that a file-to-file copy will occur regardless of whether target exists, and no error will be reported.
There was a problem hiding this comment.
Thank you, @LuciferYang . I addressed your comment.
| throw new IllegalArgumentException("Invalid input file " + src + " or directory " + dst); | ||
| } | ||
| Path from = src.toPath(); | ||
| Path to = dst.toPath(); |
There was a problem hiding this comment.
I suggest adding a check:
if (dstPath.toAbsolutePath().startsWith(srcPath.toRealPath())) {
throw new IllegalArgumentException("Cannot copy directory to itself or its subdirectory");
}
There was a problem hiding this comment.
The rest of them look ok
There was a problem hiding this comment.
Thank you. I got your point. But, for that case, if (dstPath.toRealPath().startsWith(srcPath.toRealPath())) { would be right.
There was a problem hiding this comment.
It's because AbsolutePath and RealPath could be different on the Mac.
scala> new java.io.File("/tmp/spark").toPath.toRealPath()
val res0: java.nio.file.Path = /private/tmp/spark
scala> new java.io.File("/tmp/spark").toPath.toAbsolutePath()
val res1: java.nio.file.Path = /tmp/spark
There was a problem hiding this comment.
Thank you for your corrections
There was a problem hiding this comment.
I'm working on this.
| Path to = dst.toPath().toAbsolutePath().normalize(); | ||
| if (to.startsWith(from)) { | ||
| throw new IllegalArgumentException("Cannot copy directory to itself or its subdirectory"); | ||
| } |
There was a problem hiding this comment.
Here. I addressed your comment, @LuciferYang .
LuciferYang
left a comment
LuciferYang
left a comment
There was a problem hiding this comment.
+1, LGTM
Thank you @dongjoon-hyun
|
Thank you, @LuciferYang . |
|
Merged to master. Thank you, @peter-toth and @LuciferYang . |
3 participants
What changes were proposed in this pull request?
This PR aims to support
copyDirectoryinSparkFileUtilsandJavaUtils.Why are the changes needed?
To provide a better implementation.
BEFORE
AFTER
Does this PR introduce any user-facing change?
No behavior change.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
No.