From cd5fa1c46a5bb6268caae9806c12d9f4b8878895 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 11:43:19 +0800 Subject: [PATCH 01/11] [s3] Use filesystem-owned multipart write helper --- .../apache/paimon/s3/S3MultiPartUpload.java | 26 +++---------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java index c995dd088141..ead6015fedba 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java @@ -26,9 +26,6 @@ import org.apache.hadoop.fs.s3a.S3AFileSystem; import org.apache.hadoop.fs.s3a.WriteOperationHelper; import org.apache.hadoop.fs.s3a.impl.PutObjectOptions; -import org.apache.hadoop.fs.s3a.statistics.S3AStatisticsContext; -import org.apache.hadoop.fs.store.audit.AuditSpan; -import org.apache.hadoop.fs.store.audit.AuditSpanSource; import software.amazon.awssdk.core.sync.RequestBody; import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadResponse; import software.amazon.awssdk.services.s3.model.CompletedPart; @@ -48,17 +45,12 @@ public class S3MultiPartUpload implements MultiPartUploadStore { private final S3AFileSystem s3a; - private final InternalWriteOperationHelper s3accessHelper; + private final WriteOperationHelper s3accessHelper; public S3MultiPartUpload(S3AFileSystem s3a, Configuration conf) { checkNotNull(s3a); - this.s3accessHelper = - new InternalWriteOperationHelper( - s3a, - checkNotNull(conf), - s3a.createStoreContext().getInstrumentation(), - s3a.getAuditSpanSource(), - s3a.getActiveAuditSpan()); + checkNotNull(conf); + this.s3accessHelper = s3a.createWriteOperationHelper(s3a.getActiveAuditSpan()); this.s3a = s3a; } @@ -117,16 +109,4 @@ UploadPartRequest newUploadPartRequest( public void abortMultipartUpload(String destKey, String uploadId) throws IOException { s3accessHelper.abortMultipartUpload(destKey, uploadId, false, null); } - - private static final class InternalWriteOperationHelper extends WriteOperationHelper { - - InternalWriteOperationHelper( - S3AFileSystem owner, - Configuration conf, - S3AStatisticsContext statisticsContext, - AuditSpanSource auditSpanSource, - AuditSpan auditSpan) { - super(owner, conf, statisticsContext, auditSpanSource, auditSpan, null); - } - } } From 010f553fa59fd9981d1d157e3227131b0e6ceca9 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 11:43:30 +0800 Subject: [PATCH 02/11] [common] Define provider-neutral FileIO contract --- .../java/org/apache/paimon/fs/FileIO.java | 193 +++-- .../java/org/apache/paimon/fs/FileStatus.java | 25 +- .../fs/RenamingTwoPhaseOutputStream.java | 7 +- .../apache/paimon/fs/SeekableInputStream.java | 15 +- .../paimon/fs/TwoPhaseOutputStream.java | 42 +- .../apache/paimon/fs/local/LocalFileIO.java | 27 +- .../paimon/fs/FileIOBehaviorTestBase.java | 768 +++++++++++++++--- .../fs/HadoopLocalFileIOBehaviorTest.java | 20 +- .../paimon/fs/LocalFileIOBehaviorTest.java | 30 + .../fs/RenamingTwoPhaseOutputStreamTest.java | 21 +- 10 files changed, 882 insertions(+), 266 deletions(-) diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java index 2b0dcec3f760..cfeb99e442fa 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java @@ -59,7 +59,9 @@ import static org.apache.paimon.utils.Preconditions.checkArgument; /** - * File IO to read and write file. + * Provider-neutral file I/O for files and logical directories. + * + *

Implementations are not required to materialize directory markers for logical directories. * * @since 0.4.0 */ @@ -78,37 +80,49 @@ public interface FileIO extends Serializable, Closeable { default void setRuntimeContext(Map options) {} /** - * Opens an SeekableInputStream at the indicated Path. + * Opens a {@link SeekableInputStream} for a file. + * + *

The returned stream starts at position zero and supports seeking from zero through the + * file length, inclusive. Behavior for offsets outside that range is unspecified. If the path + * is missing or is a directory, this method or the first read from the returned stream throws + * an {@link IOException}. * * @param path the file to open + * @return a seekable stream for the file + * @throws IOException if the file cannot be read */ SeekableInputStream newInputStream(Path path) throws IOException; /** - * Opens an PositionOutputStream at the indicated Path. + * Opens a {@link PositionOutputStream} for a file. + * + *

When no ancestor is a file, missing logical parents are created. A successful close makes + * the complete written content visible. If the target exists, {@code overwrite=true} replaces + * it. With {@code overwrite=false}, the conflict may be reported while opening, writing, or + * closing the stream, and the existing content remains unchanged. * - * @param path the file name to open - * @param overwrite if a file with this name already exists, then if true, the file will be - * overwritten, and if false an error will be thrown. - * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because a - * file already exists at that path and the write mode indicates to not overwrite the file. + * @param path the file to write + * @param overwrite whether to replace an existing file + * @return a stream whose position tracks the number of bytes written + * @throws IOException if the file cannot be written */ PositionOutputStream newOutputStream(Path path, boolean overwrite) throws IOException; /** - * Opens a TwoPhaseOutputStream at the indicated Path for transactional writing. + * Opens a {@link TwoPhaseOutputStream} that stages data for later publication. * - *

This method creates a stream that supports transactional writing operations. The written - * data becomes visible only after calling commit on the returned committer from closeForCommit - * method. + *

Staged data is not published at the target before commit is invoked. A successful commit + * publishes the complete data; if commit fails, the target state is unspecified. If the target + * already exists, whether the request is rejected or replaces the target, when a rejection is + * reported, and whether replacement is atomic are not specified by this interface. An + * implementation may document stronger guarantees. The staging layout is also not specified. * * @param path the file target path - * @param overwrite if a file with this name already exists, then if true, the file will be - * overwritten, and if false an error will be thrown. - * @return a TwoPhaseOutputStream that supports transactional writes - * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because a - * file already exists at that path and the write mode indicates to not overwrite the file. - * @throws UnsupportedOperationException if the filesystem does not support transactional writes + * @param overwrite requests replacement of an existing file; existing-target behavior is + * provider-specific + * @return a stream that stages data for the target + * @throws IOException if the stream cannot be created + * @throws UnsupportedOperationException if the file system does not support staged writes */ default TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrite) throws IOException { @@ -116,30 +130,37 @@ default TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrit } /** - * Return a file status object that represents the path. + * Returns a metadata snapshot for a path. * - * @param path The path we want information from - * @return a FileStatus object - * @throws FileNotFoundException when the path does not exist; IOException see specific - * implementation + * @param path the path to inspect + * @return a snapshot of the path's status + * @throws FileNotFoundException if the path does not exist + * @throws IOException if the status cannot be read */ FileStatus getFileStatus(Path path) throws IOException; /** - * List the statuses of the files/directories in the given path if the path is a directory. + * Lists the direct children of an existing directory. * - * @param path given path - * @return the statuses of the files/directories in the given path + *

The result is non-null and unordered. Each status has the child's path and type; file + * statuses also have the file length. Behavior for a missing path or a file path is + * unspecified. + * + * @param path an existing directory + * @return the direct child statuses, or an empty array for an empty directory */ FileStatus[] listStatus(Path path) throws IOException; /** - * List the statuses of the files in the given path if the path is a directory. + * Lists files under an existing directory. + * + *

The result is non-null and unordered. It contains the same set of file paths as {@link + * #listFilesIterative(Path, boolean)} for the same arguments. Behavior for a missing path or a + * file path is unspecified. * - * @param path given path - * @param recursive if set to true will recursively list files in subdirectories, - * otherwise only files in the current directory will be listed - * @return the statuses of the files in the given path + * @param path an existing directory + * @param recursive whether to descend into subdirectories + * @return only file statuses, recursively if requested */ default FileStatus[] listFiles(Path path, boolean recursive) throws IOException { List files = new ArrayList<>(); @@ -151,12 +172,15 @@ default FileStatus[] listFiles(Path path, boolean recursive) throws IOException } /** - * List the statuses of the files iteratively in the given path if the path is a directory. + * Iterates over files under an existing directory. * - * @param path given path - * @param recursive if set to true will recursively list files in subdirectories, - * otherwise only files in the current directory will be listed - * @return an {@link RemoteIterator} over {@link FileStatus} of the files in the given path + *

The iterator is non-null and unordered. It contains the same set of file paths as {@link + * #listFiles(Path, boolean)} for the same arguments. Behavior for a missing path or a file path + * is unspecified. + * + * @param path an existing directory + * @param recursive whether to descend into subdirectories + * @return an iterator containing only file statuses, recursively if requested */ default RemoteIterator listFilesIterative(Path path, boolean recursive) throws IOException { @@ -195,12 +219,13 @@ private void maybeUnpackDirectory() throws IOException { } /** - * List the statuses of the directories in the given path if the path is a directory. + * Lists the direct directories under an existing directory. * - *

{@link FileIO} implementation may have optimization for list directories. + *

The result is non-null and unordered. Behavior for a missing path or a file path is + * unspecified. * - * @param path given path - * @return the statuses of the directories in the given path + * @param path an existing directory + * @return only direct child directory statuses, or an empty array if there are none */ default FileStatus[] listDirectories(Path path) throws IOException { FileStatus[] statuses = listStatus(path); @@ -211,40 +236,52 @@ default FileStatus[] listDirectories(Path path) throws IOException { } /** - * Check if exists. + * Checks whether a file or logical directory exists. * - * @param path source file + * @param path the path to check + * @return whether the path exists */ boolean exists(Path path) throws IOException; /** - * Delete a file. + * Deletes a file or directory. + * + *

An existing file is deleted for either value of {@code recursive}. An empty directory is + * deleted, and a non-empty directory is deleted with its subtree when {@code recursive} is + * true. These successful deletions return true. Deleting a non-empty directory with {@code + * recursive=false} throws an {@link IOException} and preserves the complete tree. A missing + * path does not throw solely because it is absent; its return value is unspecified. * * @param path the path to delete - * @param recursive if path is a directory and set to true, the directory is - * deleted else throws an exception. In case of a file the recursive can be set to either - * true or false - * @return true if delete is successful, false otherwise + * @param recursive whether to delete a directory subtree + * @return true when an existing target is successfully deleted; unspecified for a missing path */ boolean delete(Path path, boolean recursive) throws IOException; /** - * Make the given file and all non-existent parents into directories. Has the semantics of Unix - * 'mkdir -p'. Existence of the directory hierarchy is not an error. + * Makes a logical directory and any missing logical parents. * - * @param path the directory/directories to be created - * @return true if at least one new directory has been created, false - * otherwise - * @throws IOException thrown if an I/O error occurs while creating the directory + *

When the target and its ancestors are not files, this method returns true and leaves them + * as directories. Repeating the call also returns true. Implementations need not materialize + * directory markers. + * + * @param path the directory to create + * @return true on successful creation or when the directory already exists + * @throws IOException if the directory cannot be created */ boolean mkdirs(Path path) throws IOException; /** - * Renames the file/directory src to dst. + * Renames a file or directory in the guaranteed non-conflicting case. + * + *

When the source exists, the exact destination is missing, and the destination parent is + * valid, this method returns true, removes the source, and preserves the file bytes or + * directory tree at the exact destination. Behavior in all other cases, including destination + * conflicts, is unspecified. Atomicity is not guaranteed. * - * @param src the file/directory to rename - * @param dst the new name of the file/directory - * @return true if the renaming was successful, false otherwise + * @param src the source file or directory + * @param dst the exact destination path + * @return true for the guaranteed case; otherwise unspecified */ boolean rename(Path src, Path dst) throws IOException; @@ -345,10 +382,13 @@ default String readFileUtf8(Path path) throws IOException { } /** - * Write content to one file atomically, initially writes to temp hidden file and only renames - * to the target file once temp file is closed. + * Writes content through a temporary file and then renames it to the target. * - * @return false if target file exists + *

If the target is missing and its parent is valid, a true result guarantees the target has + * the requested content. This method does not add an atomicity or conflict guarantee beyond + * {@link #rename(Path, Path)}. + * + * @return whether the final rename reported success */ default boolean tryToWriteAtomic(Path path, String content) throws IOException { Path tmp = path.createTempPath(); @@ -373,10 +413,7 @@ default void writeFile(Path path, String content, boolean overwrite) throws IOEx } } - /** - * Overwrite file by content atomically, different {@link FileIO}s have different atomic - * implementations. - */ + /** Overwrites a file with UTF-8 content without guaranteeing atomic replacement. */ default void overwriteFileUtf8(Path path, String content) throws IOException { try (PositionOutputStream out = newOutputStream(path, true)) { OutputStreamWriter writer = new OutputStreamWriter(out, StandardCharsets.UTF_8); @@ -385,21 +422,22 @@ default void overwriteFileUtf8(Path path, String content) throws IOException { } } - /** - * Overwrite hint file by content atomically, the characteristic of Hint file is that it can not - * exist for a period of time, which allows some file systems to perform overwrite writing by - * deleting and renaming. - */ + /** Overwrites a hint file with UTF-8 content without guaranteeing atomic replacement. */ default void overwriteHintFile(Path path, String content) throws IOException { overwriteFileUtf8(path, content); } /** - * Copy content of one file into another. + * Copies the bytes of a source file to a target file. * - * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because - * target file already exists at that path and the write mode indicates to not overwrite the - * file. + *

If the target exists, {@code overwrite=true} replaces it. With {@code overwrite=false}, + * the copy fails and preserves the existing target content. + * + * @param sourcePath the source file + * @param targetPath the target file + * @param overwrite whether to replace an existing target + * @throws IOException if the file cannot be copied or overwrite is disabled for an existing + * target */ default void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throws IOException { try (SeekableInputStream is = newInputStream(sourcePath); @@ -408,7 +446,14 @@ default void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throw } } - /** Copy all files in sourceDirectory to directory targetDirectory. */ + /** + * Copies every direct file from a source directory to a target directory according to {@link + * #copyFile(Path, Path, boolean)}. + * + *

{@code sourceDirectory} and {@code targetDirectory} must be directories, and each direct + * child of {@code sourceDirectory} must be a file. The method is not recursive and does not + * roll back files copied before a later failure. + */ default void copyFiles(Path sourceDirectory, Path targetDirectory, boolean overwrite) throws IOException { FileStatus[] fileStatuses = listStatus(sourceDirectory); diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java b/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java index c3e6cde9cf0b..1a9868d10db5 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java @@ -23,7 +23,10 @@ import javax.annotation.Nullable; /** - * Interface that represents the client side information for a file independent of the file system. + * A snapshot of provider-neutral metadata for a file or logical directory. + * + *

The values do not change when the underlying path changes. Owner and access time, as well as + * directory length and modification time, are not portable across file systems. * * @since 0.4.0 */ @@ -31,9 +34,9 @@ public interface FileStatus { /** - * Return the length of this file. + * Returns the file length in bytes. The value for a directory is not portable. * - * @return the length of this file + * @return the file length in bytes */ long getLen(); @@ -52,27 +55,27 @@ public interface FileStatus { Path getPath(); /** - * Get the last modification time of the file. + * Returns the last modification time in milliseconds since the epoch. The value for a directory + * is not portable. * - * @return A long value representing the time the file was last modified, measured in - * milliseconds since the epoch (UTC January 1, 1970). + * @return the last modification time */ long getModificationTime(); /** - * Get the last access time of the file. + * Returns the last access time in milliseconds since the epoch, if available. This value is not + * portable and may be zero. * - * @return A long value representing the time the file was last accessed, measured in - * milliseconds since the epoch (UTC January 1, 1970). + * @return the last access time, or zero when unavailable */ default long getAccessTime() { return 0; } /** - * Returns the owner of this file. + * Returns the owner, if available. This value is not portable. * - * @return the owner of this file + * @return the owner, or null when unavailable */ @Nullable default String getOwner() { diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java index a8547bd1673b..4771fd671605 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java @@ -122,12 +122,7 @@ public void commit(FileIO fileIO) throws IOException { @Override public void discard(FileIO fileIO) throws IOException { - if (fileIO.exists(targetPath)) { - fileIO.deleteQuietly(targetPath); - } - if (fileIO.exists(tempPath)) { - fileIO.deleteQuietly(tempPath); - } + fileIO.deleteQuietly(tempPath); } @Override diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java index 32be1d1139f6..6216dbd21755 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java @@ -24,7 +24,7 @@ import java.io.InputStream; /** - * {@code SeekableInputStream} provides seek methods. + * An input stream with a queryable byte position and seek support. * * @since 0.4.0 */ @@ -32,11 +32,12 @@ public abstract class SeekableInputStream extends InputStream { /** - * Seek to the given offset from the start of the file. The next read() will be from that - * location. Can't seek past the end of the stream. + * Seeks to an offset from the start of the file. After a successful return, {@link #getPos()} + * reports that offset and the next read begins there. Behavior for negative offsets or offsets + * past the end of the file is implementation-specific. * * @param desired the desired offset - * @throws IOException Thrown if an error occurred while seeking inside the input stream. + * @throws IOException if seeking fails */ public abstract void seek(long desired) throws IOException; @@ -50,9 +51,9 @@ public abstract class SeekableInputStream extends InputStream { public abstract long getPos() throws IOException; /** - * Reads up to len bytes of data from the input stream into an array of bytes. An - * attempt is made to read as many as len bytes, but a smaller number may be read. - * The number of bytes actually read is returned as an integer. + * Reads up to {@code len} bytes into the array. + * + * @return the number of bytes read, or {@code -1} at the end of the file */ public abstract int read(byte[] b, int off, int len) throws IOException; diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java index 931969ec68cb..fc67fa02c04f 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java @@ -21,48 +21,58 @@ import java.io.IOException; import java.io.Serializable; -/** TwoPhaseOutputStream provides a way to write to a file and get a committer that can commit. */ +/** + * An output stream that stages data and produces a committer to publish it. + * + *

Staged data is not published at the target before commit is invoked. A successful commit makes + * the complete data visible but does not imply atomic replacement. If commit fails, the target + * state is unspecified. + */ public abstract class TwoPhaseOutputStream extends PositionOutputStream { + /** - * Closes the stream for writing and returns a committer that can be used to make the written - * data visible. + * Closes the stream for writing and returns a committer for the staged data. * - *

After calling this method, the stream should not be used for writing anymore. The returned - * committer can be used to commit the data or discard it. + *

After this call, the stream must not be used for writing. The staged data remains + * unpublished until {@link Committer#commit(FileIO)} is invoked. * - * @return A committer that can be used to commit the data + * @return a committer that can publish or discard the staged data * @throws IOException if an I/O error occurs during closing */ public abstract Committer closeForCommit() throws IOException; - /** A committer interface that can commit or discard the written data. */ + /** A serializable handle that can publish or discard one stream's staged data. */ public interface Committer extends Serializable { /** - * Commits the written data, making it visible. + * Publishes the complete staged data at {@link #targetPath()}. + * + *

A successful return makes the complete data visible but does not guarantee atomic + * replacement. If this method throws, the target state is unspecified. * * @throws IOException if an I/O error occurs during commit */ void commit(FileIO fileIO) throws IOException; /** - * Discards the written data, cleaning up any temporary files or resources. Called instead - * of {@link #commit} when the write is given up. + * Discards this write's staged data instead of publishing it. + * + *

Only resources created by this write may be removed. In particular, discard must not + * remove target content or resources created by another writer. * * @throws IOException if an I/O error occurs during discard */ void discard(FileIO fileIO) throws IOException; + /** Returns the path where a successful commit publishes the staged data. */ Path targetPath(); /** - * Releases what this committer staged and no longer needs, after {@link #commit} has - * succeeded. May do nothing. + * Releases staging resources that this write no longer needs after {@link #commit} + * succeeds. * - *

Only resources this committer created itself. A staging directory is shared with every - * other writer of the same location, Paimon or not, and removing it is theirs to decide: - * finding it empty does not mean it is unused, because a writer that has just created it - * has not staged its file in it yet. + *

This method may do nothing. It may remove only resources created by this write and + * must not remove the committed target or resources owned by another writer. * * @throws IOException if an I/O error occurs during cleaning */ diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java index 143f9d8c243c..0bcca68dc3ff 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java @@ -243,11 +243,12 @@ public boolean rename(Path src, Path dst) throws IOException { @Override public void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throws IOException { LOG.debug("Invoking copyFile for {} to {}", sourcePath, targetPath); - if (!overwrite && exists(targetPath)) { - return; - } toPath(targetPath.getParent()).toFile().mkdirs(); - Files.copy(toPath(sourcePath), toPath(targetPath), StandardCopyOption.REPLACE_EXISTING); + if (overwrite) { + Files.copy(toPath(sourcePath), toPath(targetPath), StandardCopyOption.REPLACE_EXISTING); + } else { + Files.copy(toPath(sourcePath), toPath(targetPath)); + } } private java.nio.file.Path toPath(Path path) { @@ -362,14 +363,16 @@ public void close() throws IOException { private static class LocalFileStatus implements FileStatus { - private final File file; + private final Path path; + private final boolean directory; private final long length; - private final String scheme; + private final long modificationTime; private LocalFileStatus(File file, String scheme) { - this.file = file; + this.path = new Path(scheme + ":" + file.toURI().getPath()); + this.directory = file.isDirectory(); this.length = file.length(); - this.scheme = scheme; + this.modificationTime = file.lastModified(); } @Override @@ -379,22 +382,22 @@ public long getLen() { @Override public boolean isDir() { - return file.isDirectory(); + return directory; } @Override public Path getPath() { - return new Path(scheme + ":" + file.toURI().getPath()); + return path; } @Override public long getModificationTime() { - return file.lastModified(); + return modificationTime; } @Override public String toString() { - return "{" + "file=" + file + ", length=" + length + ", scheme='" + scheme + '\'' + '}'; + return "{" + "path=" + path + ", directory=" + directory + ", length=" + length + '}'; } } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java index 0ad71039983a..a4800639857f 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java @@ -24,20 +24,24 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.io.ByteArrayOutputStream; +import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Random; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.assertj.core.api.Assertions.fail; -/** Common tests for the behavior of {@link FileIO} methods. */ +/** Provider-neutral contract tests for {@link FileIO}. */ public abstract class FileIOBehaviorTestBase { private static final Random RND = new Random(); + private static final byte[] DEFAULT_CONTENT = new byte[] {1, 2, 3, 4, 5, 6, 7, 8}; + /** The cached file system instance. */ private FileIO fs; @@ -70,194 +74,668 @@ void cleanup() throws Exception { fs.delete(basePath, true); } + @Test + void testObjectStoreClassificationIsStable() throws IOException { + boolean objectStore = fs.isObjectStore(); + Path file = createRandomFileInDirectory(basePath); + + assertThat(fs.isObjectStore()).isEqualTo(objectStore); + + fs.delete(file, false); + assertThat(fs.isObjectStore()).isEqualTo(objectStore); + } + // ------------------------------------------------------------------------ - // Suite of Tests + // Input streams // ------------------------------------------------------------------------ - // --- exists + @Test + void testInputStreamStartsAtZeroAndReadsCorrectBytes() throws IOException { + byte[] content = new byte[] {3, 1, 4, 1, 5, 9}; + Path file = createRandomFileInDirectory(basePath, content); + + try (SeekableInputStream in = fs.newInputStream(file)) { + assertThat(in.getPos()).isZero(); + assertThat(readAll(in)).containsExactly(content); + assertThat(in.getPos()).isEqualTo(content.length); + } + } @Test - void testFileExists() throws IOException { - final Path filePath = createRandomFileInDirectory(basePath); - assertThat(fs.exists(filePath)).isTrue(); + void testInputStreamBulkReadHonorsNonZeroBufferOffset() throws IOException { + byte[] content = new byte[] {11, 22, 33}; + Path file = createRandomFileInDirectory(basePath, content); + byte[] buffer = new byte[] {99, 98, 0, 0, 0, 97, 96}; + + try (SeekableInputStream in = fs.newInputStream(file)) { + int totalRead = 0; + while (totalRead < content.length) { + int read = in.read(buffer, 2 + totalRead, content.length - totalRead); + assertThat(read).isPositive(); + totalRead += read; + } + + assertThat(totalRead).isEqualTo(content.length); + assertThat(buffer).containsExactly(99, 98, 11, 22, 33, 97, 96); + assertThat(in.getPos()).isEqualTo(content.length); + } } @Test - void testFileDoesNotExist() throws IOException { - assertThat(fs.exists(new Path(basePath, randomName()))).isFalse(); + void testInputStreamsHaveIndependentPositions() throws IOException { + Path file = createRandomFileInDirectory(basePath, new byte[] {10, 20, 30}); + + try (SeekableInputStream first = fs.newInputStream(file); + SeekableInputStream second = fs.newInputStream(file)) { + assertThat(first.read()).isEqualTo(10); + assertThat(first.getPos()).isEqualTo(1); + assertThat(second.getPos()).isZero(); + assertThat(second.read()).isEqualTo(10); + assertThat(second.getPos()).isEqualTo(1); + + first.seek(2); + assertThat(first.read()).isEqualTo(30); + assertThat(second.read()).isEqualTo(20); + } } - // --- list files + @Test + void testInputStreamSeeksForwardAndBackward() throws IOException { + byte[] content = new byte[] {10, 20, 30, 40, 50, 60}; + Path file = createRandomFileInDirectory(basePath, content); + + try (SeekableInputStream in = fs.newInputStream(file)) { + in.seek(4); + assertThat(in.getPos()).isEqualTo(4); + assertThat(in.read()).isEqualTo(50); + + in.seek(1); + assertThat(in.getPos()).isEqualTo(1); + assertThat(in.read()).isEqualTo(20); + } + } + + @Test + void testInputStreamReturnsEndOfFileAtFileLength() throws IOException { + byte[] content = new byte[] {7, 8, 9}; + Path file = createRandomFileInDirectory(basePath, content); + + try (SeekableInputStream in = fs.newInputStream(file)) { + in.seek(content.length); + assertThat(in.read()).isEqualTo(-1); + assertThat(in.read(new byte[2], 0, 2)).isEqualTo(-1); + } + } @Test - void testListFilesIterativeNonRecursive() throws IOException { - Path fileA = createRandomFileInDirectory(basePath); - Path dirB = new Path(basePath, randomName()); - fs.mkdirs(dirB); - Path fileBC = createRandomFileInDirectory(dirB); + void testInputStreamCanSeekBackToStart() throws IOException { + byte[] content = new byte[] {7, 8, 9}; + Path file = createRandomFileInDirectory(basePath, content); + + try (SeekableInputStream in = fs.newInputStream(file)) { + assertThat(in.read()).isEqualTo(7); + in.seek(0); + assertThat(in.getPos()).isZero(); + assertThat(in.read()).isEqualTo(7); + } + } - List allFiles = new ArrayList<>(); - RemoteIterator iter = fs.listFilesIterative(basePath, false); - while (iter.hasNext()) { - allFiles.add(iter.next()); + @Test + void testInputStreamSeeksForwardBeyondOneMebibyte() throws IOException { + int targetPosition = 1024 * 1024 + 17; + byte[] content = new byte[targetPosition + 1]; + content[targetPosition] = 42; + Path file = createRandomFileInDirectory(basePath, content); + + try (SeekableInputStream in = fs.newInputStream(file)) { + in.seek(targetPosition); + assertThat(in.getPos()).isEqualTo(targetPosition); + assertThat(in.read()).isEqualTo(42); } - assertThat(allFiles.size()).isEqualTo(1); - assertThat(allFiles.get(0).getPath()).isEqualTo(fileA); } @Test - void testListFilesIterativeRecursive() throws IOException { - Path fileA = createRandomFileInDirectory(basePath); - Path dirB = new Path(basePath, randomName()); - fs.mkdirs(dirB); - Path fileBC = createRandomFileInDirectory(dirB); + void testInputStreamForMissingFileFailsByFirstRead() { + Path missing = new Path(basePath, randomName()); + + assertOpenOrFirstReadFails(missing); + } + + @Test + void testInputStreamForDirectoryFailsByFirstRead() throws IOException { + Path directory = new Path(basePath, randomName()); + fs.mkdirs(directory); + + assertOpenOrFirstReadFails(directory); + } + + // ------------------------------------------------------------------------ + // Output streams + // ------------------------------------------------------------------------ - List allFiles = new ArrayList<>(); - RemoteIterator iter = fs.listFilesIterative(basePath, true); - while (iter.hasNext()) { - allFiles.add(iter.next()); + @Test + void testOutputStreamTracksPositionAndPublishesBytesOnClose() throws IOException { + Path file = new Path(basePath, randomName()); + + try (PositionOutputStream out = fs.newOutputStream(file, false)) { + assertThat(out.getPos()).isZero(); + out.write(9); + assertThat(out.getPos()).isEqualTo(1); + out.write(new byte[] {10, 11, 12, 13}, 1, 2); + assertThat(out.getPos()).isEqualTo(3); } - assertThat(allFiles.size()).isEqualTo(2); - assertThat(allFiles.stream().filter(f -> f.getPath().equals(fileA)).count()).isEqualTo(1); - assertThat(allFiles.stream().filter(f -> f.getPath().equals(fileBC)).count()).isEqualTo(1); + + assertThat(readBytes(file)).containsExactly(9, 11, 12); } - // --- delete + @Test + void testOutputStreamCreatesNestedTarget() throws IOException { + Path ancestor = new Path(basePath, randomName()); + Path parent = new Path(ancestor, randomName()); + Path file = new Path(parent, randomName()); + byte[] content = new byte[] {1, 3, 3, 7}; + + writeBytes(file, content, false); + + assertThat(readBytes(file)).containsExactly(content); + assertThat(fs.getFileStatus(ancestor).isDir()).isTrue(); + assertThat(fs.getFileStatus(parent).isDir()).isTrue(); + } @Test - void testExistingFileDeletion() throws IOException { - testSuccessfulDeletion(createRandomFileInDirectory(basePath), false); + void testOutputStreamOverwriteReplacesOldContent() throws IOException { + Path file = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3, 4, 5}); + + writeBytes(file, new byte[] {8, 9}, true); + + assertThat(readBytes(file)).containsExactly(8, 9); } @Test - void testExistingFileRecursiveDeletion() throws IOException { - testSuccessfulDeletion(createRandomFileInDirectory(basePath), true); + void testOutputStreamNoOverwriteFailsAndPreservesOldContent() throws IOException { + byte[] oldContent = new byte[] {1, 2, 3}; + Path file = createRandomFileInDirectory(basePath, oldContent); + + assertThatThrownBy(() -> writeBytes(file, new byte[] {9, 8, 7}, false)) + .isInstanceOf(IOException.class); + assertThat(readBytes(file)).containsExactly(oldContent); + } + + // ------------------------------------------------------------------------ + // File status and existence + // ------------------------------------------------------------------------ + + @Test + void testGetFileStatusForMissingPathThrowsFileNotFound() { + Path missing = new Path(basePath, randomName()); + + assertThatThrownBy(() -> fs.getFileStatus(missing)) + .isInstanceOf(FileNotFoundException.class); } @Test - void testNotExistingFileDeletion() throws IOException { - testSuccessfulDeletion(new Path(basePath, randomName()), false); + void testGetFileStatusDescribesFile() throws IOException { + byte[] content = new byte[] {2, 4, 6, 8, 10}; + Path file = createRandomFileInDirectory(basePath, content); + + FileStatus status = fs.getFileStatus(file); + + assertThat(status.getPath()).isEqualTo(file); + assertThat(status.isDir()).isFalse(); + assertThat(status.getLen()).isEqualTo(content.length); } @Test - void testNotExistingFileRecursiveDeletion() throws IOException { - testSuccessfulDeletion(new Path(basePath, randomName()), true); + void testGetFileStatusDescribesDirectory() throws IOException { + Path directory = new Path(basePath, randomName()); + fs.mkdirs(directory); + + FileStatus status = fs.getFileStatus(directory); + + assertThat(status.getPath()).isEqualTo(directory); + assertThat(status.isDir()).isTrue(); } @Test - void testExistingEmptyDirectoryDeletion() throws IOException { - final Path path = new Path(basePath, randomName()); - fs.mkdirs(path); - testSuccessfulDeletion(path, false); + void testFileStatusIsSnapshot() throws IOException { + Path path = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3}); + FileStatus snapshot = fs.getFileStatus(path); + + assertThat(fs.delete(path, false)).isTrue(); + assertThat(fs.mkdirs(path)).isTrue(); + FileStatus current = fs.getFileStatus(path); + + assertThat(snapshot.getPath()).isEqualTo(path); + assertThat(snapshot.isDir()).isFalse(); + assertThat(snapshot.getLen()).isEqualTo(3); + assertThat(current.getPath()).isEqualTo(path); + assertThat(current.isDir()).isTrue(); } @Test - void testExistingEmptyDirectoryRecursiveDeletion() throws IOException { - final Path path = new Path(basePath, randomName()); - fs.mkdirs(path); - testSuccessfulDeletion(path, true); + void testExistsReturnsTrueForFile() throws IOException { + Path file = createRandomFileInDirectory(basePath); + + assertThat(fs.exists(file)).isTrue(); } - private void testSuccessfulDeletion(Path path, boolean recursionEnabled) throws IOException { - fs.delete(path, recursionEnabled); - assertThat(fs.exists(path)).isFalse(); + @Test + void testExistsReturnsTrueForLogicalDirectory() throws IOException { + Path directory = new Path(basePath, randomName()); + fs.mkdirs(directory); + + assertThat(fs.exists(directory)).isTrue(); + } + + @Test + void testExistsReturnsFalseForMissingPath() throws IOException { + assertThat(fs.exists(new Path(basePath, randomName()))).isFalse(); + } + + // ------------------------------------------------------------------------ + // Listings + // ------------------------------------------------------------------------ + + @Test + void testListStatusOfEmptyDirectoryReturnsNonNullEmptyArray() throws IOException { + FileStatus[] statuses = fs.listStatus(basePath); + + assertThat(statuses).isNotNull().isEmpty(); } @Test - void testExistingNonEmptyDirectoryDeletion() throws IOException { - final Path directoryPath = new Path(basePath, randomName()); - final Path filePath = createRandomFileInDirectory(directoryPath); + void testListStatusReturnsOnlyCorrectDirectChildren() throws IOException { + byte[] firstContent = new byte[] {1, 2, 3, 4}; + byte[] secondContent = new byte[] {5, 6}; + Path firstFile = createRandomFileInDirectory(basePath, firstContent); + Path secondFile = createRandomFileInDirectory(basePath, secondContent); + Path firstDirectory = new Path(basePath, randomName()); + Path secondDirectory = new Path(basePath, randomName()); + Path nestedDirectory = new Path(firstDirectory, randomName()); + createRandomFileInDirectory(nestedDirectory, new byte[] {9}); + fs.mkdirs(secondDirectory); + + FileStatus[] statuses = fs.listStatus(basePath); + + assertThat(statuses).isNotNull().hasSize(4); + assertThat(statuses) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstFile, secondFile, firstDirectory, secondDirectory); + FileStatus firstFileStatus = statusFor(statuses, firstFile); + assertThat(firstFileStatus.isDir()).isFalse(); + assertThat(firstFileStatus.getLen()).isEqualTo(firstContent.length); + FileStatus secondFileStatus = statusFor(statuses, secondFile); + assertThat(secondFileStatus.isDir()).isFalse(); + assertThat(secondFileStatus.getLen()).isEqualTo(secondContent.length); + assertThat(statusFor(statuses, firstDirectory).isDir()).isTrue(); + assertThat(statusFor(statuses, secondDirectory).isDir()).isTrue(); + } - assertThatThrownBy(() -> fs.delete(directoryPath, false)).isInstanceOf(IOException.class); - assertThat(fs.exists(directoryPath)).isTrue(); - assertThat(fs.exists(filePath)).isTrue(); + @Test + void testListFilesNonRecursiveReturnsOnlyDirectFilesAndMatchesIterator() throws IOException { + Path firstDirectFile = createRandomFileInDirectory(basePath); + Path secondDirectFile = createRandomFileInDirectory(basePath); + Path directory = new Path(basePath, randomName()); + Path nestedFile = createRandomFileInDirectory(directory); + + FileStatus[] arrayResult = fs.listFiles(basePath, false); + List iteratorResult = collect(fs.listFilesIterative(basePath, false)); + + assertThat(arrayResult).isNotNull(); + assertThat(arrayResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); + assertThat(iteratorResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); + assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); + assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); + assertThat(arrayResult).noneMatch(status -> status.getPath().equals(nestedFile)); } @Test - void testExistingNonEmptyDirectoryRecursiveDeletion() throws IOException { - final Path directoryPath = new Path(basePath, randomName()); - final Path filePath = createRandomFileInDirectory(directoryPath); + void testListFilesRecursiveReturnsAllFilesAndMatchesIterator() throws IOException { + Path firstDirectFile = createRandomFileInDirectory(basePath); + Path secondDirectFile = createRandomFileInDirectory(basePath); + Path firstLevelDirectory = new Path(basePath, randomName()); + Path firstLevelFile = createRandomFileInDirectory(firstLevelDirectory); + Path secondLevelDirectory = new Path(firstLevelDirectory, randomName()); + Path secondLevelFile = createRandomFileInDirectory(secondLevelDirectory); + + FileStatus[] arrayResult = fs.listFiles(basePath, true); + List iteratorResult = collect(fs.listFilesIterative(basePath, true)); + + assertThat(arrayResult).isNotNull(); + assertThat(arrayResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder( + firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); + assertThat(iteratorResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder( + firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); + assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); + assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); + } - fs.delete(directoryPath, true); - assertThat(fs.exists(directoryPath)).isFalse(); - assertThat(fs.exists(filePath)).isFalse(); + @Test + void testListDirectoriesReturnsOnlyDirectDirectories() throws IOException { + createRandomFileInDirectory(basePath); + Path firstDirectDirectory = new Path(basePath, randomName()); + Path secondDirectDirectory = new Path(basePath, randomName()); + Path nestedDirectory = new Path(firstDirectDirectory, randomName()); + fs.mkdirs(nestedDirectory); + fs.mkdirs(secondDirectDirectory); + + FileStatus[] statuses = fs.listDirectories(basePath); + + assertThat(statuses).isNotNull().hasSize(2); + assertThat(statuses) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectDirectory, secondDirectDirectory); + assertThat(Arrays.stream(statuses).allMatch(FileStatus::isDir)).isTrue(); } + // ------------------------------------------------------------------------ + // Delete + // ------------------------------------------------------------------------ + @Test - void testExistingNonEmptyDirectoryWithSubDirRecursiveDeletion() throws IOException { - final Path level1SubDirWithFile = new Path(basePath, randomName()); - final Path fileInLevel1Subdir = createRandomFileInDirectory(level1SubDirWithFile); - final Path level2SubDirWithFile = new Path(level1SubDirWithFile, randomName()); - final Path fileInLevel2Subdir = createRandomFileInDirectory(level2SubDirWithFile); + void testExistingFileDeletion() throws IOException { + Path file = createRandomFileInDirectory(basePath); + + assertThat(fs.delete(file, false)).isTrue(); - testSuccessfulDeletion(level1SubDirWithFile, true); - assertThat(fs.exists(fileInLevel1Subdir)).isFalse(); - assertThat(fs.exists(level2SubDirWithFile)).isFalse(); - assertThat(fs.exists(fileInLevel2Subdir)).isFalse(); + assertThat(fs.exists(file)).isFalse(); } - // --- mkdirs + @Test + void testExistingFileRecursiveDeletion() throws IOException { + Path file = createRandomFileInDirectory(basePath); + + assertThat(fs.delete(file, true)).isTrue(); + + assertThat(fs.exists(file)).isFalse(); + } @Test - void testMkdirsReturnsTrueWhenCreatingDirectory() throws Exception { - // this test applies to object stores as well, as rely on the fact that they - // return true when things are not bad + void testExistingEmptyDirectoryDeletion() throws IOException { + Path directory = new Path(basePath, randomName()); + fs.mkdirs(directory); - final Path directory = new Path(basePath, randomName()); - assertThat(fs.mkdirs(directory)).isTrue(); - assertThat(fs.exists(directory)).isTrue(); + assertThat(fs.delete(directory, false)).isTrue(); + + assertThat(fs.exists(directory)).isFalse(); } @Test - void testMkdirsCreatesParentDirectories() throws Exception { - // this test applies to object stores as well, as rely on the fact that they - // return true when things are not bad + void testExistingEmptyDirectoryRecursiveDeletion() throws IOException { + Path directory = new Path(basePath, randomName()); + fs.mkdirs(directory); - final Path directory = - new Path(new Path(new Path(basePath, randomName()), randomName()), randomName()); - assertThat(fs.mkdirs(directory)).isTrue(); + assertThat(fs.delete(directory, true)).isTrue(); + + assertThat(fs.exists(directory)).isFalse(); + } + + @Test + void testNonEmptyDirectoryNonRecursiveDeletionFailsWithoutDamage() throws IOException { + Path directory = new Path(basePath, randomName()); + Path file = createRandomFileInDirectory(directory); + assertThatThrownBy(() -> fs.delete(directory, false)).isInstanceOf(IOException.class); assertThat(fs.exists(directory)).isTrue(); + assertThat(fs.exists(file)).isTrue(); + } + + @Test + void testRecursiveDeletionRemovesEntireSubtree() throws IOException { + Path directory = new Path(basePath, randomName()); + Path directFile = createRandomFileInDirectory(directory); + Path nestedDirectory = new Path(directory, randomName()); + Path nestedFile = createRandomFileInDirectory(nestedDirectory); + + assertThat(fs.delete(directory, true)).isTrue(); + + assertThat(fs.exists(directory)).isFalse(); + assertThat(fs.exists(directFile)).isFalse(); + assertThat(fs.exists(nestedDirectory)).isFalse(); + assertThat(fs.exists(nestedFile)).isFalse(); } @Test - void testMkdirsReturnsTrueForExistingDirectory() throws Exception { - // this test applies to object stores as well, as rely on the fact that they - // return true when things are not bad + void testMissingPathDeletionLeavesPathAbsent() throws IOException { + Path missing = new Path(basePath, randomName()); - final Path directory = new Path(basePath, randomName()); + fs.delete(missing, false); - // make sure the directory exists - createRandomFileInDirectory(directory); + assertThat(fs.exists(missing)).isFalse(); + } + + @Test + void testMissingPathRecursiveDeletionLeavesPathAbsent() throws IOException { + Path missing = new Path(basePath, randomName()); + + fs.delete(missing, true); + + assertThat(fs.exists(missing)).isFalse(); + } + + // ------------------------------------------------------------------------ + // Mkdirs + // ------------------------------------------------------------------------ + + @Test + void testMkdirsCreatesTargetAndLogicalParents() throws IOException { + Path first = new Path(basePath, randomName()); + Path second = new Path(first, randomName()); + Path target = new Path(second, randomName()); + + assertThat(fs.mkdirs(target)).isTrue(); + assertThat(fs.getFileStatus(first).isDir()).isTrue(); + assertThat(fs.getFileStatus(second).isDir()).isTrue(); + assertThat(fs.getFileStatus(target).isDir()).isTrue(); + } + + @Test + void testMkdirsReturnsTrueForExistingDirectory() throws IOException { + Path directory = new Path(basePath, randomName()); + assertThat(fs.mkdirs(directory)).isTrue(); assertThat(fs.mkdirs(directory)).isTrue(); + assertThat(fs.getFileStatus(directory).isDir()).isTrue(); } + // ------------------------------------------------------------------------ + // Rename + // ------------------------------------------------------------------------ + @Test - protected void testMkdirsFailsForExistingFile() throws Exception { - final Path file = new Path(getBasePath(), randomName()); - createFile(file); + void testRenameFileMovesExactBytesToMissingDestination() throws IOException { + byte[] content = new byte[] {4, 2, 4, 2}; + Path source = createRandomFileInDirectory(basePath, content); + Path destination = new Path(basePath, randomName()); - try { - fs.mkdirs(file); - fail("should fail with an IOException"); - } catch (IOException e) { - // good! + assertThat(fs.rename(source, destination)).isTrue(); + + assertThat(fs.exists(source)).isFalse(); + assertThat(readBytes(destination)).containsExactly(content); + } + + @Test + void testRenameDirectoryMovesExactTreeToMissingDestination() throws IOException { + Path source = new Path(basePath, randomName()); + Path child = createRandomFileInDirectory(source, new byte[] {1, 2}); + Path nestedDirectory = new Path(source, randomName()); + Path nestedChild = createRandomFileInDirectory(nestedDirectory, new byte[] {3, 4, 5}); + Path destination = new Path(basePath, randomName()); + + assertThat(fs.rename(source, destination)).isTrue(); + + assertThat(fs.exists(source)).isFalse(); + assertThat(fs.exists(child)).isFalse(); + assertThat(fs.exists(nestedDirectory)).isFalse(); + assertThat(fs.exists(nestedChild)).isFalse(); + assertThat(readBytes(new Path(destination, child.getName()))).containsExactly(1, 2); + assertThat( + readBytes( + new Path( + new Path(destination, nestedDirectory.getName()), + nestedChild.getName()))) + .containsExactly(3, 4, 5); + } + + // ------------------------------------------------------------------------ + // Copy + // ------------------------------------------------------------------------ + + @Test + void testCopyFileCreatesDestinationWithSourceBytes() throws IOException { + byte[] content = new byte[] {6, 2, 6, 4, 3}; + Path source = createRandomFileInDirectory(basePath, content); + Path destination = new Path(basePath, randomName()); + + fs.copyFile(source, destination, false); + + assertThat(readBytes(destination)).containsExactly(content); + assertThat(readBytes(source)).containsExactly(content); + } + + @Test + void testCopyFileOverwriteReplacesDestination() throws IOException { + byte[] content = new byte[] {7, 7}; + Path source = createRandomFileInDirectory(basePath, content); + Path destination = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3, 4}); + + fs.copyFile(source, destination, true); + + assertThat(readBytes(destination)).containsExactly(content); + assertThat(readBytes(source)).containsExactly(content); + } + + @Test + void testCopyFileNoOverwriteFailsAndPreservesDestination() throws IOException { + byte[] sourceContent = new byte[] {9, 9}; + Path source = createRandomFileInDirectory(basePath, sourceContent); + byte[] destinationContent = new byte[] {1, 2, 3}; + Path destination = createRandomFileInDirectory(basePath, destinationContent); + + assertThatThrownBy(() -> fs.copyFile(source, destination, false)) + .isInstanceOf(IOException.class); + assertThat(readBytes(destination)).containsExactly(destinationContent); + assertThat(readBytes(source)).containsExactly(sourceContent); + } + + @Test + void testCopyFilesCopiesEveryDirectFile() throws IOException { + Path sourceDirectory = new Path(basePath, randomName()); + Path first = createRandomFileInDirectory(sourceDirectory, new byte[] {1, 3}); + Path second = createRandomFileInDirectory(sourceDirectory, new byte[] {2, 4, 6}); + Path targetDirectory = new Path(basePath, randomName()); + fs.mkdirs(targetDirectory); + + fs.copyFiles(sourceDirectory, targetDirectory, false); + + assertThat(readBytes(new Path(targetDirectory, first.getName()))).containsExactly(1, 3); + assertThat(readBytes(new Path(targetDirectory, second.getName()))).containsExactly(2, 4, 6); + assertThat(readBytes(first)).containsExactly(1, 3); + assertThat(readBytes(second)).containsExactly(2, 4, 6); + } + + // ------------------------------------------------------------------------ + // Two-phase output + // ------------------------------------------------------------------------ + + @Test + void testTwoPhaseOutputPublishesOnlyAfterCommit() throws IOException { + Path target = new Path(basePath, randomName()); + byte[] content = new byte[] {5, 4, 3, 2, 1}; + TwoPhaseOutputStream.Committer committer; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + assertThat(out.getPos()).isZero(); + out.write(content); + assertThat(out.getPos()).isEqualTo(content.length); + assertThat(fs.exists(target)).isFalse(); + committer = out.closeForCommit(); } + + assertThat(committer.targetPath()).isEqualTo(target); + assertThat(fs.exists(target)).isFalse(); + + committer.commit(fs); + assertThat(readBytes(target)).containsExactly(content); } @Test - void testMkdirsFailsWithExistingParentFile() throws Exception { - final Path file = new Path(getBasePath(), randomName()); - createFile(file); + void testTwoPhaseDiscardDoesNotPublishAbandonedData() throws IOException { + Path target = new Path(basePath, randomName()); + TwoPhaseOutputStream.Committer committer; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + out.write(new byte[] {1, 2, 3}); + committer = out.closeForCommit(); + } - final Path dirUnderFile = new Path(file, randomName()); - try { - fs.mkdirs(dirUnderFile); - fail("should fail with an IOException"); - } catch (IOException e) { - // good! + committer.discard(fs); + + assertThat(fs.exists(target)).isFalse(); + } + + @Test + void testTwoPhaseDiscardPreservesPreExistingTarget() throws IOException { + byte[] oldContent = new byte[] {8, 6, 7, 5}; + Path target = new Path(basePath, randomName()); + TwoPhaseOutputStream.Committer committer; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + out.write(new byte[] {3, 0, 9}); + committer = out.closeForCommit(); + } + assertThat(fs.exists(target)).isFalse(); + + writeBytes(target, oldContent, false); + assertThat(readBytes(target)).containsExactly(oldContent); + + committer.discard(fs); + + assertThat(fs.exists(target)).isTrue(); + assertThat(readBytes(target)).containsExactly(oldContent); + } + + @Test + void testTwoPhaseDiscardDoesNotAffectAnotherWriter() throws IOException { + Path target = new Path(basePath, randomName()); + TwoPhaseOutputStream.Committer abandoned; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + out.write(new byte[] {1, 1, 1}); + abandoned = out.closeForCommit(); + } + + byte[] committedContent = new byte[] {2, 2, 2}; + TwoPhaseOutputStream.Committer successful; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + out.write(committedContent); + successful = out.closeForCommit(); + } + + abandoned.discard(fs); + successful.commit(fs); + + assertThat(readBytes(target)).containsExactly(committedContent); + } + + @Test + void testTwoPhaseCleanPreservesCommittedTarget() throws IOException { + byte[] content = new byte[] {2, 7, 1, 8}; + Path target = new Path(basePath, randomName()); + TwoPhaseOutputStream.Committer committer; + try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { + out.write(content); + committer = out.closeForCommit(); } + committer.commit(fs); + + committer.clean(fs); + + assertThat(readBytes(target)).containsExactly(content); } // ------------------------------------------------------------------------ @@ -268,17 +746,73 @@ protected static String randomName() { return StringUtils.getRandomString(RND, 16, 16, 'a', 'z'); } - private void createFile(Path file) throws IOException { - try (PositionOutputStream out = fs.newOutputStream(file, false)) { - out.write(new byte[] {1, 2, 3, 4, 5, 6, 7, 8}); + private void writeBytes(Path file, byte[] content, boolean overwrite) throws IOException { + try (PositionOutputStream out = fs.newOutputStream(file, overwrite)) { + out.write(content); + } + } + + private byte[] readBytes(Path file) throws IOException { + try (SeekableInputStream in = fs.newInputStream(file)) { + return readAll(in); + } + } + + private void assertOpenOrFirstReadFails(Path path) { + final SeekableInputStream in; + try { + in = fs.newInputStream(path); + } catch (IOException expectedAtOpen) { + return; + } + + try { + assertThatThrownBy(() -> in.read()).isInstanceOf(IOException.class); + } finally { + try { + in.close(); + } catch (IOException ignoredAtClose) { + // A close-only failure is deliberately irrelevant to the open/first-read contract. + } } } + private static byte[] readAll(SeekableInputStream in) throws IOException { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + byte[] buffer = new byte[4]; + int read; + while ((read = in.read(buffer, 0, buffer.length)) != -1) { + out.write(buffer, 0, read); + } + return out.toByteArray(); + } + + private static List collect(RemoteIterator iterator) + throws IOException { + List statuses = new ArrayList<>(); + while (iterator.hasNext()) { + statuses.add(iterator.next()); + } + return statuses; + } + + private static FileStatus statusFor(FileStatus[] statuses, Path path) { + for (FileStatus status : statuses) { + if (status.getPath().equals(path)) { + return status; + } + } + throw new AssertionError("No status for " + path); + } + private Path createRandomFileInDirectory(Path directory) throws IOException { - fs.mkdirs(directory); - final Path filePath = new Path(directory, randomName()); - createFile(filePath); + return createRandomFileInDirectory(directory, DEFAULT_CONTENT); + } - return filePath; + private Path createRandomFileInDirectory(Path directory, byte[] content) throws IOException { + fs.mkdirs(directory); + Path file = new Path(directory, randomName()); + writeBytes(file, content, false); + return file; } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java index 6f6ed70b1ad7..9b031a528060 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java @@ -22,12 +22,12 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.RawLocalFileSystem; -import org.apache.hadoop.util.VersionInfo; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import java.net.URI; -import static org.assertj.core.api.Assumptions.assumeThat; +import static org.assertj.core.api.Assertions.assertThat; /** Behavior tests for Hadoop Local. */ class HadoopLocalFileIOBehaviorTest extends FileIOBehaviorTestBase { @@ -48,18 +48,8 @@ protected Path getBasePath() { return new Path(tmp.toUri()); } - // ------------------------------------------------------------------------ - - /** This test needs to be skipped for earlier Hadoop versions because those have a bug. */ - @Override - protected void testMkdirsFailsForExistingFile() throws Exception { - final String versionString = VersionInfo.getVersion(); - final String prefix = versionString.substring(0, 3); - final float version = Float.parseFloat(prefix); - assumeThat(version) - .describedAs("Cannot execute this test on Hadoop prior to 2.8") - .isGreaterThanOrEqualTo(2.8f); - - super.testMkdirsFailsForExistingFile(); + @Test + void testIsObjectStoreReturnsFalse() throws Exception { + assertThat(getFileSystem().isObjectStore()).isFalse(); } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java index 2477bdcdbad1..8b308da2c478 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java @@ -20,8 +20,15 @@ import org.apache.paimon.fs.local.LocalFileIO; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.attribute.FileTime; + +import static org.assertj.core.api.Assertions.assertThat; + /** Test for {@link LocalFileIO}. */ public class LocalFileIOBehaviorTest extends FileIOBehaviorTestBase { @@ -36,4 +43,27 @@ protected FileIO getFileSystem() { protected Path getBasePath() { return new Path(tmp.toUri()); } + + @Test + void testIsObjectStoreReturnsFalse() { + assertThat(getFileSystem().isObjectStore()).isFalse(); + } + + @Test + void testFileStatusSnapshotsModificationTime() throws IOException { + java.nio.file.Path file = Files.createFile(tmp.resolve("snapshot")); + FileTime firstTimestamp = FileTime.fromMillis(1_000_000L); + FileTime secondTimestamp = FileTime.fromMillis(2_000_000L); + Files.setLastModifiedTime(file, firstTimestamp); + + FileIO fileIO = getFileSystem(); + Path path = new Path(file.toUri()); + FileStatus snapshot = fileIO.getFileStatus(path); + + Files.setLastModifiedTime(file, secondTimestamp); + + assertThat(snapshot.getModificationTime()).isEqualTo(firstTimestamp.toMillis()); + assertThat(fileIO.getFileStatus(path).getModificationTime()) + .isEqualTo(secondTimestamp.toMillis()); + } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java index 2fc4fef14fbc..dcb73cdc856b 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java @@ -129,21 +129,26 @@ void testCleanRemovesTheFileItStagedWhenThereWasNoCommit() throws IOException { } @Test - void testDiscard() throws IOException { + void testDiscardRemovesOnlyItsStagedFile() throws IOException { RenamingTwoPhaseOutputStream stream = new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + stream.write("abandoned".getBytes()); + TwoPhaseOutputStream.Committer committer = stream.closeForCommit(); - // Write some data - stream.write("Some data".getBytes()); + Path stagingDir = new Path(targetPath.getParent(), "_temporary"); + FileStatus[] stagedFiles = fileIO.listStatus(stagingDir); + assertThat(stagedFiles).hasSize(1); + Path stagedPath = stagedFiles[0].getPath(); - // Close for commit - TwoPhaseOutputStream.Committer committer = stream.closeForCommit(); + Path otherWriterPending = new Path(stagingDir, "attempt_0001_m_000010_15/part-00010"); + fileIO.writeFile(otherWriterPending, "concurrent", false); + fileIO.writeFile(targetPath, "published", false); - // Discard instead of commit committer.discard(fileIO); - // Target file should not exist - assertThat(fileIO.exists(targetPath)).isFalse(); + assertThat(fileIO.exists(stagedPath)).isFalse(); + assertThat(fileIO.exists(otherWriterPending)).isTrue(); + assertThat(fileIO.readFileUtf8(targetPath)).isEqualTo("published"); } @Test From 39fff42bd0273da5616e0c8748b3920605fa322e Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 14:18:36 +0800 Subject: [PATCH 03/11] [common] Isolate the FileIO contract suite --- .../java/org/apache/paimon/fs/FileIO.java | 193 ++--- .../java/org/apache/paimon/fs/FileStatus.java | 25 +- .../apache/paimon/fs/SeekableInputStream.java | 15 +- .../paimon/fs/TwoPhaseOutputStream.java | 42 +- .../apache/paimon/fs/local/LocalFileIO.java | 18 +- .../paimon/fs/FileIOBehaviorTestBase.java | 768 +++--------------- .../paimon/fs/FileIOContractTestBase.java | 651 +++++++++++++++ .../fs/HadoopLocalFileIOBehaviorTest.java | 22 +- .../apache/paimon/fs/HdfsBehaviorTest.java | 2 +- .../paimon/fs/LocalFileIOBehaviorTest.java | 32 +- .../fs/RenamingTwoPhaseOutputStreamTest.java | 18 + .../org/apache/paimon/s3/S3FileIOTest.java | 4 +- 12 files changed, 922 insertions(+), 868 deletions(-) create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java index cfeb99e442fa..2b0dcec3f760 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java @@ -59,9 +59,7 @@ import static org.apache.paimon.utils.Preconditions.checkArgument; /** - * Provider-neutral file I/O for files and logical directories. - * - *

Implementations are not required to materialize directory markers for logical directories. + * File IO to read and write file. * * @since 0.4.0 */ @@ -80,49 +78,37 @@ public interface FileIO extends Serializable, Closeable { default void setRuntimeContext(Map options) {} /** - * Opens a {@link SeekableInputStream} for a file. - * - *

The returned stream starts at position zero and supports seeking from zero through the - * file length, inclusive. Behavior for offsets outside that range is unspecified. If the path - * is missing or is a directory, this method or the first read from the returned stream throws - * an {@link IOException}. + * Opens an SeekableInputStream at the indicated Path. * * @param path the file to open - * @return a seekable stream for the file - * @throws IOException if the file cannot be read */ SeekableInputStream newInputStream(Path path) throws IOException; /** - * Opens a {@link PositionOutputStream} for a file. - * - *

When no ancestor is a file, missing logical parents are created. A successful close makes - * the complete written content visible. If the target exists, {@code overwrite=true} replaces - * it. With {@code overwrite=false}, the conflict may be reported while opening, writing, or - * closing the stream, and the existing content remains unchanged. + * Opens an PositionOutputStream at the indicated Path. * - * @param path the file to write - * @param overwrite whether to replace an existing file - * @return a stream whose position tracks the number of bytes written - * @throws IOException if the file cannot be written + * @param path the file name to open + * @param overwrite if a file with this name already exists, then if true, the file will be + * overwritten, and if false an error will be thrown. + * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because a + * file already exists at that path and the write mode indicates to not overwrite the file. */ PositionOutputStream newOutputStream(Path path, boolean overwrite) throws IOException; /** - * Opens a {@link TwoPhaseOutputStream} that stages data for later publication. + * Opens a TwoPhaseOutputStream at the indicated Path for transactional writing. * - *

Staged data is not published at the target before commit is invoked. A successful commit - * publishes the complete data; if commit fails, the target state is unspecified. If the target - * already exists, whether the request is rejected or replaces the target, when a rejection is - * reported, and whether replacement is atomic are not specified by this interface. An - * implementation may document stronger guarantees. The staging layout is also not specified. + *

This method creates a stream that supports transactional writing operations. The written + * data becomes visible only after calling commit on the returned committer from closeForCommit + * method. * * @param path the file target path - * @param overwrite requests replacement of an existing file; existing-target behavior is - * provider-specific - * @return a stream that stages data for the target - * @throws IOException if the stream cannot be created - * @throws UnsupportedOperationException if the file system does not support staged writes + * @param overwrite if a file with this name already exists, then if true, the file will be + * overwritten, and if false an error will be thrown. + * @return a TwoPhaseOutputStream that supports transactional writes + * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because a + * file already exists at that path and the write mode indicates to not overwrite the file. + * @throws UnsupportedOperationException if the filesystem does not support transactional writes */ default TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrite) throws IOException { @@ -130,37 +116,30 @@ default TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrit } /** - * Returns a metadata snapshot for a path. + * Return a file status object that represents the path. * - * @param path the path to inspect - * @return a snapshot of the path's status - * @throws FileNotFoundException if the path does not exist - * @throws IOException if the status cannot be read + * @param path The path we want information from + * @return a FileStatus object + * @throws FileNotFoundException when the path does not exist; IOException see specific + * implementation */ FileStatus getFileStatus(Path path) throws IOException; /** - * Lists the direct children of an existing directory. + * List the statuses of the files/directories in the given path if the path is a directory. * - *

The result is non-null and unordered. Each status has the child's path and type; file - * statuses also have the file length. Behavior for a missing path or a file path is - * unspecified. - * - * @param path an existing directory - * @return the direct child statuses, or an empty array for an empty directory + * @param path given path + * @return the statuses of the files/directories in the given path */ FileStatus[] listStatus(Path path) throws IOException; /** - * Lists files under an existing directory. - * - *

The result is non-null and unordered. It contains the same set of file paths as {@link - * #listFilesIterative(Path, boolean)} for the same arguments. Behavior for a missing path or a - * file path is unspecified. + * List the statuses of the files in the given path if the path is a directory. * - * @param path an existing directory - * @param recursive whether to descend into subdirectories - * @return only file statuses, recursively if requested + * @param path given path + * @param recursive if set to true will recursively list files in subdirectories, + * otherwise only files in the current directory will be listed + * @return the statuses of the files in the given path */ default FileStatus[] listFiles(Path path, boolean recursive) throws IOException { List files = new ArrayList<>(); @@ -172,15 +151,12 @@ default FileStatus[] listFiles(Path path, boolean recursive) throws IOException } /** - * Iterates over files under an existing directory. + * List the statuses of the files iteratively in the given path if the path is a directory. * - *

The iterator is non-null and unordered. It contains the same set of file paths as {@link - * #listFiles(Path, boolean)} for the same arguments. Behavior for a missing path or a file path - * is unspecified. - * - * @param path an existing directory - * @param recursive whether to descend into subdirectories - * @return an iterator containing only file statuses, recursively if requested + * @param path given path + * @param recursive if set to true will recursively list files in subdirectories, + * otherwise only files in the current directory will be listed + * @return an {@link RemoteIterator} over {@link FileStatus} of the files in the given path */ default RemoteIterator listFilesIterative(Path path, boolean recursive) throws IOException { @@ -219,13 +195,12 @@ private void maybeUnpackDirectory() throws IOException { } /** - * Lists the direct directories under an existing directory. + * List the statuses of the directories in the given path if the path is a directory. * - *

The result is non-null and unordered. Behavior for a missing path or a file path is - * unspecified. + *

{@link FileIO} implementation may have optimization for list directories. * - * @param path an existing directory - * @return only direct child directory statuses, or an empty array if there are none + * @param path given path + * @return the statuses of the directories in the given path */ default FileStatus[] listDirectories(Path path) throws IOException { FileStatus[] statuses = listStatus(path); @@ -236,52 +211,40 @@ default FileStatus[] listDirectories(Path path) throws IOException { } /** - * Checks whether a file or logical directory exists. + * Check if exists. * - * @param path the path to check - * @return whether the path exists + * @param path source file */ boolean exists(Path path) throws IOException; /** - * Deletes a file or directory. - * - *

An existing file is deleted for either value of {@code recursive}. An empty directory is - * deleted, and a non-empty directory is deleted with its subtree when {@code recursive} is - * true. These successful deletions return true. Deleting a non-empty directory with {@code - * recursive=false} throws an {@link IOException} and preserves the complete tree. A missing - * path does not throw solely because it is absent; its return value is unspecified. + * Delete a file. * * @param path the path to delete - * @param recursive whether to delete a directory subtree - * @return true when an existing target is successfully deleted; unspecified for a missing path + * @param recursive if path is a directory and set to true, the directory is + * deleted else throws an exception. In case of a file the recursive can be set to either + * true or false + * @return true if delete is successful, false otherwise */ boolean delete(Path path, boolean recursive) throws IOException; /** - * Makes a logical directory and any missing logical parents. + * Make the given file and all non-existent parents into directories. Has the semantics of Unix + * 'mkdir -p'. Existence of the directory hierarchy is not an error. * - *

When the target and its ancestors are not files, this method returns true and leaves them - * as directories. Repeating the call also returns true. Implementations need not materialize - * directory markers. - * - * @param path the directory to create - * @return true on successful creation or when the directory already exists - * @throws IOException if the directory cannot be created + * @param path the directory/directories to be created + * @return true if at least one new directory has been created, false + * otherwise + * @throws IOException thrown if an I/O error occurs while creating the directory */ boolean mkdirs(Path path) throws IOException; /** - * Renames a file or directory in the guaranteed non-conflicting case. - * - *

When the source exists, the exact destination is missing, and the destination parent is - * valid, this method returns true, removes the source, and preserves the file bytes or - * directory tree at the exact destination. Behavior in all other cases, including destination - * conflicts, is unspecified. Atomicity is not guaranteed. + * Renames the file/directory src to dst. * - * @param src the source file or directory - * @param dst the exact destination path - * @return true for the guaranteed case; otherwise unspecified + * @param src the file/directory to rename + * @param dst the new name of the file/directory + * @return true if the renaming was successful, false otherwise */ boolean rename(Path src, Path dst) throws IOException; @@ -382,13 +345,10 @@ default String readFileUtf8(Path path) throws IOException { } /** - * Writes content through a temporary file and then renames it to the target. + * Write content to one file atomically, initially writes to temp hidden file and only renames + * to the target file once temp file is closed. * - *

If the target is missing and its parent is valid, a true result guarantees the target has - * the requested content. This method does not add an atomicity or conflict guarantee beyond - * {@link #rename(Path, Path)}. - * - * @return whether the final rename reported success + * @return false if target file exists */ default boolean tryToWriteAtomic(Path path, String content) throws IOException { Path tmp = path.createTempPath(); @@ -413,7 +373,10 @@ default void writeFile(Path path, String content, boolean overwrite) throws IOEx } } - /** Overwrites a file with UTF-8 content without guaranteeing atomic replacement. */ + /** + * Overwrite file by content atomically, different {@link FileIO}s have different atomic + * implementations. + */ default void overwriteFileUtf8(Path path, String content) throws IOException { try (PositionOutputStream out = newOutputStream(path, true)) { OutputStreamWriter writer = new OutputStreamWriter(out, StandardCharsets.UTF_8); @@ -422,22 +385,21 @@ default void overwriteFileUtf8(Path path, String content) throws IOException { } } - /** Overwrites a hint file with UTF-8 content without guaranteeing atomic replacement. */ + /** + * Overwrite hint file by content atomically, the characteristic of Hint file is that it can not + * exist for a period of time, which allows some file systems to perform overwrite writing by + * deleting and renaming. + */ default void overwriteHintFile(Path path, String content) throws IOException { overwriteFileUtf8(path, content); } /** - * Copies the bytes of a source file to a target file. + * Copy content of one file into another. * - *

If the target exists, {@code overwrite=true} replaces it. With {@code overwrite=false}, - * the copy fails and preserves the existing target content. - * - * @param sourcePath the source file - * @param targetPath the target file - * @param overwrite whether to replace an existing target - * @throws IOException if the file cannot be copied or overwrite is disabled for an existing - * target + * @throws IOException Thrown, if the stream could not be opened because of an I/O, or because + * target file already exists at that path and the write mode indicates to not overwrite the + * file. */ default void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throws IOException { try (SeekableInputStream is = newInputStream(sourcePath); @@ -446,14 +408,7 @@ default void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throw } } - /** - * Copies every direct file from a source directory to a target directory according to {@link - * #copyFile(Path, Path, boolean)}. - * - *

{@code sourceDirectory} and {@code targetDirectory} must be directories, and each direct - * child of {@code sourceDirectory} must be a file. The method is not recursive and does not - * roll back files copied before a later failure. - */ + /** Copy all files in sourceDirectory to directory targetDirectory. */ default void copyFiles(Path sourceDirectory, Path targetDirectory, boolean overwrite) throws IOException { FileStatus[] fileStatuses = listStatus(sourceDirectory); diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java b/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java index 1a9868d10db5..c3e6cde9cf0b 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/FileStatus.java @@ -23,10 +23,7 @@ import javax.annotation.Nullable; /** - * A snapshot of provider-neutral metadata for a file or logical directory. - * - *

The values do not change when the underlying path changes. Owner and access time, as well as - * directory length and modification time, are not portable across file systems. + * Interface that represents the client side information for a file independent of the file system. * * @since 0.4.0 */ @@ -34,9 +31,9 @@ public interface FileStatus { /** - * Returns the file length in bytes. The value for a directory is not portable. + * Return the length of this file. * - * @return the file length in bytes + * @return the length of this file */ long getLen(); @@ -55,27 +52,27 @@ public interface FileStatus { Path getPath(); /** - * Returns the last modification time in milliseconds since the epoch. The value for a directory - * is not portable. + * Get the last modification time of the file. * - * @return the last modification time + * @return A long value representing the time the file was last modified, measured in + * milliseconds since the epoch (UTC January 1, 1970). */ long getModificationTime(); /** - * Returns the last access time in milliseconds since the epoch, if available. This value is not - * portable and may be zero. + * Get the last access time of the file. * - * @return the last access time, or zero when unavailable + * @return A long value representing the time the file was last accessed, measured in + * milliseconds since the epoch (UTC January 1, 1970). */ default long getAccessTime() { return 0; } /** - * Returns the owner, if available. This value is not portable. + * Returns the owner of this file. * - * @return the owner, or null when unavailable + * @return the owner of this file */ @Nullable default String getOwner() { diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java index 6216dbd21755..32be1d1139f6 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/SeekableInputStream.java @@ -24,7 +24,7 @@ import java.io.InputStream; /** - * An input stream with a queryable byte position and seek support. + * {@code SeekableInputStream} provides seek methods. * * @since 0.4.0 */ @@ -32,12 +32,11 @@ public abstract class SeekableInputStream extends InputStream { /** - * Seeks to an offset from the start of the file. After a successful return, {@link #getPos()} - * reports that offset and the next read begins there. Behavior for negative offsets or offsets - * past the end of the file is implementation-specific. + * Seek to the given offset from the start of the file. The next read() will be from that + * location. Can't seek past the end of the stream. * * @param desired the desired offset - * @throws IOException if seeking fails + * @throws IOException Thrown if an error occurred while seeking inside the input stream. */ public abstract void seek(long desired) throws IOException; @@ -51,9 +50,9 @@ public abstract class SeekableInputStream extends InputStream { public abstract long getPos() throws IOException; /** - * Reads up to {@code len} bytes into the array. - * - * @return the number of bytes read, or {@code -1} at the end of the file + * Reads up to len bytes of data from the input stream into an array of bytes. An + * attempt is made to read as many as len bytes, but a smaller number may be read. + * The number of bytes actually read is returned as an integer. */ public abstract int read(byte[] b, int off, int len) throws IOException; diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java index fc67fa02c04f..931969ec68cb 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/TwoPhaseOutputStream.java @@ -21,58 +21,48 @@ import java.io.IOException; import java.io.Serializable; -/** - * An output stream that stages data and produces a committer to publish it. - * - *

Staged data is not published at the target before commit is invoked. A successful commit makes - * the complete data visible but does not imply atomic replacement. If commit fails, the target - * state is unspecified. - */ +/** TwoPhaseOutputStream provides a way to write to a file and get a committer that can commit. */ public abstract class TwoPhaseOutputStream extends PositionOutputStream { - /** - * Closes the stream for writing and returns a committer for the staged data. + * Closes the stream for writing and returns a committer that can be used to make the written + * data visible. * - *

After this call, the stream must not be used for writing. The staged data remains - * unpublished until {@link Committer#commit(FileIO)} is invoked. + *

After calling this method, the stream should not be used for writing anymore. The returned + * committer can be used to commit the data or discard it. * - * @return a committer that can publish or discard the staged data + * @return A committer that can be used to commit the data * @throws IOException if an I/O error occurs during closing */ public abstract Committer closeForCommit() throws IOException; - /** A serializable handle that can publish or discard one stream's staged data. */ + /** A committer interface that can commit or discard the written data. */ public interface Committer extends Serializable { /** - * Publishes the complete staged data at {@link #targetPath()}. - * - *

A successful return makes the complete data visible but does not guarantee atomic - * replacement. If this method throws, the target state is unspecified. + * Commits the written data, making it visible. * * @throws IOException if an I/O error occurs during commit */ void commit(FileIO fileIO) throws IOException; /** - * Discards this write's staged data instead of publishing it. - * - *

Only resources created by this write may be removed. In particular, discard must not - * remove target content or resources created by another writer. + * Discards the written data, cleaning up any temporary files or resources. Called instead + * of {@link #commit} when the write is given up. * * @throws IOException if an I/O error occurs during discard */ void discard(FileIO fileIO) throws IOException; - /** Returns the path where a successful commit publishes the staged data. */ Path targetPath(); /** - * Releases staging resources that this write no longer needs after {@link #commit} - * succeeds. + * Releases what this committer staged and no longer needs, after {@link #commit} has + * succeeded. May do nothing. * - *

This method may do nothing. It may remove only resources created by this write and - * must not remove the committed target or resources owned by another writer. + *

Only resources this committer created itself. A staging directory is shared with every + * other writer of the same location, Paimon or not, and removing it is theirs to decide: + * finding it empty does not mean it is unused, because a writer that has just created it + * has not staged its file in it yet. * * @throws IOException if an I/O error occurs during cleaning */ diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java index 0bcca68dc3ff..e1e0403b13b5 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/local/LocalFileIO.java @@ -363,16 +363,14 @@ public void close() throws IOException { private static class LocalFileStatus implements FileStatus { - private final Path path; - private final boolean directory; + private final File file; private final long length; - private final long modificationTime; + private final String scheme; private LocalFileStatus(File file, String scheme) { - this.path = new Path(scheme + ":" + file.toURI().getPath()); - this.directory = file.isDirectory(); + this.file = file; this.length = file.length(); - this.modificationTime = file.lastModified(); + this.scheme = scheme; } @Override @@ -382,22 +380,22 @@ public long getLen() { @Override public boolean isDir() { - return directory; + return file.isDirectory(); } @Override public Path getPath() { - return path; + return new Path(scheme + ":" + file.toURI().getPath()); } @Override public long getModificationTime() { - return modificationTime; + return file.lastModified(); } @Override public String toString() { - return "{" + "path=" + path + ", directory=" + directory + ", length=" + length + '}'; + return "{" + "file=" + file + ", length=" + length + ", scheme='" + scheme + '\'' + '}'; } } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java index a4800639857f..0ad71039983a 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOBehaviorTestBase.java @@ -24,24 +24,20 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.io.ByteArrayOutputStream; -import java.io.FileNotFoundException; import java.io.IOException; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.Random; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.assertj.core.api.Assertions.fail; -/** Provider-neutral contract tests for {@link FileIO}. */ +/** Common tests for the behavior of {@link FileIO} methods. */ public abstract class FileIOBehaviorTestBase { private static final Random RND = new Random(); - private static final byte[] DEFAULT_CONTENT = new byte[] {1, 2, 3, 4, 5, 6, 7, 8}; - /** The cached file system instance. */ private FileIO fs; @@ -74,668 +70,194 @@ void cleanup() throws Exception { fs.delete(basePath, true); } - @Test - void testObjectStoreClassificationIsStable() throws IOException { - boolean objectStore = fs.isObjectStore(); - Path file = createRandomFileInDirectory(basePath); - - assertThat(fs.isObjectStore()).isEqualTo(objectStore); - - fs.delete(file, false); - assertThat(fs.isObjectStore()).isEqualTo(objectStore); - } - // ------------------------------------------------------------------------ - // Input streams + // Suite of Tests // ------------------------------------------------------------------------ - @Test - void testInputStreamStartsAtZeroAndReadsCorrectBytes() throws IOException { - byte[] content = new byte[] {3, 1, 4, 1, 5, 9}; - Path file = createRandomFileInDirectory(basePath, content); - - try (SeekableInputStream in = fs.newInputStream(file)) { - assertThat(in.getPos()).isZero(); - assertThat(readAll(in)).containsExactly(content); - assertThat(in.getPos()).isEqualTo(content.length); - } - } + // --- exists @Test - void testInputStreamBulkReadHonorsNonZeroBufferOffset() throws IOException { - byte[] content = new byte[] {11, 22, 33}; - Path file = createRandomFileInDirectory(basePath, content); - byte[] buffer = new byte[] {99, 98, 0, 0, 0, 97, 96}; - - try (SeekableInputStream in = fs.newInputStream(file)) { - int totalRead = 0; - while (totalRead < content.length) { - int read = in.read(buffer, 2 + totalRead, content.length - totalRead); - assertThat(read).isPositive(); - totalRead += read; - } - - assertThat(totalRead).isEqualTo(content.length); - assertThat(buffer).containsExactly(99, 98, 11, 22, 33, 97, 96); - assertThat(in.getPos()).isEqualTo(content.length); - } + void testFileExists() throws IOException { + final Path filePath = createRandomFileInDirectory(basePath); + assertThat(fs.exists(filePath)).isTrue(); } @Test - void testInputStreamsHaveIndependentPositions() throws IOException { - Path file = createRandomFileInDirectory(basePath, new byte[] {10, 20, 30}); - - try (SeekableInputStream first = fs.newInputStream(file); - SeekableInputStream second = fs.newInputStream(file)) { - assertThat(first.read()).isEqualTo(10); - assertThat(first.getPos()).isEqualTo(1); - assertThat(second.getPos()).isZero(); - assertThat(second.read()).isEqualTo(10); - assertThat(second.getPos()).isEqualTo(1); - - first.seek(2); - assertThat(first.read()).isEqualTo(30); - assertThat(second.read()).isEqualTo(20); - } - } - - @Test - void testInputStreamSeeksForwardAndBackward() throws IOException { - byte[] content = new byte[] {10, 20, 30, 40, 50, 60}; - Path file = createRandomFileInDirectory(basePath, content); - - try (SeekableInputStream in = fs.newInputStream(file)) { - in.seek(4); - assertThat(in.getPos()).isEqualTo(4); - assertThat(in.read()).isEqualTo(50); - - in.seek(1); - assertThat(in.getPos()).isEqualTo(1); - assertThat(in.read()).isEqualTo(20); - } + void testFileDoesNotExist() throws IOException { + assertThat(fs.exists(new Path(basePath, randomName()))).isFalse(); } - @Test - void testInputStreamReturnsEndOfFileAtFileLength() throws IOException { - byte[] content = new byte[] {7, 8, 9}; - Path file = createRandomFileInDirectory(basePath, content); - - try (SeekableInputStream in = fs.newInputStream(file)) { - in.seek(content.length); - assertThat(in.read()).isEqualTo(-1); - assertThat(in.read(new byte[2], 0, 2)).isEqualTo(-1); - } - } + // --- list files @Test - void testInputStreamCanSeekBackToStart() throws IOException { - byte[] content = new byte[] {7, 8, 9}; - Path file = createRandomFileInDirectory(basePath, content); - - try (SeekableInputStream in = fs.newInputStream(file)) { - assertThat(in.read()).isEqualTo(7); - in.seek(0); - assertThat(in.getPos()).isZero(); - assertThat(in.read()).isEqualTo(7); - } - } + void testListFilesIterativeNonRecursive() throws IOException { + Path fileA = createRandomFileInDirectory(basePath); + Path dirB = new Path(basePath, randomName()); + fs.mkdirs(dirB); + Path fileBC = createRandomFileInDirectory(dirB); - @Test - void testInputStreamSeeksForwardBeyondOneMebibyte() throws IOException { - int targetPosition = 1024 * 1024 + 17; - byte[] content = new byte[targetPosition + 1]; - content[targetPosition] = 42; - Path file = createRandomFileInDirectory(basePath, content); - - try (SeekableInputStream in = fs.newInputStream(file)) { - in.seek(targetPosition); - assertThat(in.getPos()).isEqualTo(targetPosition); - assertThat(in.read()).isEqualTo(42); + List allFiles = new ArrayList<>(); + RemoteIterator iter = fs.listFilesIterative(basePath, false); + while (iter.hasNext()) { + allFiles.add(iter.next()); } + assertThat(allFiles.size()).isEqualTo(1); + assertThat(allFiles.get(0).getPath()).isEqualTo(fileA); } @Test - void testInputStreamForMissingFileFailsByFirstRead() { - Path missing = new Path(basePath, randomName()); - - assertOpenOrFirstReadFails(missing); - } - - @Test - void testInputStreamForDirectoryFailsByFirstRead() throws IOException { - Path directory = new Path(basePath, randomName()); - fs.mkdirs(directory); - - assertOpenOrFirstReadFails(directory); - } - - // ------------------------------------------------------------------------ - // Output streams - // ------------------------------------------------------------------------ + void testListFilesIterativeRecursive() throws IOException { + Path fileA = createRandomFileInDirectory(basePath); + Path dirB = new Path(basePath, randomName()); + fs.mkdirs(dirB); + Path fileBC = createRandomFileInDirectory(dirB); - @Test - void testOutputStreamTracksPositionAndPublishesBytesOnClose() throws IOException { - Path file = new Path(basePath, randomName()); - - try (PositionOutputStream out = fs.newOutputStream(file, false)) { - assertThat(out.getPos()).isZero(); - out.write(9); - assertThat(out.getPos()).isEqualTo(1); - out.write(new byte[] {10, 11, 12, 13}, 1, 2); - assertThat(out.getPos()).isEqualTo(3); + List allFiles = new ArrayList<>(); + RemoteIterator iter = fs.listFilesIterative(basePath, true); + while (iter.hasNext()) { + allFiles.add(iter.next()); } - - assertThat(readBytes(file)).containsExactly(9, 11, 12); - } - - @Test - void testOutputStreamCreatesNestedTarget() throws IOException { - Path ancestor = new Path(basePath, randomName()); - Path parent = new Path(ancestor, randomName()); - Path file = new Path(parent, randomName()); - byte[] content = new byte[] {1, 3, 3, 7}; - - writeBytes(file, content, false); - - assertThat(readBytes(file)).containsExactly(content); - assertThat(fs.getFileStatus(ancestor).isDir()).isTrue(); - assertThat(fs.getFileStatus(parent).isDir()).isTrue(); - } - - @Test - void testOutputStreamOverwriteReplacesOldContent() throws IOException { - Path file = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3, 4, 5}); - - writeBytes(file, new byte[] {8, 9}, true); - - assertThat(readBytes(file)).containsExactly(8, 9); - } - - @Test - void testOutputStreamNoOverwriteFailsAndPreservesOldContent() throws IOException { - byte[] oldContent = new byte[] {1, 2, 3}; - Path file = createRandomFileInDirectory(basePath, oldContent); - - assertThatThrownBy(() -> writeBytes(file, new byte[] {9, 8, 7}, false)) - .isInstanceOf(IOException.class); - assertThat(readBytes(file)).containsExactly(oldContent); - } - - // ------------------------------------------------------------------------ - // File status and existence - // ------------------------------------------------------------------------ - - @Test - void testGetFileStatusForMissingPathThrowsFileNotFound() { - Path missing = new Path(basePath, randomName()); - - assertThatThrownBy(() -> fs.getFileStatus(missing)) - .isInstanceOf(FileNotFoundException.class); - } - - @Test - void testGetFileStatusDescribesFile() throws IOException { - byte[] content = new byte[] {2, 4, 6, 8, 10}; - Path file = createRandomFileInDirectory(basePath, content); - - FileStatus status = fs.getFileStatus(file); - - assertThat(status.getPath()).isEqualTo(file); - assertThat(status.isDir()).isFalse(); - assertThat(status.getLen()).isEqualTo(content.length); - } - - @Test - void testGetFileStatusDescribesDirectory() throws IOException { - Path directory = new Path(basePath, randomName()); - fs.mkdirs(directory); - - FileStatus status = fs.getFileStatus(directory); - - assertThat(status.getPath()).isEqualTo(directory); - assertThat(status.isDir()).isTrue(); - } - - @Test - void testFileStatusIsSnapshot() throws IOException { - Path path = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3}); - FileStatus snapshot = fs.getFileStatus(path); - - assertThat(fs.delete(path, false)).isTrue(); - assertThat(fs.mkdirs(path)).isTrue(); - FileStatus current = fs.getFileStatus(path); - - assertThat(snapshot.getPath()).isEqualTo(path); - assertThat(snapshot.isDir()).isFalse(); - assertThat(snapshot.getLen()).isEqualTo(3); - assertThat(current.getPath()).isEqualTo(path); - assertThat(current.isDir()).isTrue(); - } - - @Test - void testExistsReturnsTrueForFile() throws IOException { - Path file = createRandomFileInDirectory(basePath); - - assertThat(fs.exists(file)).isTrue(); - } - - @Test - void testExistsReturnsTrueForLogicalDirectory() throws IOException { - Path directory = new Path(basePath, randomName()); - fs.mkdirs(directory); - - assertThat(fs.exists(directory)).isTrue(); - } - - @Test - void testExistsReturnsFalseForMissingPath() throws IOException { - assertThat(fs.exists(new Path(basePath, randomName()))).isFalse(); - } - - // ------------------------------------------------------------------------ - // Listings - // ------------------------------------------------------------------------ - - @Test - void testListStatusOfEmptyDirectoryReturnsNonNullEmptyArray() throws IOException { - FileStatus[] statuses = fs.listStatus(basePath); - - assertThat(statuses).isNotNull().isEmpty(); - } - - @Test - void testListStatusReturnsOnlyCorrectDirectChildren() throws IOException { - byte[] firstContent = new byte[] {1, 2, 3, 4}; - byte[] secondContent = new byte[] {5, 6}; - Path firstFile = createRandomFileInDirectory(basePath, firstContent); - Path secondFile = createRandomFileInDirectory(basePath, secondContent); - Path firstDirectory = new Path(basePath, randomName()); - Path secondDirectory = new Path(basePath, randomName()); - Path nestedDirectory = new Path(firstDirectory, randomName()); - createRandomFileInDirectory(nestedDirectory, new byte[] {9}); - fs.mkdirs(secondDirectory); - - FileStatus[] statuses = fs.listStatus(basePath); - - assertThat(statuses).isNotNull().hasSize(4); - assertThat(statuses) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder(firstFile, secondFile, firstDirectory, secondDirectory); - FileStatus firstFileStatus = statusFor(statuses, firstFile); - assertThat(firstFileStatus.isDir()).isFalse(); - assertThat(firstFileStatus.getLen()).isEqualTo(firstContent.length); - FileStatus secondFileStatus = statusFor(statuses, secondFile); - assertThat(secondFileStatus.isDir()).isFalse(); - assertThat(secondFileStatus.getLen()).isEqualTo(secondContent.length); - assertThat(statusFor(statuses, firstDirectory).isDir()).isTrue(); - assertThat(statusFor(statuses, secondDirectory).isDir()).isTrue(); - } - - @Test - void testListFilesNonRecursiveReturnsOnlyDirectFilesAndMatchesIterator() throws IOException { - Path firstDirectFile = createRandomFileInDirectory(basePath); - Path secondDirectFile = createRandomFileInDirectory(basePath); - Path directory = new Path(basePath, randomName()); - Path nestedFile = createRandomFileInDirectory(directory); - - FileStatus[] arrayResult = fs.listFiles(basePath, false); - List iteratorResult = collect(fs.listFilesIterative(basePath, false)); - - assertThat(arrayResult).isNotNull(); - assertThat(arrayResult) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); - assertThat(iteratorResult) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); - assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); - assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); - assertThat(arrayResult).noneMatch(status -> status.getPath().equals(nestedFile)); - } - - @Test - void testListFilesRecursiveReturnsAllFilesAndMatchesIterator() throws IOException { - Path firstDirectFile = createRandomFileInDirectory(basePath); - Path secondDirectFile = createRandomFileInDirectory(basePath); - Path firstLevelDirectory = new Path(basePath, randomName()); - Path firstLevelFile = createRandomFileInDirectory(firstLevelDirectory); - Path secondLevelDirectory = new Path(firstLevelDirectory, randomName()); - Path secondLevelFile = createRandomFileInDirectory(secondLevelDirectory); - - FileStatus[] arrayResult = fs.listFiles(basePath, true); - List iteratorResult = collect(fs.listFilesIterative(basePath, true)); - - assertThat(arrayResult).isNotNull(); - assertThat(arrayResult) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder( - firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); - assertThat(iteratorResult) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder( - firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); - assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); - assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); + assertThat(allFiles.size()).isEqualTo(2); + assertThat(allFiles.stream().filter(f -> f.getPath().equals(fileA)).count()).isEqualTo(1); + assertThat(allFiles.stream().filter(f -> f.getPath().equals(fileBC)).count()).isEqualTo(1); } - @Test - void testListDirectoriesReturnsOnlyDirectDirectories() throws IOException { - createRandomFileInDirectory(basePath); - Path firstDirectDirectory = new Path(basePath, randomName()); - Path secondDirectDirectory = new Path(basePath, randomName()); - Path nestedDirectory = new Path(firstDirectDirectory, randomName()); - fs.mkdirs(nestedDirectory); - fs.mkdirs(secondDirectDirectory); - - FileStatus[] statuses = fs.listDirectories(basePath); - - assertThat(statuses).isNotNull().hasSize(2); - assertThat(statuses) - .extracting(FileStatus::getPath) - .containsExactlyInAnyOrder(firstDirectDirectory, secondDirectDirectory); - assertThat(Arrays.stream(statuses).allMatch(FileStatus::isDir)).isTrue(); - } - - // ------------------------------------------------------------------------ - // Delete - // ------------------------------------------------------------------------ + // --- delete @Test void testExistingFileDeletion() throws IOException { - Path file = createRandomFileInDirectory(basePath); - - assertThat(fs.delete(file, false)).isTrue(); - - assertThat(fs.exists(file)).isFalse(); + testSuccessfulDeletion(createRandomFileInDirectory(basePath), false); } @Test void testExistingFileRecursiveDeletion() throws IOException { - Path file = createRandomFileInDirectory(basePath); - - assertThat(fs.delete(file, true)).isTrue(); - - assertThat(fs.exists(file)).isFalse(); + testSuccessfulDeletion(createRandomFileInDirectory(basePath), true); } @Test - void testExistingEmptyDirectoryDeletion() throws IOException { - Path directory = new Path(basePath, randomName()); - fs.mkdirs(directory); - - assertThat(fs.delete(directory, false)).isTrue(); - - assertThat(fs.exists(directory)).isFalse(); + void testNotExistingFileDeletion() throws IOException { + testSuccessfulDeletion(new Path(basePath, randomName()), false); } @Test - void testExistingEmptyDirectoryRecursiveDeletion() throws IOException { - Path directory = new Path(basePath, randomName()); - fs.mkdirs(directory); - - assertThat(fs.delete(directory, true)).isTrue(); - - assertThat(fs.exists(directory)).isFalse(); + void testNotExistingFileRecursiveDeletion() throws IOException { + testSuccessfulDeletion(new Path(basePath, randomName()), true); } @Test - void testNonEmptyDirectoryNonRecursiveDeletionFailsWithoutDamage() throws IOException { - Path directory = new Path(basePath, randomName()); - Path file = createRandomFileInDirectory(directory); - - assertThatThrownBy(() -> fs.delete(directory, false)).isInstanceOf(IOException.class); - assertThat(fs.exists(directory)).isTrue(); - assertThat(fs.exists(file)).isTrue(); + void testExistingEmptyDirectoryDeletion() throws IOException { + final Path path = new Path(basePath, randomName()); + fs.mkdirs(path); + testSuccessfulDeletion(path, false); } @Test - void testRecursiveDeletionRemovesEntireSubtree() throws IOException { - Path directory = new Path(basePath, randomName()); - Path directFile = createRandomFileInDirectory(directory); - Path nestedDirectory = new Path(directory, randomName()); - Path nestedFile = createRandomFileInDirectory(nestedDirectory); - - assertThat(fs.delete(directory, true)).isTrue(); - - assertThat(fs.exists(directory)).isFalse(); - assertThat(fs.exists(directFile)).isFalse(); - assertThat(fs.exists(nestedDirectory)).isFalse(); - assertThat(fs.exists(nestedFile)).isFalse(); + void testExistingEmptyDirectoryRecursiveDeletion() throws IOException { + final Path path = new Path(basePath, randomName()); + fs.mkdirs(path); + testSuccessfulDeletion(path, true); } - @Test - void testMissingPathDeletionLeavesPathAbsent() throws IOException { - Path missing = new Path(basePath, randomName()); - - fs.delete(missing, false); - - assertThat(fs.exists(missing)).isFalse(); + private void testSuccessfulDeletion(Path path, boolean recursionEnabled) throws IOException { + fs.delete(path, recursionEnabled); + assertThat(fs.exists(path)).isFalse(); } @Test - void testMissingPathRecursiveDeletionLeavesPathAbsent() throws IOException { - Path missing = new Path(basePath, randomName()); + void testExistingNonEmptyDirectoryDeletion() throws IOException { + final Path directoryPath = new Path(basePath, randomName()); + final Path filePath = createRandomFileInDirectory(directoryPath); - fs.delete(missing, true); - - assertThat(fs.exists(missing)).isFalse(); + assertThatThrownBy(() -> fs.delete(directoryPath, false)).isInstanceOf(IOException.class); + assertThat(fs.exists(directoryPath)).isTrue(); + assertThat(fs.exists(filePath)).isTrue(); } - // ------------------------------------------------------------------------ - // Mkdirs - // ------------------------------------------------------------------------ - @Test - void testMkdirsCreatesTargetAndLogicalParents() throws IOException { - Path first = new Path(basePath, randomName()); - Path second = new Path(first, randomName()); - Path target = new Path(second, randomName()); - - assertThat(fs.mkdirs(target)).isTrue(); - assertThat(fs.getFileStatus(first).isDir()).isTrue(); - assertThat(fs.getFileStatus(second).isDir()).isTrue(); - assertThat(fs.getFileStatus(target).isDir()).isTrue(); - } + void testExistingNonEmptyDirectoryRecursiveDeletion() throws IOException { + final Path directoryPath = new Path(basePath, randomName()); + final Path filePath = createRandomFileInDirectory(directoryPath); - @Test - void testMkdirsReturnsTrueForExistingDirectory() throws IOException { - Path directory = new Path(basePath, randomName()); - assertThat(fs.mkdirs(directory)).isTrue(); - - assertThat(fs.mkdirs(directory)).isTrue(); - assertThat(fs.getFileStatus(directory).isDir()).isTrue(); + fs.delete(directoryPath, true); + assertThat(fs.exists(directoryPath)).isFalse(); + assertThat(fs.exists(filePath)).isFalse(); } - // ------------------------------------------------------------------------ - // Rename - // ------------------------------------------------------------------------ - @Test - void testRenameFileMovesExactBytesToMissingDestination() throws IOException { - byte[] content = new byte[] {4, 2, 4, 2}; - Path source = createRandomFileInDirectory(basePath, content); - Path destination = new Path(basePath, randomName()); - - assertThat(fs.rename(source, destination)).isTrue(); + void testExistingNonEmptyDirectoryWithSubDirRecursiveDeletion() throws IOException { + final Path level1SubDirWithFile = new Path(basePath, randomName()); + final Path fileInLevel1Subdir = createRandomFileInDirectory(level1SubDirWithFile); + final Path level2SubDirWithFile = new Path(level1SubDirWithFile, randomName()); + final Path fileInLevel2Subdir = createRandomFileInDirectory(level2SubDirWithFile); - assertThat(fs.exists(source)).isFalse(); - assertThat(readBytes(destination)).containsExactly(content); + testSuccessfulDeletion(level1SubDirWithFile, true); + assertThat(fs.exists(fileInLevel1Subdir)).isFalse(); + assertThat(fs.exists(level2SubDirWithFile)).isFalse(); + assertThat(fs.exists(fileInLevel2Subdir)).isFalse(); } - @Test - void testRenameDirectoryMovesExactTreeToMissingDestination() throws IOException { - Path source = new Path(basePath, randomName()); - Path child = createRandomFileInDirectory(source, new byte[] {1, 2}); - Path nestedDirectory = new Path(source, randomName()); - Path nestedChild = createRandomFileInDirectory(nestedDirectory, new byte[] {3, 4, 5}); - Path destination = new Path(basePath, randomName()); - - assertThat(fs.rename(source, destination)).isTrue(); - - assertThat(fs.exists(source)).isFalse(); - assertThat(fs.exists(child)).isFalse(); - assertThat(fs.exists(nestedDirectory)).isFalse(); - assertThat(fs.exists(nestedChild)).isFalse(); - assertThat(readBytes(new Path(destination, child.getName()))).containsExactly(1, 2); - assertThat( - readBytes( - new Path( - new Path(destination, nestedDirectory.getName()), - nestedChild.getName()))) - .containsExactly(3, 4, 5); - } - - // ------------------------------------------------------------------------ - // Copy - // ------------------------------------------------------------------------ + // --- mkdirs @Test - void testCopyFileCreatesDestinationWithSourceBytes() throws IOException { - byte[] content = new byte[] {6, 2, 6, 4, 3}; - Path source = createRandomFileInDirectory(basePath, content); - Path destination = new Path(basePath, randomName()); - - fs.copyFile(source, destination, false); + void testMkdirsReturnsTrueWhenCreatingDirectory() throws Exception { + // this test applies to object stores as well, as rely on the fact that they + // return true when things are not bad - assertThat(readBytes(destination)).containsExactly(content); - assertThat(readBytes(source)).containsExactly(content); + final Path directory = new Path(basePath, randomName()); + assertThat(fs.mkdirs(directory)).isTrue(); + assertThat(fs.exists(directory)).isTrue(); } @Test - void testCopyFileOverwriteReplacesDestination() throws IOException { - byte[] content = new byte[] {7, 7}; - Path source = createRandomFileInDirectory(basePath, content); - Path destination = createRandomFileInDirectory(basePath, new byte[] {1, 2, 3, 4}); - - fs.copyFile(source, destination, true); + void testMkdirsCreatesParentDirectories() throws Exception { + // this test applies to object stores as well, as rely on the fact that they + // return true when things are not bad - assertThat(readBytes(destination)).containsExactly(content); - assertThat(readBytes(source)).containsExactly(content); - } + final Path directory = + new Path(new Path(new Path(basePath, randomName()), randomName()), randomName()); + assertThat(fs.mkdirs(directory)).isTrue(); - @Test - void testCopyFileNoOverwriteFailsAndPreservesDestination() throws IOException { - byte[] sourceContent = new byte[] {9, 9}; - Path source = createRandomFileInDirectory(basePath, sourceContent); - byte[] destinationContent = new byte[] {1, 2, 3}; - Path destination = createRandomFileInDirectory(basePath, destinationContent); - - assertThatThrownBy(() -> fs.copyFile(source, destination, false)) - .isInstanceOf(IOException.class); - assertThat(readBytes(destination)).containsExactly(destinationContent); - assertThat(readBytes(source)).containsExactly(sourceContent); + assertThat(fs.exists(directory)).isTrue(); } @Test - void testCopyFilesCopiesEveryDirectFile() throws IOException { - Path sourceDirectory = new Path(basePath, randomName()); - Path first = createRandomFileInDirectory(sourceDirectory, new byte[] {1, 3}); - Path second = createRandomFileInDirectory(sourceDirectory, new byte[] {2, 4, 6}); - Path targetDirectory = new Path(basePath, randomName()); - fs.mkdirs(targetDirectory); - - fs.copyFiles(sourceDirectory, targetDirectory, false); - - assertThat(readBytes(new Path(targetDirectory, first.getName()))).containsExactly(1, 3); - assertThat(readBytes(new Path(targetDirectory, second.getName()))).containsExactly(2, 4, 6); - assertThat(readBytes(first)).containsExactly(1, 3); - assertThat(readBytes(second)).containsExactly(2, 4, 6); - } + void testMkdirsReturnsTrueForExistingDirectory() throws Exception { + // this test applies to object stores as well, as rely on the fact that they + // return true when things are not bad - // ------------------------------------------------------------------------ - // Two-phase output - // ------------------------------------------------------------------------ - - @Test - void testTwoPhaseOutputPublishesOnlyAfterCommit() throws IOException { - Path target = new Path(basePath, randomName()); - byte[] content = new byte[] {5, 4, 3, 2, 1}; - TwoPhaseOutputStream.Committer committer; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - assertThat(out.getPos()).isZero(); - out.write(content); - assertThat(out.getPos()).isEqualTo(content.length); - assertThat(fs.exists(target)).isFalse(); - committer = out.closeForCommit(); - } + final Path directory = new Path(basePath, randomName()); - assertThat(committer.targetPath()).isEqualTo(target); - assertThat(fs.exists(target)).isFalse(); + // make sure the directory exists + createRandomFileInDirectory(directory); - committer.commit(fs); - assertThat(readBytes(target)).containsExactly(content); + assertThat(fs.mkdirs(directory)).isTrue(); } @Test - void testTwoPhaseDiscardDoesNotPublishAbandonedData() throws IOException { - Path target = new Path(basePath, randomName()); - TwoPhaseOutputStream.Committer committer; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - out.write(new byte[] {1, 2, 3}); - committer = out.closeForCommit(); - } - - committer.discard(fs); + protected void testMkdirsFailsForExistingFile() throws Exception { + final Path file = new Path(getBasePath(), randomName()); + createFile(file); - assertThat(fs.exists(target)).isFalse(); - } - - @Test - void testTwoPhaseDiscardPreservesPreExistingTarget() throws IOException { - byte[] oldContent = new byte[] {8, 6, 7, 5}; - Path target = new Path(basePath, randomName()); - TwoPhaseOutputStream.Committer committer; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - out.write(new byte[] {3, 0, 9}); - committer = out.closeForCommit(); + try { + fs.mkdirs(file); + fail("should fail with an IOException"); + } catch (IOException e) { + // good! } - assertThat(fs.exists(target)).isFalse(); - - writeBytes(target, oldContent, false); - assertThat(readBytes(target)).containsExactly(oldContent); - - committer.discard(fs); - - assertThat(fs.exists(target)).isTrue(); - assertThat(readBytes(target)).containsExactly(oldContent); } @Test - void testTwoPhaseDiscardDoesNotAffectAnotherWriter() throws IOException { - Path target = new Path(basePath, randomName()); - TwoPhaseOutputStream.Committer abandoned; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - out.write(new byte[] {1, 1, 1}); - abandoned = out.closeForCommit(); - } - - byte[] committedContent = new byte[] {2, 2, 2}; - TwoPhaseOutputStream.Committer successful; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - out.write(committedContent); - successful = out.closeForCommit(); - } - - abandoned.discard(fs); - successful.commit(fs); - - assertThat(readBytes(target)).containsExactly(committedContent); - } + void testMkdirsFailsWithExistingParentFile() throws Exception { + final Path file = new Path(getBasePath(), randomName()); + createFile(file); - @Test - void testTwoPhaseCleanPreservesCommittedTarget() throws IOException { - byte[] content = new byte[] {2, 7, 1, 8}; - Path target = new Path(basePath, randomName()); - TwoPhaseOutputStream.Committer committer; - try (TwoPhaseOutputStream out = fs.newTwoPhaseOutputStream(target, false)) { - out.write(content); - committer = out.closeForCommit(); + final Path dirUnderFile = new Path(file, randomName()); + try { + fs.mkdirs(dirUnderFile); + fail("should fail with an IOException"); + } catch (IOException e) { + // good! } - committer.commit(fs); - - committer.clean(fs); - - assertThat(readBytes(target)).containsExactly(content); } // ------------------------------------------------------------------------ @@ -746,73 +268,17 @@ protected static String randomName() { return StringUtils.getRandomString(RND, 16, 16, 'a', 'z'); } - private void writeBytes(Path file, byte[] content, boolean overwrite) throws IOException { - try (PositionOutputStream out = fs.newOutputStream(file, overwrite)) { - out.write(content); - } - } - - private byte[] readBytes(Path file) throws IOException { - try (SeekableInputStream in = fs.newInputStream(file)) { - return readAll(in); - } - } - - private void assertOpenOrFirstReadFails(Path path) { - final SeekableInputStream in; - try { - in = fs.newInputStream(path); - } catch (IOException expectedAtOpen) { - return; - } - - try { - assertThatThrownBy(() -> in.read()).isInstanceOf(IOException.class); - } finally { - try { - in.close(); - } catch (IOException ignoredAtClose) { - // A close-only failure is deliberately irrelevant to the open/first-read contract. - } - } - } - - private static byte[] readAll(SeekableInputStream in) throws IOException { - ByteArrayOutputStream out = new ByteArrayOutputStream(); - byte[] buffer = new byte[4]; - int read; - while ((read = in.read(buffer, 0, buffer.length)) != -1) { - out.write(buffer, 0, read); - } - return out.toByteArray(); - } - - private static List collect(RemoteIterator iterator) - throws IOException { - List statuses = new ArrayList<>(); - while (iterator.hasNext()) { - statuses.add(iterator.next()); - } - return statuses; - } - - private static FileStatus statusFor(FileStatus[] statuses, Path path) { - for (FileStatus status : statuses) { - if (status.getPath().equals(path)) { - return status; - } + private void createFile(Path file) throws IOException { + try (PositionOutputStream out = fs.newOutputStream(file, false)) { + out.write(new byte[] {1, 2, 3, 4, 5, 6, 7, 8}); } - throw new AssertionError("No status for " + path); } private Path createRandomFileInDirectory(Path directory) throws IOException { - return createRandomFileInDirectory(directory, DEFAULT_CONTENT); - } - - private Path createRandomFileInDirectory(Path directory, byte[] content) throws IOException { fs.mkdirs(directory); - Path file = new Path(directory, randomName()); - writeBytes(file, content, false); - return file; + final Path filePath = new Path(directory, randomName()); + createFile(filePath); + + return filePath; } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java new file mode 100644 index 000000000000..58ad87416b65 --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java @@ -0,0 +1,651 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayOutputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Opt-in provider-neutral contract tests for {@link FileIO}. */ +public abstract class FileIOContractTestBase extends FileIOBehaviorTestBase { + + private static final byte[] DEFAULT_CONTENT = new byte[] {1, 2, 3, 4, 5, 6, 7, 8}; + + private FileIO contractFileIO; + + private Path contractBasePath; + + @AfterEach + void cleanupContractFixture() throws IOException { + if (contractFileIO != null) { + contractFileIO.delete(contractBasePath, true); + } + } + + // ------------------------------------------------------------------------ + // Input streams + // ------------------------------------------------------------------------ + + @Test + void testInputStreamStartsAtZeroAndReadsCorrectBytes() throws IOException { + byte[] content = new byte[] {3, 1, 4, 1, 5, 9}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + assertThat(in.getPos()).isZero(); + assertThat(readAll(in)).containsExactly(content); + assertThat(in.getPos()).isEqualTo(content.length); + } + } + + @Test + void testInputStreamBulkReadHonorsNonZeroBufferOffset() throws IOException { + byte[] content = new byte[] {11, 22, 33}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + byte[] buffer = new byte[] {99, 98, 0, 0, 0, 97, 96}; + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + int totalRead = 0; + while (totalRead < content.length) { + int read = in.read(buffer, 2 + totalRead, content.length - totalRead); + assertThat(read).isPositive(); + totalRead += read; + } + + assertThat(totalRead).isEqualTo(content.length); + assertThat(buffer).containsExactly(99, 98, 11, 22, 33, 97, 96); + assertThat(in.getPos()).isEqualTo(content.length); + } + } + + @Test + void testInputStreamsHaveIndependentPositions() throws IOException { + Path file = createRandomFileInDirectory(contractBasePath(), new byte[] {10, 20, 30}); + + try (SeekableInputStream first = contractFileIO().newInputStream(file); + SeekableInputStream second = contractFileIO().newInputStream(file)) { + assertThat(first.read()).isEqualTo(10); + assertThat(first.getPos()).isEqualTo(1); + assertThat(second.getPos()).isZero(); + assertThat(second.read()).isEqualTo(10); + assertThat(second.getPos()).isEqualTo(1); + + first.seek(2); + assertThat(first.read()).isEqualTo(30); + assertThat(second.read()).isEqualTo(20); + } + } + + @Test + void testInputStreamSeeksForwardAndBackward() throws IOException { + byte[] content = new byte[] {10, 20, 30, 40, 50, 60}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + in.seek(4); + assertThat(in.getPos()).isEqualTo(4); + assertThat(in.read()).isEqualTo(50); + + in.seek(1); + assertThat(in.getPos()).isEqualTo(1); + assertThat(in.read()).isEqualTo(20); + } + } + + @Test + void testInputStreamReturnsEndOfFileAtFileLength() throws IOException { + byte[] content = new byte[] {7, 8, 9}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + in.seek(content.length); + assertThat(in.read()).isEqualTo(-1); + assertThat(in.read(new byte[2], 0, 2)).isEqualTo(-1); + } + } + + @Test + void testInputStreamCanSeekBackToStart() throws IOException { + byte[] content = new byte[] {7, 8, 9}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + assertThat(in.read()).isEqualTo(7); + in.seek(0); + assertThat(in.getPos()).isZero(); + assertThat(in.read()).isEqualTo(7); + } + } + + @Test + void testInputStreamSeeksForwardBeyondOneMebibyte() throws IOException { + int targetPosition = 1024 * 1024 + 17; + byte[] content = new byte[targetPosition + 1]; + content[targetPosition] = 42; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + in.seek(targetPosition); + assertThat(in.getPos()).isEqualTo(targetPosition); + assertThat(in.read()).isEqualTo(42); + } + } + + @Test + void testInputStreamForMissingFileFailsByFirstRead() throws IOException { + Path missing = new Path(contractBasePath(), randomName()); + + assertOpenOrFirstReadFails(missing); + } + + @Test + void testInputStreamForDirectoryFailsByFirstRead() throws IOException { + Path directory = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(directory); + + assertOpenOrFirstReadFails(directory); + } + + // ------------------------------------------------------------------------ + // Output streams + // ------------------------------------------------------------------------ + + @Test + void testOutputStreamTracksPositionAndPublishesBytesOnClose() throws IOException { + Path file = new Path(contractBasePath(), randomName()); + + try (PositionOutputStream out = contractFileIO().newOutputStream(file, false)) { + assertThat(out.getPos()).isZero(); + out.write(9); + assertThat(out.getPos()).isEqualTo(1); + out.write(new byte[] {10, 11, 12, 13}, 1, 2); + assertThat(out.getPos()).isEqualTo(3); + } + + assertThat(readBytes(file)).containsExactly(9, 11, 12); + } + + @Test + void testOutputStreamCreatesNestedTarget() throws IOException { + Path ancestor = new Path(contractBasePath(), randomName()); + Path parent = new Path(ancestor, randomName()); + Path file = new Path(parent, randomName()); + byte[] content = new byte[] {1, 3, 3, 7}; + + writeBytes(file, content, false); + + assertThat(readBytes(file)).containsExactly(content); + assertThat(contractFileIO().getFileStatus(ancestor).isDir()).isTrue(); + assertThat(contractFileIO().getFileStatus(parent).isDir()).isTrue(); + } + + @Test + void testOutputStreamOverwriteReplacesOldContent() throws IOException { + Path file = createRandomFileInDirectory(contractBasePath(), new byte[] {1, 2, 3, 4, 5}); + + writeBytes(file, new byte[] {8, 9}, true); + + assertThat(readBytes(file)).containsExactly(8, 9); + } + + @Test + void testOutputStreamNoOverwriteFailsAndPreservesOldContent() throws IOException { + byte[] oldContent = new byte[] {1, 2, 3}; + Path file = createRandomFileInDirectory(contractBasePath(), oldContent); + + assertThatThrownBy(() -> writeBytes(file, new byte[] {9, 8, 7}, false)) + .isInstanceOf(IOException.class); + assertThat(readBytes(file)).containsExactly(oldContent); + } + + // ------------------------------------------------------------------------ + // File status + // ------------------------------------------------------------------------ + + @Test + void testGetFileStatusForMissingPathThrowsFileNotFound() throws IOException { + Path missing = new Path(contractBasePath(), randomName()); + + assertThatThrownBy(() -> contractFileIO().getFileStatus(missing)) + .isInstanceOf(FileNotFoundException.class); + } + + @Test + void testGetFileStatusDescribesFile() throws IOException { + byte[] content = new byte[] {2, 4, 6, 8, 10}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + FileStatus status = contractFileIO().getFileStatus(file); + + assertThat(status.getPath()).isEqualTo(file); + assertThat(status.isDir()).isFalse(); + assertThat(status.getLen()).isEqualTo(content.length); + } + + @Test + void testGetFileStatusDescribesDirectory() throws IOException { + Path directory = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(directory); + + FileStatus status = contractFileIO().getFileStatus(directory); + + assertThat(status.getPath()).isEqualTo(directory); + assertThat(status.isDir()).isTrue(); + } + + // ------------------------------------------------------------------------ + // Listings + // ------------------------------------------------------------------------ + + @Test + void testListStatusOfEmptyDirectoryReturnsNonNullEmptyArray() throws IOException { + FileStatus[] statuses = contractFileIO().listStatus(contractBasePath()); + + assertThat(statuses).isEmpty(); + } + + @Test + void testListStatusReturnsOnlyCorrectDirectChildren() throws IOException { + byte[] firstContent = new byte[] {1, 2, 3, 4}; + byte[] secondContent = new byte[] {5, 6}; + Path firstFile = createRandomFileInDirectory(contractBasePath(), firstContent); + Path secondFile = createRandomFileInDirectory(contractBasePath(), secondContent); + Path firstDirectory = new Path(contractBasePath(), randomName()); + Path secondDirectory = new Path(contractBasePath(), randomName()); + Path nestedDirectory = new Path(firstDirectory, randomName()); + createRandomFileInDirectory(nestedDirectory, new byte[] {9}); + contractFileIO().mkdirs(secondDirectory); + + FileStatus[] statuses = contractFileIO().listStatus(contractBasePath()); + + assertThat(statuses) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstFile, secondFile, firstDirectory, secondDirectory); + FileStatus firstFileStatus = statusFor(statuses, firstFile); + assertThat(firstFileStatus.isDir()).isFalse(); + assertThat(firstFileStatus.getLen()).isEqualTo(firstContent.length); + FileStatus secondFileStatus = statusFor(statuses, secondFile); + assertThat(secondFileStatus.isDir()).isFalse(); + assertThat(secondFileStatus.getLen()).isEqualTo(secondContent.length); + assertThat(statusFor(statuses, firstDirectory).isDir()).isTrue(); + assertThat(statusFor(statuses, secondDirectory).isDir()).isTrue(); + } + + @Test + void testListFilesNonRecursiveReturnsOnlyDirectFilesAndMatchesIterator() throws IOException { + Path firstDirectFile = createRandomFileInDirectory(contractBasePath()); + Path secondDirectFile = createRandomFileInDirectory(contractBasePath()); + Path directory = new Path(contractBasePath(), randomName()); + createRandomFileInDirectory(directory); + + FileStatus[] arrayResult = contractFileIO().listFiles(contractBasePath(), false); + List iteratorResult = + collect(contractFileIO().listFilesIterative(contractBasePath(), false)); + + assertThat(arrayResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); + assertThat(iteratorResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectFile, secondDirectFile); + assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); + assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); + } + + @Test + void testListFilesRecursiveReturnsAllFilesAndMatchesIterator() throws IOException { + Path firstDirectFile = createRandomFileInDirectory(contractBasePath()); + Path secondDirectFile = createRandomFileInDirectory(contractBasePath()); + Path firstLevelDirectory = new Path(contractBasePath(), randomName()); + Path firstLevelFile = createRandomFileInDirectory(firstLevelDirectory); + Path secondLevelDirectory = new Path(firstLevelDirectory, randomName()); + Path secondLevelFile = createRandomFileInDirectory(secondLevelDirectory); + + FileStatus[] arrayResult = contractFileIO().listFiles(contractBasePath(), true); + List iteratorResult = + collect(contractFileIO().listFilesIterative(contractBasePath(), true)); + + assertThat(arrayResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder( + firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); + assertThat(iteratorResult) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder( + firstDirectFile, secondDirectFile, firstLevelFile, secondLevelFile); + assertThat(Arrays.stream(arrayResult).allMatch(status -> !status.isDir())).isTrue(); + assertThat(iteratorResult.stream().allMatch(status -> !status.isDir())).isTrue(); + } + + @Test + void testListDirectoriesReturnsOnlyDirectDirectories() throws IOException { + createRandomFileInDirectory(contractBasePath()); + Path firstDirectDirectory = new Path(contractBasePath(), randomName()); + Path secondDirectDirectory = new Path(contractBasePath(), randomName()); + Path nestedDirectory = new Path(firstDirectDirectory, randomName()); + contractFileIO().mkdirs(nestedDirectory); + contractFileIO().mkdirs(secondDirectDirectory); + + FileStatus[] statuses = contractFileIO().listDirectories(contractBasePath()); + + assertThat(statuses) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(firstDirectDirectory, secondDirectDirectory); + assertThat(Arrays.stream(statuses).allMatch(FileStatus::isDir)).isTrue(); + } + + // ------------------------------------------------------------------------ + // Delete + // ------------------------------------------------------------------------ + + @Test + void testDeleteReturnsTrueForExistingTargets() throws IOException { + Path file = createRandomFileInDirectory(contractBasePath()); + Path directory = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(directory); + + assertThat(contractFileIO().delete(file, false)).isTrue(); + assertThat(contractFileIO().delete(directory, false)).isTrue(); + } + + // ------------------------------------------------------------------------ + // Rename + // ------------------------------------------------------------------------ + + @Test + void testRenameFileMovesExactBytesToMissingDestination() throws IOException { + byte[] content = new byte[] {4, 2, 4, 2}; + Path source = createRandomFileInDirectory(contractBasePath(), content); + Path destination = new Path(contractBasePath(), randomName()); + + assertThat(contractFileIO().rename(source, destination)).isTrue(); + + assertThat(contractFileIO().exists(source)).isFalse(); + assertThat(readBytes(destination)).containsExactly(content); + } + + @Test + void testRenameDirectoryMovesExactTreeToMissingDestination() throws IOException { + Path source = new Path(contractBasePath(), randomName()); + Path child = createRandomFileInDirectory(source, new byte[] {1, 2}); + Path nestedDirectory = new Path(source, randomName()); + Path nestedChild = createRandomFileInDirectory(nestedDirectory, new byte[] {3, 4, 5}); + Path destination = new Path(contractBasePath(), randomName()); + + assertThat(contractFileIO().rename(source, destination)).isTrue(); + + assertThat(contractFileIO().exists(source)).isFalse(); + assertThat(contractFileIO().exists(child)).isFalse(); + assertThat(contractFileIO().exists(nestedDirectory)).isFalse(); + assertThat(contractFileIO().exists(nestedChild)).isFalse(); + assertThat(readBytes(new Path(destination, child.getName()))).containsExactly(1, 2); + assertThat( + readBytes( + new Path( + new Path(destination, nestedDirectory.getName()), + nestedChild.getName()))) + .containsExactly(3, 4, 5); + } + + // ------------------------------------------------------------------------ + // Copy + // ------------------------------------------------------------------------ + + @Test + void testCopyFileCreatesDestinationWithSourceBytes() throws IOException { + byte[] content = new byte[] {6, 2, 6, 4, 3}; + Path source = createRandomFileInDirectory(contractBasePath(), content); + Path destination = new Path(contractBasePath(), randomName()); + + contractFileIO().copyFile(source, destination, false); + + assertThat(readBytes(destination)).containsExactly(content); + assertThat(readBytes(source)).containsExactly(content); + } + + @Test + void testCopyFileOverwriteReplacesDestination() throws IOException { + byte[] content = new byte[] {7, 7}; + Path source = createRandomFileInDirectory(contractBasePath(), content); + Path destination = createRandomFileInDirectory(contractBasePath(), new byte[] {1, 2, 3, 4}); + + contractFileIO().copyFile(source, destination, true); + + assertThat(readBytes(destination)).containsExactly(content); + assertThat(readBytes(source)).containsExactly(content); + } + + @Test + void testCopyFileNoOverwriteFailsAndPreservesDestination() throws IOException { + byte[] sourceContent = new byte[] {9, 9}; + Path source = createRandomFileInDirectory(contractBasePath(), sourceContent); + byte[] destinationContent = new byte[] {1, 2, 3}; + Path destination = createRandomFileInDirectory(contractBasePath(), destinationContent); + + assertThatThrownBy(() -> contractFileIO().copyFile(source, destination, false)) + .isInstanceOf(IOException.class); + assertThat(readBytes(destination)).containsExactly(destinationContent); + assertThat(readBytes(source)).containsExactly(sourceContent); + } + + @Test + void testCopyFilesCopiesEveryDirectFile() throws IOException { + Path sourceDirectory = new Path(contractBasePath(), randomName()); + Path first = createRandomFileInDirectory(sourceDirectory, new byte[] {1, 3}); + Path second = createRandomFileInDirectory(sourceDirectory, new byte[] {2, 4, 6}); + Path targetDirectory = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(targetDirectory); + + contractFileIO().copyFiles(sourceDirectory, targetDirectory, false); + + assertThat(readBytes(new Path(targetDirectory, first.getName()))).containsExactly(1, 3); + assertThat(readBytes(new Path(targetDirectory, second.getName()))).containsExactly(2, 4, 6); + assertThat(readBytes(first)).containsExactly(1, 3); + assertThat(readBytes(second)).containsExactly(2, 4, 6); + } + + // ------------------------------------------------------------------------ + // Two-phase output + // ------------------------------------------------------------------------ + + @Test + void testTwoPhaseOutputPublishesOnlyAfterCommit() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + byte[] content = new byte[] {5, 4, 3, 2, 1}; + TwoPhaseOutputStream out = contractFileIO().newTwoPhaseOutputStream(target, false); + assertThat(out.getPos()).isZero(); + out.write(content); + assertThat(out.getPos()).isEqualTo(content.length); + assertThat(contractFileIO().exists(target)).isFalse(); + TwoPhaseOutputStream.Committer committer = out.closeForCommit(); + + assertThat(committer.targetPath()).isEqualTo(target); + assertThat(contractFileIO().exists(target)).isFalse(); + + committer.commit(contractFileIO()); + assertThat(readBytes(target)).containsExactly(content); + } + + @Test + void testTwoPhaseDiscardDoesNotPublishAbandonedData() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer committer = + stageTwoPhaseOutput(target, new byte[] {1, 2, 3}); + + committer.discard(contractFileIO()); + + assertThat(contractFileIO().exists(target)).isFalse(); + } + + @Test + void testTwoPhaseDiscardPreservesPreExistingTarget() throws IOException { + byte[] oldContent = new byte[] {8, 6, 7, 5}; + Path target = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer committer = + stageTwoPhaseOutput(target, new byte[] {3, 0, 9}); + assertThat(contractFileIO().exists(target)).isFalse(); + + writeBytes(target, oldContent, false); + assertThat(readBytes(target)).containsExactly(oldContent); + + committer.discard(contractFileIO()); + + assertThat(readBytes(target)).containsExactly(oldContent); + } + + @Test + void testTwoPhaseDiscardDoesNotAffectAnotherWriter() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer abandoned = + stageTwoPhaseOutput(target, new byte[] {1, 1, 1}); + + byte[] committedContent = new byte[] {2, 2, 2}; + TwoPhaseOutputStream.Committer successful = stageTwoPhaseOutput(target, committedContent); + + abandoned.discard(contractFileIO()); + successful.commit(contractFileIO()); + + assertThat(readBytes(target)).containsExactly(committedContent); + } + + @Test + void testTwoPhaseCleanPreservesCommittedTarget() throws IOException { + byte[] content = new byte[] {2, 7, 1, 8}; + Path target = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer committer = stageTwoPhaseOutput(target, content); + committer.commit(contractFileIO()); + + committer.clean(contractFileIO()); + + assertThat(readBytes(target)).containsExactly(content); + } + + private FileIO contractFileIO() throws IOException { + initializeContractFixture(); + return contractFileIO; + } + + private Path contractBasePath() throws IOException { + initializeContractFixture(); + return contractBasePath; + } + + private void initializeContractFixture() throws IOException { + if (contractFileIO != null) { + return; + } + + try { + FileIO fileIO = getFileSystem(); + Path basePath = new Path(getBasePath(), randomName()); + fileIO.mkdirs(basePath); + contractFileIO = fileIO; + contractBasePath = basePath; + } catch (IOException e) { + throw e; + } catch (Exception e) { + throw new IOException(e); + } + } + + private void writeBytes(Path file, byte[] content, boolean overwrite) throws IOException { + try (PositionOutputStream out = contractFileIO().newOutputStream(file, overwrite)) { + out.write(content); + } + } + + private TwoPhaseOutputStream.Committer stageTwoPhaseOutput(Path target, byte[] content) + throws IOException { + TwoPhaseOutputStream out = contractFileIO().newTwoPhaseOutputStream(target, false); + out.write(content); + return out.closeForCommit(); + } + + private byte[] readBytes(Path file) throws IOException { + try (SeekableInputStream in = contractFileIO().newInputStream(file)) { + return readAll(in); + } + } + + private void assertOpenOrFirstReadFails(Path path) throws IOException { + final SeekableInputStream in; + try { + in = contractFileIO().newInputStream(path); + } catch (IOException expectedAtOpen) { + return; + } + + try { + assertThatThrownBy(in::read).isInstanceOf(IOException.class); + } finally { + try { + in.close(); + } catch (IOException ignoredAtClose) { + } + } + } + + private static byte[] readAll(SeekableInputStream in) throws IOException { + ByteArrayOutputStream out = new ByteArrayOutputStream(); + byte[] buffer = new byte[4]; + int read; + while ((read = in.read(buffer, 0, buffer.length)) != -1) { + out.write(buffer, 0, read); + } + return out.toByteArray(); + } + + private static List collect(RemoteIterator iterator) + throws IOException { + List statuses = new ArrayList<>(); + while (iterator.hasNext()) { + statuses.add(iterator.next()); + } + return statuses; + } + + private static FileStatus statusFor(FileStatus[] statuses, Path path) { + for (FileStatus status : statuses) { + if (status.getPath().equals(path)) { + return status; + } + } + throw new AssertionError("No status for " + path); + } + + private Path createRandomFileInDirectory(Path directory) throws IOException { + return createRandomFileInDirectory(directory, DEFAULT_CONTENT); + } + + private Path createRandomFileInDirectory(Path directory, byte[] content) throws IOException { + contractFileIO().mkdirs(directory); + Path file = new Path(directory, randomName()); + writeBytes(file, content, false); + return file; + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java index 9b031a528060..e5946a049431 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java @@ -22,15 +22,15 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.RawLocalFileSystem; -import org.junit.jupiter.api.Test; +import org.apache.hadoop.util.VersionInfo; import org.junit.jupiter.api.io.TempDir; import java.net.URI; -import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assumptions.assumeThat; /** Behavior tests for Hadoop Local. */ -class HadoopLocalFileIOBehaviorTest extends FileIOBehaviorTestBase { +class HadoopLocalFileIOBehaviorTest extends FileIOContractTestBase { @TempDir private java.nio.file.Path tmp; @@ -48,8 +48,18 @@ protected Path getBasePath() { return new Path(tmp.toUri()); } - @Test - void testIsObjectStoreReturnsFalse() throws Exception { - assertThat(getFileSystem().isObjectStore()).isFalse(); + // ------------------------------------------------------------------------ + + /** This test needs to be skipped for earlier Hadoop versions because those have a bug. */ + @Override + protected void testMkdirsFailsForExistingFile() throws Exception { + final String versionString = VersionInfo.getVersion(); + final String prefix = versionString.substring(0, 3); + final float version = Float.parseFloat(prefix); + assumeThat(version) + .describedAs("Cannot execute this test on Hadoop prior to 2.8") + .isGreaterThanOrEqualTo(2.8f); + + super.testMkdirsFailsForExistingFile(); } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/HdfsBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/HdfsBehaviorTest.java index b719038caa07..0a5479a8de89 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/HdfsBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/HdfsBehaviorTest.java @@ -38,7 +38,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; /** Behavior tests for HDFS. */ -class HdfsBehaviorTest extends FileIOBehaviorTestBase { +class HdfsBehaviorTest extends FileIOContractTestBase { private static MiniDFSCluster hdfsCluster; diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java index 8b308da2c478..cdb0dfaa7637 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java @@ -20,17 +20,10 @@ import org.apache.paimon.fs.local.LocalFileIO; -import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; -import java.io.IOException; -import java.nio.file.Files; -import java.nio.file.attribute.FileTime; - -import static org.assertj.core.api.Assertions.assertThat; - /** Test for {@link LocalFileIO}. */ -public class LocalFileIOBehaviorTest extends FileIOBehaviorTestBase { +public class LocalFileIOBehaviorTest extends FileIOContractTestBase { @TempDir private java.nio.file.Path tmp; @@ -43,27 +36,4 @@ protected FileIO getFileSystem() { protected Path getBasePath() { return new Path(tmp.toUri()); } - - @Test - void testIsObjectStoreReturnsFalse() { - assertThat(getFileSystem().isObjectStore()).isFalse(); - } - - @Test - void testFileStatusSnapshotsModificationTime() throws IOException { - java.nio.file.Path file = Files.createFile(tmp.resolve("snapshot")); - FileTime firstTimestamp = FileTime.fromMillis(1_000_000L); - FileTime secondTimestamp = FileTime.fromMillis(2_000_000L); - Files.setLastModifiedTime(file, firstTimestamp); - - FileIO fileIO = getFileSystem(); - Path path = new Path(file.toUri()); - FileStatus snapshot = fileIO.getFileStatus(path); - - Files.setLastModifiedTime(file, secondTimestamp); - - assertThat(snapshot.getModificationTime()).isEqualTo(firstTimestamp.toMillis()); - assertThat(fileIO.getFileStatus(path).getModificationTime()) - .isEqualTo(secondTimestamp.toMillis()); - } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java index dcb73cdc856b..1a66abf99cce 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java @@ -128,6 +128,24 @@ void testCleanRemovesTheFileItStagedWhenThereWasNoCommit() throws IOException { assertThat(fileIO.listStatus(stagingDir)).isEmpty(); } + @Test + void testDiscard() throws IOException { + RenamingTwoPhaseOutputStream stream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, false); + + // Write some data + stream.write("Some data".getBytes()); + + // Close for commit + TwoPhaseOutputStream.Committer committer = stream.closeForCommit(); + + // Discard instead of commit + committer.discard(fileIO); + + // Target file should not exist + assertThat(fileIO.exists(targetPath)).isFalse(); + } + @Test void testDiscardRemovesOnlyItsStagedFile() throws IOException { RenamingTwoPhaseOutputStream stream = diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3FileIOTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3FileIOTest.java index 51e6f0c6f74d..911c8f632811 100644 --- a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3FileIOTest.java +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3FileIOTest.java @@ -20,7 +20,7 @@ import org.apache.paimon.catalog.CatalogContext; import org.apache.paimon.fs.FileIO; -import org.apache.paimon.fs.FileIOBehaviorTestBase; +import org.apache.paimon.fs.FileIOContractTestBase; import org.apache.paimon.fs.Path; import org.apache.paimon.options.Options; @@ -36,7 +36,7 @@ * Behavior tests for {@link S3FileIO}, backed by a MinIO container. Exercises the file system * contract with credentials and, separately, credential-less (anonymous) access. */ -class S3FileIOTest extends FileIOBehaviorTestBase { +class S3FileIOTest extends FileIOContractTestBase { private static final String TEMPORARY_PROVIDER = "org.apache.hadoop.fs.s3a.TemporaryAWSCredentialsProvider"; From 99ec823b945f8089aac1b7db681bf18a2dc154e1 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 17:30:38 +0800 Subject: [PATCH 04/11] [common] Cover all FileIO contract layers --- .../fs/RenamingTwoPhaseOutputStream.java | 15 +- .../apache/paimon/fs/hadoop/HadoopFileIO.java | 8 + .../paimon/fs/FileIOContractCoverageTest.java | 122 +++++ .../paimon/fs/FileIOContractTestBase.java | 183 ++++++- .../paimon/fs/FileIODefaultMethodTest.java | 487 ++++++++++++++++++ .../paimon/fs/FileIOReturnTypeTest.java | 99 ++++ .../java/org/apache/paimon/fs/FileIOTest.java | 151 ++++++ .../fs/HadoopLocalFileIOBehaviorTest.java | 7 + .../paimon/fs/LocalFileIOBehaviorTest.java | 8 + .../org/apache/paimon/fs/RecordingFileIO.java | 427 +++++++++++++++ .../apache/paimon/fs/RecordingFileIOTest.java | 81 +++ .../paimon/fs/StrictContractFileIO.java | 347 +++++++++++++ .../paimon/fs/StrictContractFileIOTest.java | 182 +++++++ .../table/format/FormatTableCommit.java | 5 + .../java/org/apache/paimon/TestFileStore.java | 19 +- .../paimon/catalog/FileSystemCatalogTest.java | 31 ++ .../paimon/operation/FileStoreCommitTest.java | 108 +++- .../utils/FileSystemBranchManagerTest.java | 6 +- .../apache/paimon/utils/TagManagerTest.java | 53 ++ 19 files changed, 2315 insertions(+), 24 deletions(-) create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractCoverageTest.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/FileIODefaultMethodTest.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/FileIOReturnTypeTest.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIO.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIOTest.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIO.java create mode 100644 paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIOTest.java diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java b/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java index 4771fd671605..cad050f99e8e 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStream.java @@ -35,6 +35,7 @@ public class RenamingTwoPhaseOutputStream extends TwoPhaseOutputStream { private final Path targetPath; private final Path tempPath; private final PositionOutputStream tempOutputStream; + private final boolean overwrite; public RenamingTwoPhaseOutputStream(FileIO fileIO, Path targetPath, boolean overwrite) throws IOException { @@ -43,6 +44,7 @@ public RenamingTwoPhaseOutputStream(FileIO fileIO, Path targetPath, boolean over } this.targetPath = targetPath; this.tempPath = generateTempPath(targetPath); + this.overwrite = overwrite; // Create temporary file this.tempOutputStream = fileIO.newOutputStream(tempPath, overwrite); @@ -81,7 +83,7 @@ public void close() throws IOException { @Override public Committer closeForCommit() throws IOException { close(); - return new TempFileCommitter(tempPath, targetPath); + return new TempFileCommitter(tempPath, targetPath, overwrite); } /** @@ -100,10 +102,12 @@ private static class TempFileCommitter implements Committer { private final Path tempPath; private final Path targetPath; + private final boolean overwrite; - private TempFileCommitter(Path tempPath, Path targetPath) { + private TempFileCommitter(Path tempPath, Path targetPath, boolean overwrite) { this.tempPath = tempPath; this.targetPath = targetPath; + this.overwrite = overwrite; } @Override @@ -112,7 +116,12 @@ public void commit(FileIO fileIO) throws IOException { if (parentDir != null && !fileIO.exists(parentDir)) { fileIO.mkdirs(parentDir); } - if (!fileIO.rename(tempPath, targetPath)) { + boolean renamed = fileIO.rename(tempPath, targetPath); + if (!renamed && overwrite && fileIO.exists(tempPath)) { + fileIO.delete(targetPath, false); + renamed = fileIO.rename(tempPath, targetPath); + } + if (!renamed) { throw new IOException("Failed to rename " + tempPath + " to " + targetPath); } if (fileIO.exists(tempPath)) { diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/hadoop/HadoopFileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/hadoop/HadoopFileIO.java index 3ff241d6c8f2..ba091e1ba366 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/hadoop/HadoopFileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/hadoop/HadoopFileIO.java @@ -176,6 +176,14 @@ public boolean rename(Path src, Path dst) throws IOException { return getFileSystem(hadoopSrc).rename(hadoopSrc, hadoopDst); } + @Override + public boolean tryToWriteAtomic(Path path, String content) throws IOException { + if ("file".equalsIgnoreCase(path.toUri().getScheme()) && exists(path)) { + return false; + } + return FileIO.super.tryToWriteAtomic(path, content); + } + @Override public void overwriteFileUtf8(Path path, String content) throws IOException { boolean success = tryAtomicOverwriteViaRename(path, content); diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractCoverageTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractCoverageTest.java new file mode 100644 index 000000000000..15ec8a98249d --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractCoverageTest.java @@ -0,0 +1,122 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.assertj.core.api.Assertions.assertThat; + +/** Guards the ownership of every public method declared by {@link FileIO}. */ +public class FileIOContractCoverageTest { + + private static final Set PROVIDER_CORE = + signatures( + "isObjectStore()", + "newInputStream(Path)", + "newOutputStream(Path,boolean)", + "getFileStatus(Path)", + "listStatus(Path)", + "exists(Path)", + "delete(Path,boolean)", + "mkdirs(Path)", + "rename(Path,Path)"); + + private static final Set DEFAULT_METHOD = + signatures( + "newTwoPhaseOutputStream(Path,boolean)", + "listFiles(Path,boolean)", + "listFilesIterative(Path,boolean)", + "listDirectories(Path)", + "deleteQuietly(Path)", + "deleteFilesQuietly(List)", + "deleteDirectoryQuietly(Path)", + "getFileSize(Path)", + "isDir(Path)", + "checkOrMkdirs(Path)", + "readFileUtf8(Path)", + "tryToWriteAtomic(Path,String)", + "writeFile(Path,String,boolean)", + "overwriteFileUtf8(Path,String)", + "overwriteHintFile(Path,String)", + "copyFile(Path,Path,boolean)", + "copyFiles(Path,Path,boolean)", + "readOverwrittenFileUtf8(Path)"); + + private static final Set PROVIDER_LIFECYCLE = + signatures("configure(CatalogContext)", "setRuntimeContext(Map)", "close()"); + + private static final Set FACTORY = + signatures( + "get(Path,CatalogContext)", + "discoverLoaders()", + "checkAccess(FileIOLoader,Path,CatalogContext)"); + + private static final Set OPTIONAL_CAPABILITY = + signatures( + "archive(Path,StorageType)", + "restoreArchive(Path,Duration)", + "unarchive(Path,StorageType)", + "createBlobPresignedUrl(Path,BlobDescriptor,Duration)"); + + @Test + public void testEveryDeclaredPublicMethodHasExactlyOneOwner() { + Set actual = + Arrays.stream(FileIO.class.getDeclaredMethods()) + .filter(method -> Modifier.isPublic(method.getModifiers())) + .filter(method -> !method.isSynthetic()) + .map(FileIOContractCoverageTest::signature) + .collect(Collectors.toSet()); + + List> categories = + Arrays.asList( + PROVIDER_CORE, + DEFAULT_METHOD, + PROVIDER_LIFECYCLE, + FACTORY, + OPTIONAL_CAPABILITY); + Set classified = new HashSet<>(); + for (Set category : categories) { + assertThat(Collections.disjoint(category, classified)).isTrue(); + classified.addAll(category); + } + + assertThat(actual).hasSize(37); + assertThat(classified).containsExactlyInAnyOrderElementsOf(actual); + } + + private static Set signatures(String... signatures) { + return new HashSet<>(Arrays.asList(signatures)); + } + + private static String signature(Method method) { + return method.getName() + + Arrays.stream(method.getParameterTypes()) + .map(Class::getSimpleName) + .collect(Collectors.joining(",", "(", ")")); + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java index 58ad87416b65..3bb178962de5 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java @@ -18,15 +18,19 @@ package org.apache.paimon.fs; +import org.apache.paimon.utils.InstantiationUtil; + import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import java.io.ByteArrayOutputStream; import java.io.FileNotFoundException; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -125,7 +129,9 @@ void testInputStreamReturnsEndOfFileAtFileLength() throws IOException { try (SeekableInputStream in = contractFileIO().newInputStream(file)) { in.seek(content.length); assertThat(in.read()).isEqualTo(-1); + assertThat(in.getPos()).isEqualTo(content.length); assertThat(in.read(new byte[2], 0, 2)).isEqualTo(-1); + assertThat(in.getPos()).isEqualTo(content.length); } } @@ -190,6 +196,21 @@ void testOutputStreamTracksPositionAndPublishesBytesOnClose() throws IOException assertThat(readBytes(file)).containsExactly(9, 11, 12); } + @Test + void testOutputStreamFlushKeepsPositionAndClosePublishesLaterWrites() throws IOException { + Path file = new Path(contractBasePath(), randomName()); + + try (PositionOutputStream out = contractFileIO().newOutputStream(file, false)) { + out.write(new byte[] {1, 2}); + out.flush(); + assertThat(out.getPos()).isEqualTo(2); + out.write(3); + assertThat(out.getPos()).isEqualTo(3); + } + + assertThat(readBytes(file)).containsExactly(1, 2, 3); + } + @Test void testOutputStreamCreatesNestedTarget() throws IOException { Path ancestor = new Path(contractBasePath(), randomName()); @@ -258,6 +279,36 @@ void testGetFileStatusDescribesDirectory() throws IOException { assertThat(status.isDir()).isTrue(); } + @Test + void testFileStatusProvidesConsistentModificationTime() throws IOException { + Path file = createRandomFileInDirectory(contractBasePath()); + + FileStatus directStatus = contractFileIO().getFileStatus(file); + FileStatus listedStatus = statusFor(contractFileIO().listStatus(contractBasePath()), file); + + assertThat(directStatus.getModificationTime()).isGreaterThan(1_000_000_000_000L); + assertThat(listedStatus.getModificationTime()) + .isEqualTo(directStatus.getModificationTime()); + } + + @Test + void testExistsRecognizesDirectory() throws IOException { + Path directory = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(directory); + + assertThat(contractFileIO().exists(directory)).isTrue(); + } + + @Test + void testStatusHelpersDescribeFilesAndDirectories() throws IOException { + byte[] content = new byte[] {1, 4, 9, 16}; + Path file = createRandomFileInDirectory(contractBasePath(), content); + + assertThat(contractFileIO().getFileSize(file)).isEqualTo(content.length); + assertThat(contractFileIO().isDir(file)).isFalse(); + assertThat(contractFileIO().isDir(contractBasePath())).isTrue(); + } + // ------------------------------------------------------------------------ // Listings // ------------------------------------------------------------------------ @@ -469,6 +520,50 @@ void testCopyFilesCopiesEveryDirectFile() throws IOException { assertThat(readBytes(second)).containsExactly(2, 4, 6); } + // ------------------------------------------------------------------------ + // Text and atomic helpers + // ------------------------------------------------------------------------ + + @Test + void testUtf8ReadWriteHelpersPreserveContent() throws IOException { + Path file = new Path(contractBasePath(), randomName()); + String content = "Paimon-文件-IO"; + + contractFileIO().writeFile(file, content, false); + + assertThat(contractFileIO().readFileUtf8(file)).isEqualTo(content); + assertThat(readBytes(file)).containsExactly(content.getBytes(StandardCharsets.UTF_8)); + } + + @Test + void testOverwriteHelpersReplaceVisibleContent() throws IOException { + Path file = new Path(contractBasePath(), randomName()); + contractFileIO().writeFile(file, "old", false); + + contractFileIO().overwriteFileUtf8(file, "new"); + assertThat(contractFileIO().readFileUtf8(file)).isEqualTo("new"); + + contractFileIO().overwriteHintFile(file, "hint"); + assertThat(contractFileIO().readFileUtf8(file)).isEqualTo("hint"); + } + + @Test + void testTryToWriteAtomicPublishesMissingTarget() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + + assertThat(contractFileIO().tryToWriteAtomic(target, "atomic")).isTrue(); + assertThat(contractFileIO().readFileUtf8(target)).isEqualTo("atomic"); + } + + @Test + void testTryToWriteAtomicPreservesExistingTarget() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + contractFileIO().writeFile(target, "existing", false); + + assertThat(contractFileIO().tryToWriteAtomic(target, "replacement")).isFalse(); + assertThat(contractFileIO().readFileUtf8(target)).isEqualTo("existing"); + } + // ------------------------------------------------------------------------ // Two-phase output // ------------------------------------------------------------------------ @@ -491,6 +586,44 @@ void testTwoPhaseOutputPublishesOnlyAfterCommit() throws IOException { assertThat(readBytes(target)).containsExactly(content); } + @Test + void testTwoPhaseNoOverwritePreservesExistingTarget() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + byte[] existing = new byte[] {9, 8, 7}; + writeBytes(target, existing, false); + + AtomicReference staged = new AtomicReference<>(); + try { + assertThatThrownBy( + () -> { + TwoPhaseOutputStream out = + contractFileIO().newTwoPhaseOutputStream(target, false); + out.write(new byte[] {1, 2, 3}); + staged.set(out.closeForCommit()); + staged.get().commit(contractFileIO()); + }) + .isInstanceOf(IOException.class); + } finally { + if (staged.get() != null) { + staged.get().discard(contractFileIO()); + } + } + assertThat(readBytes(target)).containsExactly(existing); + } + + @Test + void testTwoPhaseOverwriteReplacesExistingTargetOnCommit() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + writeBytes(target, new byte[] {9, 8, 7}, false); + byte[] replacement = new byte[] {1, 2, 3}; + + TwoPhaseOutputStream out = contractFileIO().newTwoPhaseOutputStream(target, true); + out.write(replacement); + out.closeForCommit().commit(contractFileIO()); + + assertThat(readBytes(target)).containsExactly(replacement); + } + @Test void testTwoPhaseDiscardDoesNotPublishAbandonedData() throws IOException { Path target = new Path(contractBasePath(), randomName()); @@ -545,6 +678,49 @@ void testTwoPhaseCleanPreservesCommittedTarget() throws IOException { assertThat(readBytes(target)).containsExactly(content); } + @Test + void testTwoPhaseCleanDoesNotAffectAnotherWriter() throws IOException { + Path target = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer first = stageTwoPhaseOutput(target, new byte[] {1, 2, 3}); + byte[] secondContent = new byte[] {4, 5, 6}; + TwoPhaseOutputStream.Committer second = stageTwoPhaseOutput(target, secondContent); + + first.commit(contractFileIO()); + assertThat(contractFileIO().delete(target, false)).isTrue(); + first.clean(contractFileIO()); + second.commit(contractFileIO()); + + assertThat(readBytes(target)).containsExactly(secondContent); + } + + @Test + void testTwoPhaseCommitterSurvivesSerialization() throws Exception { + Path target = new Path(contractBasePath(), randomName()); + byte[] content = new byte[] {8, 5, 3, 0, 9}; + TwoPhaseOutputStream.Committer committer = stageTwoPhaseOutput(target, content); + + TwoPhaseOutputStream.Committer restored = InstantiationUtil.clone(committer); + + assertThat(restored.targetPath()).isEqualTo(target); + restored.commit(contractFileIO()); + assertThat(readBytes(target)).containsExactly(content); + + Path discardedTarget = new Path(contractBasePath(), randomName()); + TwoPhaseOutputStream.Committer discarded = + InstantiationUtil.clone( + stageTwoPhaseOutput(discardedTarget, new byte[] {1, 4, 1, 4})); + discarded.discard(contractFileIO()); + assertThat(contractFileIO().exists(discardedTarget)).isFalse(); + + Path overwrittenTarget = new Path(contractBasePath(), randomName()); + writeBytes(overwrittenTarget, new byte[] {9, 9, 9}, false); + byte[] replacement = new byte[] {2, 6, 5, 3}; + TwoPhaseOutputStream.Committer overwriting = + InstantiationUtil.clone(stageTwoPhaseOutput(overwrittenTarget, replacement, true)); + overwriting.commit(contractFileIO()); + assertThat(readBytes(overwrittenTarget)).containsExactly(replacement); + } + private FileIO contractFileIO() throws IOException { initializeContractFixture(); return contractFileIO; @@ -581,7 +757,12 @@ private void writeBytes(Path file, byte[] content, boolean overwrite) throws IOE private TwoPhaseOutputStream.Committer stageTwoPhaseOutput(Path target, byte[] content) throws IOException { - TwoPhaseOutputStream out = contractFileIO().newTwoPhaseOutputStream(target, false); + return stageTwoPhaseOutput(target, content, false); + } + + private TwoPhaseOutputStream.Committer stageTwoPhaseOutput( + Path target, byte[] content, boolean overwrite) throws IOException { + TwoPhaseOutputStream out = contractFileIO().newTwoPhaseOutputStream(target, overwrite); out.write(content); return out.closeForCommit(); } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIODefaultMethodTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIODefaultMethodTest.java new file mode 100644 index 000000000000..014463834b63 --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIODefaultMethodTest.java @@ -0,0 +1,487 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +import static org.apache.paimon.fs.RecordingFileIO.Method.DELETE; +import static org.apache.paimon.fs.RecordingFileIO.Method.EXISTS; +import static org.apache.paimon.fs.RecordingFileIO.Method.GET_FILE_STATUS; +import static org.apache.paimon.fs.RecordingFileIO.Method.INPUT_READ; +import static org.apache.paimon.fs.RecordingFileIO.Method.LIST_STATUS; +import static org.apache.paimon.fs.RecordingFileIO.Method.MKDIRS; +import static org.apache.paimon.fs.RecordingFileIO.Method.NEW_INPUT_STREAM; +import static org.apache.paimon.fs.RecordingFileIO.Method.NEW_OUTPUT_STREAM; +import static org.apache.paimon.fs.RecordingFileIO.Method.OUTPUT_WRITE; +import static org.apache.paimon.fs.RecordingFileIO.Method.RENAME; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Contract tests for methods implemented directly on {@link FileIO}. */ +class FileIODefaultMethodTest { + + private RecordingFileIO fileIO; + private Path root; + + @BeforeEach + void beforeEach() { + fileIO = new RecordingFileIO(); + root = new Path("test:///root"); + fileIO.putDirectory(root); + fileIO.reset(); + } + + @Test + void lifecycleDefaultsAreNoOps() throws Exception { + fileIO.setRuntimeContext(Collections.singletonMap("key", "value")); + fileIO.close(); + + assertThat(fileIO.calls()).isEmpty(); + } + + @Test + void statusHelpersUseOnlyFileStatus() throws Exception { + Path file = new Path(root, "file"); + Path directory = new Path(root, "directory"); + fileIO.putFile(file, "你好"); + fileIO.putDirectory(directory); + fileIO.reset(); + + assertThat(fileIO.getFileSize(file)) + .isEqualTo("你好".getBytes(StandardCharsets.UTF_8).length); + assertThat(fileIO.calls()).containsExactly(RecordingFileIO.call(GET_FILE_STATUS, file)); + + fileIO.reset(); + assertThat(fileIO.isDir(file)).isFalse(); + assertThat(fileIO.calls()).containsExactly(RecordingFileIO.call(GET_FILE_STATUS, file)); + + fileIO.reset(); + assertThat(fileIO.isDir(directory)).isTrue(); + assertThat(fileIO.calls()) + .containsExactly(RecordingFileIO.call(GET_FILE_STATUS, directory)); + + fileIO.reset(); + fileIO.failNext(GET_FILE_STATUS, new IOException("status failed")); + assertThatThrownBy(() -> fileIO.getFileSize(file)) + .isInstanceOf(IOException.class) + .hasMessage("status failed"); + + fileIO.reset(); + fileIO.failNext(GET_FILE_STATUS, new IOException("type failed")); + assertThatThrownBy(() -> fileIO.isDir(file)) + .isInstanceOf(IOException.class) + .hasMessage("type failed"); + } + + @Test + void checkOrMkdirsAvoidsUnneededMutations() throws Exception { + Path existingDirectory = new Path(root, "directory"); + fileIO.putDirectory(existingDirectory); + fileIO.reset(); + + fileIO.checkOrMkdirs(existingDirectory); + + assertThat(fileIO.callCount(EXISTS) + fileIO.callCount(GET_FILE_STATUS)).isBetween(1L, 2L); + assertThat(fileIO.callCount(MKDIRS)).isZero(); + assertOnlyCalls(EXISTS, GET_FILE_STATUS); + + Path missing = new Path(root, "created"); + fileIO.reset(); + fileIO.checkOrMkdirs(missing); + assertThat(fileIO.isDirectoryInMemory(missing)).isTrue(); + assertThat(fileIO.callCount(EXISTS) + fileIO.callCount(GET_FILE_STATUS)) + .isLessThanOrEqualTo(1L); + assertThat(fileIO.callCount(MKDIRS)).isEqualTo(1); + assertOnlyCalls(EXISTS, GET_FILE_STATUS, MKDIRS); + + Path file = new Path(root, "file"); + fileIO.putFile(file, "content"); + fileIO.reset(); + assertThatThrownBy(() -> fileIO.checkOrMkdirs(file)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("should be a directory"); + assertThat(fileIO.callCount(MKDIRS)).isZero(); + assertOnlyCalls(EXISTS, GET_FILE_STATUS); + } + + @Test + void quietDeletesUseTheRequestedRecursionAndSuppressIoFailures() { + Path file = new Path(root, "file"); + fileIO.putFile(file, "content"); + fileIO.deleteQuietly(file); + assertThat(fileIO.existsInMemory(file)).isFalse(); + assertThat(fileIO.calls(DELETE)).containsExactly(RecordingFileIO.call(DELETE, file, false)); + assertThat(fileIO.callCount(EXISTS)).isZero(); + + Path first = new Path(root, "first"); + Path second = new Path(root, "second"); + fileIO.putFile(first, "1"); + fileIO.putFile(second, "2"); + fileIO.reset(); + fileIO.deleteFilesQuietly(Arrays.asList(first, second)); + assertThat(fileIO.existsInMemory(first)).isFalse(); + assertThat(fileIO.existsInMemory(second)).isFalse(); + assertThat(fileIO.calls()) + .containsExactly( + RecordingFileIO.call(DELETE, first, false), + RecordingFileIO.call(DELETE, second, false)); + + fileIO.putFile(first, "1"); + fileIO.putFile(second, "2"); + fileIO.reset(); + fileIO.failNext(DELETE, new IOException("first failed")); + fileIO.deleteFilesQuietly(Arrays.asList(first, second)); + assertThat(fileIO.existsInMemory(first)).isTrue(); + assertThat(fileIO.existsInMemory(second)).isFalse(); + assertThat(fileIO.calls(DELETE)) + .containsExactly( + RecordingFileIO.call(DELETE, first, false), + RecordingFileIO.call(DELETE, second, false)); + + Path directory = new Path(root, "directory"); + fileIO.putDirectory(directory); + fileIO.reset(); + fileIO.deleteDirectoryQuietly(directory); + assertThat(fileIO.calls(DELETE)) + .containsExactly(RecordingFileIO.call(DELETE, directory, true)); + + Path missing = new Path(root, "missing"); + fileIO.reset(); + fileIO.deleteQuietly(missing); + assertThat(fileIO.callCount(DELETE)).isEqualTo(1); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(1); + + fileIO.putFile(file, "content"); + fileIO.reset(); + fileIO.failNext(DELETE, new IOException("planned")); + fileIO.deleteQuietly(file); + assertThat(fileIO.existsInMemory(file)).isTrue(); + assertThat(fileIO.callCount(DELETE)).isEqualTo(1); + assertThat(fileIO.callCount(EXISTS)).isZero(); + } + + @Test + void utf8HelpersPreserveContentAndForwardOverwriteMode() throws Exception { + Path file = new Path(root, "unicode"); + String content = "Paimon-文件-🙂"; + + fileIO.writeFile(file, content, false); + assertThat(fileIO.fileContent(file)).isEqualTo(content); + assertThat(fileIO.openOutputStreams()).isZero(); + assertThat(fileIO.calls(NEW_OUTPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_OUTPUT_STREAM, file, false)); + + fileIO.reset(); + assertThat(fileIO.readFileUtf8(file)).isEqualTo(content); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.calls(NEW_INPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_INPUT_STREAM, file)); + + fileIO.reset(); + fileIO.overwriteFileUtf8(file, "replacement"); + assertThat(fileIO.fileContent(file)).isEqualTo("replacement"); + assertThat(fileIO.calls(NEW_OUTPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_OUTPUT_STREAM, file, true)); + + fileIO.reset(); + fileIO.overwriteHintFile(file, "hint"); + assertThat(fileIO.fileContent(file)).isEqualTo("hint"); + assertThat(fileIO.calls(NEW_OUTPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_OUTPUT_STREAM, file, true)); + } + + @Test + void listingDefaultsReturnFilesOrDirectoriesWithoutEagerRelisting() throws Exception { + Path topFile = new Path(root, "top"); + Path directory = new Path(root, "directory"); + Path nestedFile = new Path(directory, "nested"); + fileIO.putFile(topFile, "top"); + fileIO.putDirectory(directory); + fileIO.putFile(nestedFile, "nested"); + fileIO.reset(); + + assertThat(fileIO.listFiles(root, false)) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(topFile); + assertThat(fileIO.callCount(LIST_STATUS)).isEqualTo(1); + assertNoMetadataPreflight(); + + fileIO.reset(); + assertThat(fileIO.listFiles(root, true)) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(topFile, nestedFile); + assertThat(fileIO.callCount(LIST_STATUS)).isLessThanOrEqualTo(2); + assertNoMetadataPreflight(); + + fileIO.reset(); + RemoteIterator iterator = fileIO.listFilesIterative(root, true); + assertThat(iterator.hasNext()).isTrue(); + assertThat(iterator.hasNext()).isTrue(); + assertThat(collect(iterator)) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(topFile, nestedFile); + assertThat(fileIO.callCount(LIST_STATUS)).isLessThanOrEqualTo(2); + assertNoMetadataPreflight(); + + fileIO.reset(); + assertThat(fileIO.listDirectories(root)) + .extracting(FileStatus::getPath) + .containsExactlyInAnyOrder(directory); + assertThat(fileIO.callCount(LIST_STATUS)).isEqualTo(1); + assertNoMetadataPreflight(); + + Path empty = new Path(root, "empty"); + fileIO.putDirectory(empty); + fileIO.reset(); + assertThat(fileIO.listFiles(empty, true)).isEmpty(); + assertThat(fileIO.listDirectories(empty)).isEmpty(); + + fileIO.reset(); + fileIO.failNext(LIST_STATUS, new IOException("lazy listing failed")); + RemoteIterator failing = fileIO.listFilesIterative(root, true); + assertThatThrownBy(failing::hasNext) + .isInstanceOf(IOException.class) + .hasMessage("lazy listing failed"); + } + + @Test + void copyDefaultsTransferBytesAndForwardOverwriteMode() throws Exception { + Path source = new Path(root, "source"); + Path target = new Path(root, "target"); + fileIO.putFile(source, "source-文件"); + fileIO.reset(); + + fileIO.copyFile(source, target, false); + + assertThat(fileIO.fileContent(target)).isEqualTo("source-文件"); + assertThat(fileIO.fileContent(source)).isEqualTo("source-文件"); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.openOutputStreams()).isZero(); + assertThat(fileIO.calls(NEW_INPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_INPUT_STREAM, source)); + assertThat(fileIO.calls(NEW_OUTPUT_STREAM)) + .containsExactly(RecordingFileIO.call(NEW_OUTPUT_STREAM, target, false)); + assertNoCopyPreflights(0); + + Path sourceDirectory = new Path(root, "sources"); + Path targetDirectory = new Path(root, "targets"); + Path first = new Path(sourceDirectory, "first"); + Path second = new Path(sourceDirectory, "second"); + fileIO.putDirectory(sourceDirectory); + fileIO.putDirectory(targetDirectory); + fileIO.putFile(first, "1"); + fileIO.putFile(second, "2"); + fileIO.reset(); + + fileIO.copyFiles(sourceDirectory, targetDirectory, true); + + assertThat(fileIO.fileContent(new Path(targetDirectory, "first"))).isEqualTo("1"); + assertThat(fileIO.fileContent(new Path(targetDirectory, "second"))).isEqualTo("2"); + assertThat(fileIO.fileContent(first)).isEqualTo("1"); + assertThat(fileIO.fileContent(second)).isEqualTo("2"); + assertThat(fileIO.callCount(LIST_STATUS)).isEqualTo(1); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(2); + assertThat(fileIO.calls(NEW_OUTPUT_STREAM)) + .containsExactlyInAnyOrder( + RecordingFileIO.call( + NEW_OUTPUT_STREAM, new Path(targetDirectory, "first"), true), + RecordingFileIO.call( + NEW_OUTPUT_STREAM, new Path(targetDirectory, "second"), true)); + assertNoCopyPreflights(1); + + Path failedTarget = new Path(root, "failed-target"); + fileIO.reset(); + fileIO.failNext(NEW_OUTPUT_STREAM, new IOException("target open failed")); + assertThatThrownBy(() -> fileIO.copyFile(source, failedTarget, false)) + .isInstanceOf(IOException.class) + .hasMessage("target open failed"); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.openOutputStreams()).isZero(); + assertThat(fileIO.existsInMemory(failedTarget)).isFalse(); + assertThat(fileIO.existsInMemory(source)).isTrue(); + assertNoCopyPreflights(0); + + fileIO.reset(); + fileIO.failNext(INPUT_READ, new IOException("source read failed")); + assertThatThrownBy(() -> fileIO.copyFile(source, new Path(root, "read-failed"), false)) + .isInstanceOf(IOException.class) + .hasMessage("source read failed"); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.openOutputStreams()).isZero(); + assertNoCopyPreflights(0); + + fileIO.reset(); + fileIO.failNext(OUTPUT_WRITE, new IOException("target write failed")); + assertThatThrownBy(() -> fileIO.copyFile(source, new Path(root, "write-failed"), false)) + .isInstanceOf(IOException.class) + .hasMessage("target write failed"); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.openOutputStreams()).isZero(); + assertNoCopyPreflights(0); + } + + @Test + void tryToWriteAtomicPublishesOrCleansUpTheTemporaryFile() throws Exception { + Path target = new Path(root, "atomic"); + + assertThat(fileIO.tryToWriteAtomic(target, "new")).isTrue(); + assertThat(fileIO.fileContent(target)).isEqualTo("new"); + assertThat(fileIO.callCount(NEW_OUTPUT_STREAM)).isEqualTo(1); + assertThat(fileIO.callCount(RENAME)).isEqualTo(1); + assertThat(fileIO.callCount(DELETE)).isZero(); + assertThat(fileIO.callCount(EXISTS)).isZero(); + assertThat(fileIO.callCount(GET_FILE_STATUS)).isZero(); + assertThat(fileIO.callCount(LIST_STATUS)).isZero(); + assertAtomicTemporaryPath(target, fileIO.calls(RENAME).get(0)); + + Path failedTarget = new Path(root, "failed-atomic"); + fileIO.reset(); + fileIO.failNext(RENAME, new IOException("rename failed")); + assertThatThrownBy(() -> fileIO.tryToWriteAtomic(failedTarget, "content")) + .isInstanceOf(IOException.class) + .hasMessage("rename failed"); + RecordingFileIO.Call failedRename = fileIO.calls(RENAME).get(0); + Path failedTemporary = failedRename.argument(0, Path.class); + assertThat(fileIO.existsInMemory(failedTemporary)).isFalse(); + assertThat(fileIO.existsInMemory(failedTarget)).isFalse(); + assertThat(fileIO.callCount(DELETE)).isEqualTo(1); + assertThat(fileIO.callCount(LIST_STATUS)).isZero(); + + fileIO.putFile(target, "existing"); + fileIO.reset(); + assertThat(fileIO.tryToWriteAtomic(target, "replacement")).isFalse(); + assertThat(fileIO.fileContent(target)).isEqualTo("existing"); + assertThat(fileIO.callCount(NEW_OUTPUT_STREAM)).isEqualTo(1); + assertThat(fileIO.callCount(RENAME)).isEqualTo(1); + assertThat(fileIO.callCount(DELETE)).isEqualTo(1); + assertThat(fileIO.callCount(EXISTS)).isZero(); + assertThat(fileIO.callCount(LIST_STATUS)).isZero(); + assertAtomicTemporaryPath(target, fileIO.calls(RENAME).get(0)); + } + + @Test + void defaultTwoPhaseOutputStagesThenPublishesOnCommit() throws Exception { + Path target = new Path(root, "two-phase"); + String content = "staged-文件"; + + TwoPhaseOutputStream stream = fileIO.newTwoPhaseOutputStream(target, false); + stream.write(content.getBytes(StandardCharsets.UTF_8)); + TwoPhaseOutputStream.Committer committer = stream.closeForCommit(); + + assertThat(fileIO.existsInMemory(target)).isFalse(); + assertThat(committer.targetPath()).isEqualTo(target); + committer.commit(fileIO); + assertThat(fileIO.fileContent(target)).isEqualTo(content); + assertThat(fileIO.openOutputStreams()).isZero(); + } + + @Test + void overwrittenReadReturnsEmptyForMissingFilesWithoutExistenceProbe() throws Exception { + Path missing = new Path(root, "missing"); + + assertThat(fileIO.readOverwrittenFileUtf8(missing)).isEmpty(); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(1); + assertThat(fileIO.callCount(EXISTS)).isZero(); + + Path disappeared = new Path(root, "disappeared"); + fileIO.reset(); + fileIO.failNext(NEW_INPUT_STREAM, new IOException("transient")); + assertThat(fileIO.readOverwrittenFileUtf8(disappeared)).isEmpty(); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(1); + } + + @Test + void overwrittenReadRetriesOnlyKnownConcurrentChangeFailures() throws Exception { + Path file = new Path(root, "overwritten"); + fileIO.putFile(file, "stable"); + fileIO.failNext(NEW_INPUT_STREAM, blocklistChanged()); + fileIO.failNext(NEW_INPUT_STREAM, blocklistChanged()); + + assertThat(fileIO.readOverwrittenFileUtf8(file)).contains("stable"); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(3); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(2); + + fileIO.reset(); + fileIO.failNext(NEW_INPUT_STREAM, new IOException("unrelated")); + assertThatThrownBy(() -> fileIO.readOverwrittenFileUtf8(file)) + .isInstanceOf(IOException.class) + .hasMessage("unrelated"); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(1); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(1); + } + + @Test + void overwrittenReadStopsAfterFiveKnownFailures() { + Path file = new Path(root, "overwritten"); + fileIO.putFile(file, "stable"); + for (int i = 0; i < 5; i++) { + fileIO.failNext(NEW_INPUT_STREAM, blocklistChanged()); + } + + assertThatThrownBy(() -> fileIO.readOverwrittenFileUtf8(file)) + .isInstanceOf(IOException.class) + .hasMessageContaining("Blocklist for"); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(5); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(5); + } + + private static IOException blocklistChanged() { + return new IOException("Blocklist for test has changed"); + } + + private void assertNoMetadataPreflight() { + assertThat(fileIO.callCount(GET_FILE_STATUS)).isZero(); + assertThat(fileIO.callCount(EXISTS)).isZero(); + } + + private void assertNoCopyPreflights(long expectedListCalls) { + assertNoMetadataPreflight(); + assertThat(fileIO.callCount(DELETE)).isZero(); + assertThat(fileIO.callCount(LIST_STATUS)).isEqualTo(expectedListCalls); + } + + private void assertOnlyCalls(RecordingFileIO.Method... allowedMethods) { + List allowed = Arrays.asList(allowedMethods); + assertThat(fileIO.calls()).allMatch(call -> allowed.contains(call.method())); + } + + private static List collect(RemoteIterator iterator) + throws IOException { + List result = new ArrayList<>(); + while (iterator.hasNext()) { + result.add(iterator.next()); + } + return result; + } + + private static void assertAtomicTemporaryPath(Path target, RecordingFileIO.Call renameCall) { + Path temporary = renameCall.argument(0, Path.class); + assertThat(renameCall.argument(1, Path.class)).isEqualTo(target); + assertThat(temporary.getParent()).isEqualTo(target.getParent()); + assertThat(temporary.getName()).startsWith("." + target.getName() + ".").endsWith(".tmp"); + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOReturnTypeTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOReturnTypeTest.java new file mode 100644 index 000000000000..0ffc4015d9ec --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOReturnTypeTest.java @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.junit.jupiter.api.Test; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStream; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.same; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** Tests default return values and adapters in the public file I/O API. */ +public class FileIOReturnTypeTest { + + @Test + public void testFileStatusDefaultValues() { + FileStatus status = + new FileStatus() { + @Override + public long getLen() { + return 0; + } + + @Override + public boolean isDir() { + return false; + } + + @Override + public Path getPath() { + return new Path("file:/status"); + } + + @Override + public long getModificationTime() { + return 0; + } + }; + + assertThat(status.getAccessTime()).isZero(); + assertThat(status.getOwner()).isNull(); + } + + @Test + public void testSeekableInputStreamWrapForwardsReadAndClose() throws IOException { + InputStream input = mock(InputStream.class); + byte[] buffer = new byte[4]; + when(input.read()).thenReturn(17); + when(input.read(buffer, 1, 2)).thenReturn(2); + + SeekableInputStream wrapped = SeekableInputStream.wrap(input); + assertThat(wrapped.read()).isEqualTo(17); + assertThat(wrapped.read(buffer, 1, 2)).isEqualTo(2); + wrapped.close(); + + verify(input).read(); + verify(input).read(same(buffer), eq(1), eq(2)); + verify(input).close(); + } + + @Test + public void testSeekableInputStreamWrapRejectsSeek() { + SeekableInputStream wrapped = + SeekableInputStream.wrap(new ByteArrayInputStream(new byte[0])); + + assertThatThrownBy(() -> wrapped.seek(0)).isInstanceOf(UnsupportedOperationException.class); + } + + @Test + public void testSeekableInputStreamWrapRejectsGetPos() { + SeekableInputStream wrapped = + SeekableInputStream.wrap(new ByteArrayInputStream(new byte[0])); + + assertThatThrownBy(wrapped::getPos).isInstanceOf(UnsupportedOperationException.class); + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java index 96e023d1eb46..db77750d4272 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java @@ -19,12 +19,15 @@ package org.apache.paimon.fs; import org.apache.paimon.catalog.CatalogContext; +import org.apache.paimon.data.BlobDescriptor; import org.apache.paimon.fs.local.LocalFileIO; +import org.apache.paimon.options.CatalogOptions; import org.apache.paimon.options.Options; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.mockito.MockedStatic; import java.io.File; import java.io.FileNotFoundException; @@ -37,14 +40,20 @@ import java.nio.file.StandardCopyOption; import java.time.Duration; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; +import java.util.List; import java.util.Optional; +import java.util.ServiceLoader; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReentrantLock; import static org.apache.paimon.utils.Preconditions.checkState; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; /** Test static methods and methods with default implementations of {@link FileIO}. */ public class FileIOTest { @@ -74,6 +83,91 @@ public void testRequireOptions() throws IOException { assertThat(fileIO).isInstanceOf(RequireOptionsFileIOLoader.MyFileIO.class); } + @Test + public void testGetSchemelessPathUsesLocalFileIO() throws IOException { + Path path = new Path(tempDir.resolve("local").toString()); + + FileIO fileIO = FileIO.get(path, CatalogContext.create(new Options())); + + assertThat(fileIO).isInstanceOf(LocalFileIO.class); + } + + @Test + public void testGetUsesResolvingFileIOWhenEnabled() throws IOException { + Options options = new Options(); + options.set(CatalogOptions.RESOLVING_FILE_IO_ENABLED, true); + + FileIO fileIO = + FileIO.get( + new Path(tempDir.resolve("resolving").toUri()), + CatalogContext.create(options)); + + assertThat(fileIO).isInstanceOf(ResolvingFileIO.class); + } + + @Test + public void testGetRejectsLocalPathWithAuthority() { + Path malformed = new Path("file://host/tmp/table"); + + assertThatThrownBy(() -> FileIO.get(malformed, CatalogContext.create(new Options()))) + .isInstanceOf(IOException.class) + .hasMessageContaining("authority 'host'") + .hasMessageContaining("file:///host/tmp/table"); + } + + @Test + public void testDiscoverLoadersIncludesTestService() { + assertThat(FileIO.discoverLoaders().get("require-options")) + .isInstanceOf(RequireOptionsFileIOLoader.class); + } + + @Test + @SuppressWarnings("unchecked") + public void testDiscoverLoadersRejectsDuplicateSchemes() { + FileIOLoader first = new TrackingLoader("duplicate"); + FileIOLoader second = new TrackingLoader("duplicate"); + ServiceLoader services = mock(ServiceLoader.class); + when(services.iterator()).thenReturn(Arrays.asList(first, second).iterator()); + + try (MockedStatic serviceLoader = mockStatic(ServiceLoader.class)) { + serviceLoader + .when( + () -> + ServiceLoader.load( + FileIOLoader.class, + FileIOLoader.class.getClassLoader())) + .thenReturn(services); + + assertThatThrownBy(FileIO::discoverLoaders) + .isInstanceOf(RuntimeException.class) + .hasMessageContaining("Multiple FileIO for scheme 'duplicate'"); + } + } + + @Test + public void testFailedPreferredLoaderFallsBackToAccessibleLoader() throws IOException { + TrackingLoader preferred = new TrackingLoader("preferred", "required-by-preferred"); + TrackingLoader fallback = new TrackingLoader("fallback"); + Path path = new Path("unregistered:///warehouse"); + + FileIO selected = + FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); + + assertThat(selected).isSameAs(fallback.fileIO); + } + + @Test + public void testAccessiblePreferredLoaderIsSelectedBeforeFallback() throws IOException { + TrackingLoader preferred = new TrackingLoader("preferred"); + TrackingLoader fallback = new TrackingLoader("fallback"); + Path path = new Path("unregistered:///warehouse"); + + FileIO selected = + FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); + + assertThat(selected).isSameAs(preferred.fileIO); + } + @Test public void testCopy() throws Exception { Path srcFile = new Path(tempDir.resolve("src.txt").toUri()); @@ -196,6 +290,63 @@ public void testDefaultArchiveUnsupported() { .isInstanceOf(UnsupportedOperationException.class) .hasMessageContaining(DummyFileIO.class.getName()) .hasMessageContaining("unarchive"); + + BlobDescriptor descriptor = new BlobDescriptor(path.toString(), 0, 1); + assertThatThrownBy( + () -> + fileIO.createBlobPresignedUrl( + new Path(tempDir.toUri()), + descriptor, + Duration.ofMinutes(5))) + .isInstanceOf(UnsupportedOperationException.class) + .hasMessageContaining(DummyFileIO.class.getName()) + .hasMessageContaining("presigned"); + } + + private static class TrackingLoader implements FileIOLoader { + + private static final long serialVersionUID = 1L; + + private final String scheme; + private final TrackingLocalFileIO fileIO; + private final String requiredOption; + + private TrackingLoader(String scheme) { + this(scheme, null); + } + + private TrackingLoader(String scheme, String requiredOption) { + this.scheme = scheme; + this.fileIO = new TrackingLocalFileIO(); + this.requiredOption = requiredOption; + } + + @Override + public String getScheme() { + return scheme; + } + + @Override + public List requiredOptions() { + return requiredOption == null + ? Collections.emptyList() + : Collections.singletonList(new String[] {requiredOption}); + } + + @Override + public FileIO load(Path path) { + return fileIO; + } + } + + private static class TrackingLocalFileIO extends LocalFileIO { + + private static final long serialVersionUID = 1L; + + @Override + public boolean exists(Path path) { + return true; + } } /** A {@link FileIO} on local filesystem to test various default implementations. */ diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java index e5946a049431..14f4887ee279 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/HadoopLocalFileIOBehaviorTest.java @@ -23,10 +23,12 @@ import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.RawLocalFileSystem; import org.apache.hadoop.util.VersionInfo; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; import java.net.URI; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assumptions.assumeThat; /** Behavior tests for Hadoop Local. */ @@ -48,6 +50,11 @@ protected Path getBasePath() { return new Path(tmp.toUri()); } + @Test + void testIsNotObjectStore() throws Exception { + assertThat(getFileSystem().isObjectStore()).isFalse(); + } + // ------------------------------------------------------------------------ /** This test needs to be skipped for earlier Hadoop versions because those have a bug. */ diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java index cdb0dfaa7637..209921fe3f09 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/LocalFileIOBehaviorTest.java @@ -20,8 +20,11 @@ import org.apache.paimon.fs.local.LocalFileIO; +import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import static org.assertj.core.api.Assertions.assertThat; + /** Test for {@link LocalFileIO}. */ public class LocalFileIOBehaviorTest extends FileIOContractTestBase { @@ -36,4 +39,9 @@ protected FileIO getFileSystem() { protected Path getBasePath() { return new Path(tmp.toUri()); } + + @Test + void testIsNotObjectStore() { + assertThat(new LocalFileIO().isObjectStore()).isFalse(); + } } diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIO.java b/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIO.java new file mode 100644 index 000000000000..0ebeeb1b7fdf --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIO.java @@ -0,0 +1,427 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.apache.paimon.catalog.CatalogContext; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileAlreadyExistsException; +import java.util.ArrayDeque; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Comparator; +import java.util.Deque; +import java.util.EnumMap; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; + +/** Deterministic in-memory implementation of the primitive {@link FileIO} operations. */ +final class RecordingFileIO implements FileIO { + + enum Method { + GET_FILE_STATUS, + LIST_STATUS, + EXISTS, + DELETE, + MKDIRS, + RENAME, + NEW_INPUT_STREAM, + NEW_OUTPUT_STREAM, + INPUT_READ, + OUTPUT_WRITE + } + + static final class Call { + private final Method method; + private final List arguments; + + private Call(Method method, Object... arguments) { + this.method = method; + this.arguments = Arrays.asList(arguments); + } + + Method method() { + return method; + } + + T argument(int index, Class type) { + return type.cast(arguments.get(index)); + } + + @Override + public boolean equals(Object other) { + if (!(other instanceof Call)) { + return false; + } + Call that = (Call) other; + return method == that.method && arguments.equals(that.arguments); + } + + @Override + public int hashCode() { + return Objects.hash(method, arguments); + } + + @Override + public String toString() { + return method + arguments.toString(); + } + } + + private final Map files = new LinkedHashMap<>(); + private final Set directories = new LinkedHashSet<>(); + private final List calls = new ArrayList<>(); + private final Map> failures = new EnumMap<>(Method.class); + private int openInputStreams; + private int openOutputStreams; + + static Call call(Method method, Object... arguments) { + return new Call(method, arguments); + } + + void putFile(Path path, String content) { + addParentDirectories(path); + files.put(path, content.getBytes(StandardCharsets.UTF_8)); + } + + void putDirectory(Path path) { + addParentDirectories(path); + directories.add(path); + } + + String fileContent(Path path) { + return new String(files.get(path), StandardCharsets.UTF_8); + } + + List calls() { + return new ArrayList<>(calls); + } + + List calls(Method method) { + return calls.stream().filter(call -> call.method == method).collect(Collectors.toList()); + } + + long callCount(Method method) { + return calls.stream().filter(call -> call.method == method).count(); + } + + boolean existsInMemory(Path path) { + return files.containsKey(path) || directories.contains(path); + } + + boolean isDirectoryInMemory(Path path) { + return directories.contains(path); + } + + int openInputStreams() { + return openInputStreams; + } + + int openOutputStreams() { + return openOutputStreams; + } + + void failNext(Method method, IOException failure) { + failures.computeIfAbsent(method, ignored -> new ArrayDeque<>()).add(failure); + } + + void reset() { + calls.clear(); + failures.clear(); + } + + @Override + public boolean isObjectStore() { + return false; + } + + @Override + public void configure(CatalogContext context) {} + + @Override + public SeekableInputStream newInputStream(Path path) throws IOException { + calls.add(call(Method.NEW_INPUT_STREAM, path)); + maybeFail(Method.NEW_INPUT_STREAM); + byte[] content = files.get(path); + if (content == null) { + throw new FileNotFoundException(path.toString()); + } + openInputStreams++; + return new SeekableInputStream() { + private final ByteArrayInputStream input = new ByteArrayInputStream(content); + private long position; + private boolean closed; + + @Override + public void seek(long desired) throws IOException { + if (desired < 0 || desired > content.length) { + throw new IOException("Invalid seek position " + desired); + } + input.reset(); + long skipped = input.skip(desired); + if (skipped != desired) { + throw new IOException("Could not seek to " + desired); + } + position = desired; + } + + @Override + public long getPos() { + return position; + } + + @Override + public int read() throws IOException { + maybeFail(Method.INPUT_READ); + int value = input.read(); + if (value >= 0) { + position++; + } + return value; + } + + @Override + public int read(byte[] bytes, int offset, int length) throws IOException { + maybeFail(Method.INPUT_READ); + int read = input.read(bytes, offset, length); + if (read > 0) { + position += read; + } + return read; + } + + @Override + public void close() { + if (!closed) { + closed = true; + openInputStreams--; + } + } + }; + } + + @Override + public PositionOutputStream newOutputStream(Path path, boolean overwrite) throws IOException { + calls.add(call(Method.NEW_OUTPUT_STREAM, path, overwrite)); + maybeFail(Method.NEW_OUTPUT_STREAM); + if (!overwrite && existsInMemory(path)) { + throw new FileAlreadyExistsException(path.toString()); + } + openOutputStreams++; + ByteArrayOutputStream output = new ByteArrayOutputStream(); + return new PositionOutputStream() { + private boolean closed; + + @Override + public long getPos() { + return output.size(); + } + + @Override + public void write(int value) throws IOException { + maybeFail(Method.OUTPUT_WRITE); + output.write(value); + } + + @Override + public void write(byte[] bytes) throws IOException { + maybeFail(Method.OUTPUT_WRITE); + output.write(bytes); + } + + @Override + public void write(byte[] bytes, int offset, int length) throws IOException { + maybeFail(Method.OUTPUT_WRITE); + output.write(bytes, offset, length); + } + + @Override + public void flush() throws IOException { + output.flush(); + } + + @Override + public void close() { + if (!closed) { + closed = true; + addParentDirectories(path); + files.put(path, output.toByteArray()); + openOutputStreams--; + } + } + }; + } + + @Override + public FileStatus getFileStatus(Path path) throws IOException { + calls.add(call(Method.GET_FILE_STATUS, path)); + maybeFail(Method.GET_FILE_STATUS); + if (files.containsKey(path)) { + return new MemoryFileStatus(path, false, files.get(path).length); + } + if (directories.contains(path)) { + return new MemoryFileStatus(path, true, 0); + } + throw new FileNotFoundException(path.toString()); + } + + @Override + public FileStatus[] listStatus(Path path) throws IOException { + calls.add(call(Method.LIST_STATUS, path)); + maybeFail(Method.LIST_STATUS); + if (!directories.contains(path)) { + throw new FileNotFoundException(path.toString()); + } + List statuses = new ArrayList<>(); + for (Path directory : directories) { + if (!directory.equals(path) && path.equals(directory.getParent())) { + statuses.add(new MemoryFileStatus(directory, true, 0)); + } + } + for (Map.Entry file : files.entrySet()) { + if (path.equals(file.getKey().getParent())) { + statuses.add(new MemoryFileStatus(file.getKey(), false, file.getValue().length)); + } + } + statuses.sort(Comparator.comparing(FileStatus::getPath)); + return statuses.toArray(new FileStatus[0]); + } + + @Override + public boolean exists(Path path) throws IOException { + calls.add(call(Method.EXISTS, path)); + maybeFail(Method.EXISTS); + return existsInMemory(path); + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + calls.add(call(Method.DELETE, path, recursive)); + maybeFail(Method.DELETE); + if (files.remove(path) != null) { + return true; + } + if (!directories.contains(path)) { + return false; + } + boolean hasChildren = + files.keySet().stream().anyMatch(child -> isDescendant(child, path)) + || directories.stream() + .anyMatch( + child -> !child.equals(path) && isDescendant(child, path)); + if (hasChildren && !recursive) { + return false; + } + files.keySet().removeIf(child -> isDescendant(child, path)); + directories.removeIf(child -> child.equals(path) || isDescendant(child, path)); + return true; + } + + @Override + public boolean mkdirs(Path path) throws IOException { + calls.add(call(Method.MKDIRS, path)); + maybeFail(Method.MKDIRS); + boolean missing = !directories.contains(path); + putDirectory(path); + return missing; + } + + @Override + public boolean rename(Path src, Path dst) throws IOException { + calls.add(call(Method.RENAME, src, dst)); + maybeFail(Method.RENAME); + if (existsInMemory(dst)) { + return false; + } + byte[] content = files.remove(src); + if (content == null) { + return false; + } + addParentDirectories(dst); + files.put(dst, content); + return true; + } + + private void maybeFail(Method method) throws IOException { + Deque scripted = failures.get(method); + if (scripted != null && !scripted.isEmpty()) { + throw scripted.remove(); + } + } + + private void addParentDirectories(Path path) { + Path parent = path.getParent(); + while (parent != null) { + directories.add(parent); + parent = parent.getParent(); + } + } + + private static boolean isDescendant(Path candidate, Path parent) { + Path current = candidate.getParent(); + while (current != null) { + if (current.equals(parent)) { + return true; + } + current = current.getParent(); + } + return false; + } + + private static final class MemoryFileStatus implements FileStatus { + private final Path path; + private final boolean directory; + private final long length; + + private MemoryFileStatus(Path path, boolean directory, long length) { + this.path = path; + this.directory = directory; + this.length = length; + } + + @Override + public long getLen() { + return length; + } + + @Override + public boolean isDir() { + return directory; + } + + @Override + public Path getPath() { + return path; + } + + @Override + public long getModificationTime() { + return 0; + } + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIOTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIOTest.java new file mode 100644 index 000000000000..759cfb77c207 --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/RecordingFileIOTest.java @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.apache.paimon.fs.RecordingFileIO.Method.NEW_INPUT_STREAM; +import static org.apache.paimon.fs.RecordingFileIO.Method.NEW_OUTPUT_STREAM; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Tests for {@link RecordingFileIO}. */ +class RecordingFileIOTest { + + @Test + void recordsTypedPrimitiveArgumentsWithoutLosingFileStateOnReset() throws Exception { + RecordingFileIO fileIO = new RecordingFileIO(); + Path path = new Path("test:///data/value.txt"); + fileIO.putFile(path, "old"); + + fileIO.reset(); + fileIO.writeFile(path, "new", true); + + assertThat(fileIO.fileContent(path)).isEqualTo("new"); + assertThat(fileIO.calls()) + .containsExactly(RecordingFileIO.call(NEW_OUTPUT_STREAM, path, true)); + } + + @Test + void scriptsOneShotPrimitiveFailuresAndResetClearsThem() throws Exception { + RecordingFileIO fileIO = new RecordingFileIO(); + Path path = new Path("test:///data/value.txt"); + fileIO.putFile(path, "value"); + fileIO.failNext(NEW_INPUT_STREAM, new IOException("planned")); + + assertThatThrownBy(() -> fileIO.readFileUtf8(path)) + .isInstanceOf(IOException.class) + .hasMessage("planned"); + assertThat(fileIO.calls()).containsExactly(RecordingFileIO.call(NEW_INPUT_STREAM, path)); + + fileIO.failNext(NEW_INPUT_STREAM, new IOException("cleared")); + fileIO.reset(); + assertThat(fileIO.readFileUtf8(path)).isEqualTo("value"); + } + + @Test + void tracksOpenStreamsUntilTheyAreClosed() throws Exception { + RecordingFileIO fileIO = new RecordingFileIO(); + Path source = new Path("test:///data/source.txt"); + Path target = new Path("test:///data/target.txt"); + fileIO.putFile(source, "value"); + + SeekableInputStream input = fileIO.newInputStream(source); + PositionOutputStream output = fileIO.newOutputStream(target, false); + assertThat(fileIO.openInputStreams()).isEqualTo(1); + assertThat(fileIO.openOutputStreams()).isEqualTo(1); + + input.close(); + output.close(); + assertThat(fileIO.openInputStreams()).isZero(); + assertThat(fileIO.openOutputStreams()).isZero(); + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIO.java b/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIO.java new file mode 100644 index 000000000000..fd3abf376622 --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIO.java @@ -0,0 +1,347 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.apache.paimon.catalog.CatalogContext; +import org.apache.paimon.data.BlobDescriptor; + +import java.io.IOException; +import java.time.Duration; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +/** Test-only {@link FileIO} which checks the portable input domain of selected operations. */ +public final class StrictContractFileIO implements FileIO { + + private static final long serialVersionUID = 1L; + + private final FileIO delegate; + + public StrictContractFileIO(FileIO delegate) { + this.delegate = delegate; + } + + @Override + public boolean isObjectStore() { + return delegate.isObjectStore(); + } + + @Override + public void configure(CatalogContext context) { + delegate.configure(context); + } + + @Override + public void setRuntimeContext(Map options) { + delegate.setRuntimeContext(options); + } + + @Override + public SeekableInputStream newInputStream(Path path) throws IOException { + return delegate.newInputStream(path); + } + + @Override + public PositionOutputStream newOutputStream(Path path, boolean overwrite) throws IOException { + return delegate.newOutputStream(path, overwrite); + } + + @Override + public TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrite) + throws IOException { + return new ForwardingTwoPhaseOutputStream( + delegate.newTwoPhaseOutputStream(path, overwrite)); + } + + @Override + public FileStatus getFileStatus(Path path) throws IOException { + return delegate.getFileStatus(path); + } + + @Override + public FileStatus[] listStatus(Path path) throws IOException { + requireDirectory(path, "listStatus"); + return delegate.listStatus(path); + } + + @Override + public FileStatus[] listFiles(Path path, boolean recursive) throws IOException { + requireDirectory(path, "listFiles"); + return delegate.listFiles(path, recursive); + } + + @Override + public RemoteIterator listFilesIterative(Path path, boolean recursive) + throws IOException { + requireDirectory(path, "listFilesIterative"); + return delegate.listFilesIterative(path, recursive); + } + + @Override + public FileStatus[] listDirectories(Path path) throws IOException { + requireDirectory(path, "listDirectories"); + return delegate.listDirectories(path); + } + + @Override + public boolean exists(Path path) throws IOException { + return delegate.exists(path); + } + + @Override + public boolean delete(Path path, boolean recursive) throws IOException { + return delegate.delete(path, recursive); + } + + @Override + public boolean mkdirs(Path path) throws IOException { + return delegate.mkdirs(path); + } + + @Override + public boolean rename(Path src, Path dst) throws IOException { + requireDistinctPaths(src, dst); + requireExisting(src, "rename source"); + requireMissing(dst, "rename destination"); + Path parent = dst.getParent(); + if (parent == null) { + throw violation("rename destination has no parent: " + dst); + } + requireDirectory(parent, "rename destination parent"); + return delegate.rename(src, dst); + } + + @Override + public Optional archive(Path path, StorageType type) throws IOException { + return delegate.archive(path, type); + } + + @Override + public void restoreArchive(Path path, Duration duration) throws IOException { + delegate.restoreArchive(path, duration); + } + + @Override + public Optional unarchive(Path path, StorageType type) throws IOException { + return delegate.unarchive(path, type); + } + + @Override + public String createBlobPresignedUrl( + Path tableRoot, BlobDescriptor descriptor, Duration validity) throws IOException { + return delegate.createBlobPresignedUrl(tableRoot, descriptor, validity); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + + @Override + public void deleteQuietly(Path file) { + delegate.deleteQuietly(file); + } + + @Override + public void deleteFilesQuietly(List files) { + delegate.deleteFilesQuietly(files); + } + + @Override + public void deleteDirectoryQuietly(Path directory) { + delegate.deleteDirectoryQuietly(directory); + } + + @Override + public long getFileSize(Path path) throws IOException { + return delegate.getFileSize(path); + } + + @Override + public boolean isDir(Path path) throws IOException { + return delegate.isDir(path); + } + + @Override + public void checkOrMkdirs(Path path) throws IOException { + delegate.checkOrMkdirs(path); + } + + @Override + public String readFileUtf8(Path path) throws IOException { + return delegate.readFileUtf8(path); + } + + @Override + public boolean tryToWriteAtomic(Path path, String content) throws IOException { + return delegate.tryToWriteAtomic(path, content); + } + + @Override + public void writeFile(Path path, String content, boolean overwrite) throws IOException { + delegate.writeFile(path, content, overwrite); + } + + @Override + public void overwriteFileUtf8(Path path, String content) throws IOException { + delegate.overwriteFileUtf8(path, content); + } + + @Override + public void overwriteHintFile(Path path, String content) throws IOException { + delegate.overwriteHintFile(path, content); + } + + @Override + public void copyFile(Path sourcePath, Path targetPath, boolean overwrite) throws IOException { + delegate.copyFile(sourcePath, targetPath, overwrite); + } + + @Override + public void copyFiles(Path sourceDirectory, Path targetDirectory, boolean overwrite) + throws IOException { + requireDirectory(sourceDirectory, "copyFiles source"); + delegate.copyFiles(sourceDirectory, targetDirectory, overwrite); + } + + @Override + public Optional readOverwrittenFileUtf8(Path path) throws IOException { + return delegate.readOverwrittenFileUtf8(path); + } + + private void requireExisting(Path path, String operation) throws IOException { + if (!delegate.exists(path)) { + throw violation(operation + " does not exist: " + path); + } + } + + private void requireMissing(Path path, String operation) throws IOException { + if (delegate.exists(path)) { + throw violation(operation + " already exists: " + path); + } + } + + private void requireDirectory(Path path, String operation) throws IOException { + final FileStatus status; + try { + status = delegate.getFileStatus(path); + } catch (IOException e) { + throw violation(operation + " requires an existing directory: " + path, e); + } + if (!status.isDir()) { + throw violation(operation + " requires a directory: " + path); + } + } + + private static void requireDistinctPaths(Path src, Path dst) { + if (src.equals(dst)) { + throw violation("rename source and destination are the same path: " + src); + } + } + + private static AssertionError violation(String message) { + return new AssertionError(message); + } + + private static AssertionError violation(String message, Exception cause) { + return new AssertionError(message, cause); + } + + private static FileIO unwrap(FileIO fileIO) { + return fileIO instanceof StrictContractFileIO + ? ((StrictContractFileIO) fileIO).delegate + : fileIO; + } + + private static final class ForwardingTwoPhaseOutputStream extends TwoPhaseOutputStream { + + private final TwoPhaseOutputStream delegate; + + private ForwardingTwoPhaseOutputStream(TwoPhaseOutputStream delegate) { + this.delegate = delegate; + } + + @Override + public void write(int b) throws IOException { + delegate.write(b); + } + + @Override + public void write(byte[] b) throws IOException { + delegate.write(b); + } + + @Override + public void write(byte[] b, int off, int len) throws IOException { + delegate.write(b, off, len); + } + + @Override + public void flush() throws IOException { + delegate.flush(); + } + + @Override + public long getPos() throws IOException { + return delegate.getPos(); + } + + @Override + public void close() throws IOException { + delegate.close(); + } + + @Override + public Committer closeForCommit() throws IOException { + return new UnwrappingCommitter(delegate.closeForCommit()); + } + } + + private static final class UnwrappingCommitter implements TwoPhaseOutputStream.Committer { + + private static final long serialVersionUID = 1L; + + private final TwoPhaseOutputStream.Committer delegate; + + private UnwrappingCommitter(TwoPhaseOutputStream.Committer delegate) { + this.delegate = delegate; + } + + @Override + public void commit(FileIO fileIO) throws IOException { + delegate.commit(unwrap(fileIO)); + } + + @Override + public void discard(FileIO fileIO) throws IOException { + delegate.discard(unwrap(fileIO)); + } + + @Override + public Path targetPath() { + return delegate.targetPath(); + } + + @Override + public void clean(FileIO fileIO) throws IOException { + delegate.clean(unwrap(fileIO)); + } + } +} diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIOTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIOTest.java new file mode 100644 index 000000000000..68907d9545e1 --- /dev/null +++ b/paimon-common/src/test/java/org/apache/paimon/fs/StrictContractFileIOTest.java @@ -0,0 +1,182 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.apache.paimon.fs.local.LocalFileIO; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.FileNotFoundException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; +import java.time.Duration; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** Tests for {@link StrictContractFileIO}. */ +class StrictContractFileIOTest { + + @TempDir private java.nio.file.Path tempDir; + + private FileIO delegate; + private StrictContractFileIO strict; + private Path root; + + @BeforeEach + void before() throws Exception { + delegate = new LocalFileIO(); + strict = new StrictContractFileIO(delegate); + root = new Path(tempDir.toUri()); + delegate.mkdirs(root); + } + + @Test + void testOverridesEveryFileIOInstanceMethod() throws Exception { + for (Method method : FileIO.class.getDeclaredMethods()) { + if (Modifier.isPublic(method.getModifiers()) + && !Modifier.isStatic(method.getModifiers()) + && !method.isSynthetic()) { + assertThat( + StrictContractFileIO.class + .getMethod(method.getName(), method.getParameterTypes()) + .getDeclaringClass()) + .as(method.toString()) + .isEqualTo(StrictContractFileIO.class); + } + } + } + + @Test + void testListingRequiresExistingDirectory() throws Exception { + Path file = new Path(root, "file"); + delegate.writeFile(file, "content", false); + Path missing = new Path(root, "missing"); + + assertThat(strict.listStatus(root)).extracting(FileStatus::getPath).containsExactly(file); + assertThatThrownBy(() -> strict.listStatus(file)).isInstanceOf(AssertionError.class); + assertThatThrownBy(() -> strict.listFilesIterative(file, false)) + .isInstanceOf(AssertionError.class); + assertThatThrownBy(() -> strict.listFiles(missing, true)) + .isInstanceOf(AssertionError.class); + assertThatThrownBy(() -> strict.listDirectories(missing)) + .isInstanceOf(AssertionError.class); + } + + @Test + void testRenameAllowsExactMissingDestination() throws Exception { + Path source = new Path(root, "source"); + Path destination = new Path(root, "destination"); + delegate.writeFile(source, "content", false); + + assertThat(strict.rename(source, destination)).isTrue(); + + assertThat(delegate.exists(source)).isFalse(); + assertThat(delegate.readFileUtf8(destination)).isEqualTo("content"); + } + + @Test + void testRenameRejectsUnspecifiedShapesBeforeMutation() throws Exception { + Path source = new Path(root, "source"); + Path destination = new Path(root, "destination"); + delegate.writeFile(source, "source", false); + delegate.writeFile(destination, "destination", false); + + assertThatThrownBy(() -> strict.rename(source, source)).isInstanceOf(AssertionError.class); + assertThatThrownBy(() -> strict.rename(source, destination)) + .isInstanceOf(AssertionError.class); + assertThatThrownBy( + () -> + strict.rename( + new Path(root, "missing-source"), + new Path(root, "missing-destination"))) + .isInstanceOf(AssertionError.class); + assertThatThrownBy( + () -> + strict.rename( + source, + new Path(new Path(root, "missing-parent"), "target"))) + .isInstanceOf(AssertionError.class); + + Path fileParent = new Path(root, "file-parent"); + delegate.writeFile(fileParent, "not-a-directory", false); + assertThatThrownBy(() -> strict.rename(source, new Path(fileParent, "target"))) + .isInstanceOf(AssertionError.class); + + assertThat(delegate.readFileUtf8(source)).isEqualTo("source"); + assertThat(delegate.readFileUtf8(destination)).isEqualTo("destination"); + } + + @Test + void testCopyFilesRequiresExistingSourceDirectory() throws Exception { + Path sourceDirectory = new Path(root, "source"); + Path targetDirectory = new Path(root, "target"); + delegate.mkdirs(sourceDirectory); + delegate.mkdirs(targetDirectory); + delegate.writeFile(new Path(sourceDirectory, "file"), "content", false); + + strict.copyFiles(sourceDirectory, targetDirectory, false); + + assertThat(delegate.readFileUtf8(new Path(targetDirectory, "file"))).isEqualTo("content"); + assertThatThrownBy( + () -> + strict.copyFiles( + new Path(sourceDirectory, "file"), targetDirectory, false)) + .isInstanceOf(AssertionError.class); + assertThatThrownBy( + () -> + strict.copyFiles( + new Path(root, "missing-source"), targetDirectory, false)) + .isInstanceOf(AssertionError.class); + } + + @Test + void testDocumentedErrorPathsAreForwarded() throws Exception { + Path missing = new Path(root, "missing"); + Path existing = new Path(root, "existing"); + delegate.writeFile(existing, "old", false); + + assertThatThrownBy(() -> strict.getFileStatus(missing)) + .isInstanceOf(FileNotFoundException.class); + assertThatThrownBy(() -> strict.writeFile(existing, "new", false)) + .isInstanceOf(Exception.class); + assertThat(strict.tryToWriteAtomic(existing, "new")).isFalse(); + assertThat(delegate.readFileUtf8(existing)).isEqualTo("old"); + assertThatThrownBy(() -> strict.archive(existing, StorageType.ARCHIVE)) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> strict.restoreArchive(existing, Duration.ofMinutes(1))) + .isInstanceOf(UnsupportedOperationException.class); + } + + @Test + void testTwoPhaseCommitUsesTheProviderWithoutReapplyingCallerGuards() throws Exception { + Path target = new Path(root, "two-phase"); + delegate.writeFile(target, "old", false); + + TwoPhaseOutputStream output = strict.newTwoPhaseOutputStream(target, true); + output.write("replacement".getBytes()); + TwoPhaseOutputStream.Committer committer = output.closeForCommit(); + committer.commit(strict); + committer.clean(strict); + + assertThat(delegate.readFileUtf8(target)).isEqualTo("replacement"); + } +} diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index 278e4c4f369f..1b7548969c14 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -106,6 +106,7 @@ public FormatTableCommit( @Override public void commit(List commitMessages) { + List committed = new ArrayList<>(); try { List committers = new ArrayList<>(); for (CommitMessage commitMessage : commitMessages) { @@ -154,6 +155,7 @@ public void commit(List commitMessages) { for (TwoPhaseOutputStream.Committer committer : committers) { committer.commit(this.fileIO); + committed.add(committer); if (partitionKeys != null && !partitionKeys.isEmpty() && (hiveCatalog != null || partitionManager != null)) { @@ -190,6 +192,9 @@ public void commit(List commitMessages) { } } catch (Exception e) { + for (TwoPhaseOutputStream.Committer committer : committed) { + fileIO.deleteQuietly(committer.targetPath()); + } this.abort(commitMessages); throw new RuntimeException(e); } diff --git a/paimon-core/src/test/java/org/apache/paimon/TestFileStore.java b/paimon-core/src/test/java/org/apache/paimon/TestFileStore.java index b670ffa48f0b..b9575c1af7ed 100644 --- a/paimon-core/src/test/java/org/apache/paimon/TestFileStore.java +++ b/paimon-core/src/test/java/org/apache/paimon/TestFileStore.java @@ -110,6 +110,7 @@ public class TestFileStore extends KeyValueFileStore { private TestFileStore( String root, + FileIO fileIO, CoreOptions options, RowType partitionType, RowType keyType, @@ -118,8 +119,8 @@ private TestFileStore( MergeFunctionFactory mfFactory, TableSchema tableSchema) { super( - FileIOFinder.find(new Path(root)), - schemaManager(root, options), + fileIO, + schemaManager(fileIO, options), tableSchema != null ? tableSchema : new TableSchema( @@ -140,7 +141,7 @@ private TestFileStore( (new Path(root)).getName(), CatalogEnvironment.empty()); this.root = root; - this.fileIO = FileIOFinder.find(new Path(root)); + this.fileIO = fileIO; this.keySerializer = new InternalRowSerializer(keyType); this.valueSerializer = new InternalRowSerializer(valueType); this.commitUser = UUID.randomUUID().toString(); @@ -154,8 +155,8 @@ private static List cleanPrimaryKeys(List primaryKeys) { .collect(Collectors.toList()); } - private static SchemaManager schemaManager(String root, CoreOptions options) { - return new SchemaManager(FileIOFinder.find(new Path(root)), options.path()); + private static SchemaManager schemaManager(FileIO fileIO, CoreOptions options) { + return new SchemaManager(fileIO, options.path()); } public FileIO fileIO() { @@ -777,6 +778,7 @@ public static class Builder { private final TableSchema tableSchema; private CoreOptions.ChangelogProducer changelogProducer; + private FileIO fileIO; public Builder( String format, @@ -806,6 +808,11 @@ public Builder changelogProducer(CoreOptions.ChangelogProducer changelogProducer return this; } + public Builder fileIO(FileIO fileIO) { + this.fileIO = fileIO; + return this; + } + public TestFileStore build() { Options conf = tableSchema == null ? new Options() : Options.fromMap(tableSchema.options()); @@ -827,8 +834,10 @@ public TestFileStore build() { // disable dynamic-partition-overwrite in FileStoreCommit layer test conf.set(CoreOptions.DYNAMIC_PARTITION_OVERWRITE, false); + FileIO effectiveFileIO = fileIO == null ? FileIOFinder.find(new Path(root)) : fileIO; return new TestFileStore( root, + effectiveFileIO, new CoreOptions(conf), partitionType, keyType, diff --git a/paimon-core/src/test/java/org/apache/paimon/catalog/FileSystemCatalogTest.java b/paimon-core/src/test/java/org/apache/paimon/catalog/FileSystemCatalogTest.java index 5e186cdf10df..e87ecd10917d 100644 --- a/paimon-core/src/test/java/org/apache/paimon/catalog/FileSystemCatalogTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/catalog/FileSystemCatalogTest.java @@ -20,7 +20,10 @@ import org.apache.paimon.CoreOptions; import org.apache.paimon.TableType; +import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.StrictContractFileIO; +import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.options.Options; import org.apache.paimon.schema.Schema; import org.apache.paimon.schema.SchemaManager; @@ -69,6 +72,34 @@ public void testCreateTableCaseSensitive() throws Exception { catalog.createTable(identifier, schema, false); } + @Test + public void testCoreFileIOUsageStaysWithinPortableContract() throws Exception { + FileIO strictFileIO = new StrictContractFileIO(new LocalFileIO()); + Path strictWarehouse = new Path(tempFile.resolve("strict-contract").toUri()); + FileSystemCatalog strictCatalog = + new FileSystemCatalog( + strictFileIO, strictWarehouse, CatalogContext.create(new Options())); + Identifier source = Identifier.create("contract_db", "source_table"); + Identifier destination = Identifier.create("contract_db", "destination_table"); + Schema schema = Schema.newBuilder().column("id", DataTypes.INT()).build(); + + try { + strictCatalog.createDatabase(source.getDatabaseName(), false); + strictCatalog.createTable(source, schema, false); + strictCatalog.renameTable(source, destination, false); + + assertThat(strictCatalog.tableExists(source)).isFalse(); + assertThat(strictCatalog.tableExists(destination)).isTrue(); + + strictCatalog.dropTable(destination, false); + assertThat(strictCatalog.tableExists(destination)).isFalse(); + strictCatalog.dropDatabase(source.getDatabaseName(), false, false); + assertThat(strictCatalog.listDatabases()).doesNotContain(source.getDatabaseName()); + } finally { + strictCatalog.close(); + } + } + @Test public void testValidateFormatTableDefaultOptions() throws Exception { String database = "format_table_default_validation_db"; diff --git a/paimon-core/src/test/java/org/apache/paimon/operation/FileStoreCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/operation/FileStoreCommitTest.java index f326be18dcab..250c7837ada4 100644 --- a/paimon-core/src/test/java/org/apache/paimon/operation/FileStoreCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/operation/FileStoreCommitTest.java @@ -29,7 +29,9 @@ import org.apache.paimon.data.BinaryRow; import org.apache.paimon.deletionvectors.BucketedDvMaintainer; import org.apache.paimon.deletionvectors.DeletionVector; +import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.StrictContractFileIO; import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.index.GlobalIndexMeta; import org.apache.paimon.index.IndexFileHandler; @@ -113,6 +115,7 @@ import static org.apache.paimon.utils.HintFileUtils.LATEST; import static org.apache.paimon.utils.Preconditions.checkNotNull; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; import static org.assertj.core.api.Assertions.assertThatThrownBy; /** Tests for {@link FileStoreCommitImpl}. */ @@ -139,6 +142,69 @@ public void afterEach() { assertThat(FailingFileIO.openOutputStreams(pathPredicate)).isEmpty(); } + @Test + public void testSnapshotCommitWithStrictFileIO() throws Exception { + FileIO strictFileIO = new StrictContractFileIO(new TraceableFileIO()); + TestFileStore store = + createStore( + false, + 1, + CoreOptions.ChangelogProducer.NONE, + Collections.emptyMap(), + strictFileIO); + KeyValue record = gen.next(); + + Snapshot snapshot = + store.commitData(Collections.singletonList(record), gen::getPartition, kv -> 0) + .get(0); + + assertThat(store.fileIO()).isSameAs(strictFileIO); + assertThat(store.snapshotManager().latestSnapshot()).isEqualTo(snapshot); + assertThat(store.toKvMap(store.readKvsFromSnapshot(snapshot.id()))) + .isEqualTo(store.toKvMap(Collections.singletonList(record))); + } + + @Test + public void testAbortCleanupWithStrictFileIO() throws Exception { + FileIO strictFileIO = new StrictContractFileIO(new TraceableFileIO()); + TestFileStore store = + createStore( + false, + 1, + CoreOptions.ChangelogProducer.NONE, + Collections.emptyMap(), + strictFileIO); + AtomicReference abandonedFile = new AtomicReference<>(); + + List snapshots = + store.commitDataImpl( + Collections.singletonList(gen.next()), + gen::getPartition, + kv -> 0, + false, + null, + null, + Collections.emptyList(), + (commit, committable) -> { + CommitMessageImpl message = + (CommitMessageImpl) committable.fileCommittables().get(0); + DataFileMeta dataFile = message.newFilesIncrement().newFiles().get(0); + Path path = + store.pathFactory() + .createDataFilePathFactory( + message.partition(), message.bucket()) + .toPath(dataFile); + abandonedFile.set(path); + assertThatCode(() -> store.fileIO().getFileStatus(path)) + .doesNotThrowAnyException(); + commit.abort(committable.fileCommittables()); + }); + + assertThat(snapshots).isEmpty(); + assertThat(store.snapshotManager().latestSnapshotId()).isNull(); + assertThat(store.fileIO().exists(checkNotNull(abandonedFile.get()))).isFalse(); + } + @ParameterizedTest @CsvSource({ "false,NONE", @@ -2301,11 +2367,21 @@ private TestFileStore createStore( CoreOptions.ChangelogProducer changelogProducer, Map options) throws Exception { + return createStore(failing, numBucket, changelogProducer, options, null); + } + + private TestFileStore createStore( + boolean failing, + int numBucket, + CoreOptions.ChangelogProducer changelogProducer, + Map options, + @Nullable FileIO fileIO) + throws Exception { String root = failing ? FailingFileIO.getFailingPath(failingName, tempDir.toString()) : TraceableFileIO.SCHEME + "://" + tempDir.toString(); - Path path = new Path(tempDir.toUri()); + Path path = fileIO == null ? new Path(tempDir.toUri()) : new Path(root); List primaryKeys = Boolean.parseBoolean(options.get(CoreOptions.ROW_TRACKING_ENABLED.key())) ? Collections.emptyList() @@ -2313,25 +2389,29 @@ private TestFileStore createStore( TestKeyValueGenerator.GeneratorMode.MULTI_PARTITIONED); TableSchema tableSchema = SchemaUtils.forceCommit( - new SchemaManager(new LocalFileIO(), path), + new SchemaManager(fileIO == null ? new LocalFileIO() : fileIO, path), new Schema( TestKeyValueGenerator.DEFAULT_ROW_TYPE.getFields(), TestKeyValueGenerator.DEFAULT_PART_TYPE.getFieldNames(), primaryKeys, options, null)); - return new TestFileStore.Builder( - "avro", - root, - numBucket, - TestKeyValueGenerator.DEFAULT_PART_TYPE, - TestKeyValueGenerator.KEY_TYPE, - TestKeyValueGenerator.DEFAULT_ROW_TYPE, - TestKeyValueGenerator.TestKeyValueFieldsExtractor.EXTRACTOR, - DeduplicateMergeFunction.factory(), - tableSchema) - .changelogProducer(changelogProducer) - .build(); + TestFileStore.Builder builder = + new TestFileStore.Builder( + "avro", + root, + numBucket, + TestKeyValueGenerator.DEFAULT_PART_TYPE, + TestKeyValueGenerator.KEY_TYPE, + TestKeyValueGenerator.DEFAULT_ROW_TYPE, + TestKeyValueGenerator.TestKeyValueFieldsExtractor.EXTRACTOR, + DeduplicateMergeFunction.factory(), + tableSchema) + .changelogProducer(changelogProducer); + if (fileIO != null) { + builder.fileIO(fileIO); + } + return builder.build(); } private List generateDataList(int numRecords) { diff --git a/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java b/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java index 109303a9288d..174e8ad2bcbc 100644 --- a/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java @@ -21,6 +21,7 @@ import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.FileIOFinder; import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.StrictContractFileIO; import org.apache.paimon.schema.Schema; import org.apache.paimon.schema.SchemaManager; import org.apache.paimon.types.DataTypes; @@ -49,7 +50,7 @@ class FileSystemBranchManagerTest { @BeforeEach void before() throws Exception { tablePath = new Path(tempDir.toUri().toString()); - fileIO = FileIOFinder.find(tablePath); + fileIO = new StrictContractFileIO(FileIOFinder.find(tablePath)); // Create schema Schema schema = @@ -70,6 +71,7 @@ void before() throws Exception { branchManager = new FileSystemBranchManager( fileIO, tablePath, snapshotManager, tagManager, schemaManager, null); + assertThat(fileIO).isInstanceOf(StrictContractFileIO.class); } @Test @@ -146,6 +148,8 @@ void testRenameBranchPreservesData() { // Verify the renamed branch exists and the original does not assertThat(branchManager.branchExists("test_branch")).isFalse(); assertThat(branchManager.branchExists("renamed_branch")).isTrue(); + assertThat(schemaManager.copyWithBranch("renamed_branch").latest()) + .isEqualTo(schemaManager.latest()); } @Test diff --git a/paimon-core/src/test/java/org/apache/paimon/utils/TagManagerTest.java b/paimon-core/src/test/java/org/apache/paimon/utils/TagManagerTest.java index eaf11bc4e756..3146e707e510 100644 --- a/paimon-core/src/test/java/org/apache/paimon/utils/TagManagerTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/utils/TagManagerTest.java @@ -26,6 +26,7 @@ import org.apache.paimon.data.BinaryRow; import org.apache.paimon.fs.FileIO; import org.apache.paimon.fs.Path; +import org.apache.paimon.fs.StrictContractFileIO; import org.apache.paimon.fs.local.LocalFileIO; import org.apache.paimon.mergetree.compact.DeduplicateMergeFunction; import org.apache.paimon.operation.FileStoreTestUtils; @@ -172,6 +173,58 @@ public void testRenameTagWithExistingTargetName() throws Exception { Assertions.assertTrue(exception.getMessage().contains("Tag 'tag2' already exists.")); } + @Test + public void testRenameTagToExactMissingDestination() throws Exception { + LocalFileIO delegate = new LocalFileIO(); + Path tablePath = new Path(tempDir.toUri().toString()); + FileIO strictFileIO = new StrictContractFileIO(delegate); + TagManager strictTagManager = new TagManager(strictFileIO, tablePath); + SnapshotManager snapshotManager = + new SnapshotManager(strictFileIO, tablePath, null, null, null); + Snapshot snapshot = strictContractSnapshot(); + Path source = strictTagManager.tagPath("source"); + Path destination = strictTagManager.tagPath("target"); + delegate.overwriteFileUtf8(snapshotManager.snapshotPath(snapshot.id()), snapshot.toJson()); + strictTagManager.createTag(snapshot, "source", null, Collections.emptyList(), false); + assertThat(delegate.exists(destination)).isFalse(); + + strictTagManager.renameTag("source", "target"); + + assertThat(delegate.exists(source)).isFalse(); + assertThat(delegate.exists(destination)).isTrue(); + assertThat(strictTagManager.getOrThrow("target").id()).isEqualTo(snapshot.id()); + assertThat(strictTagManager.tagNames(name -> true)).containsExactly("target"); + + strictTagManager.deleteTag("target", null, snapshotManager, Collections.emptyList()); + assertThat(strictTagManager.tagExists("target")).isFalse(); + assertThat(strictTagManager.tagNames(name -> true)).isEmpty(); + } + + private static Snapshot strictContractSnapshot() { + return new Snapshot( + 1, + 0, + "base-manifest-list", + null, + "delta-manifest-list", + null, + null, + null, + null, + "strict-contract", + 1, + Snapshot.CommitKind.APPEND, + System.currentTimeMillis(), + 0, + 0, + null, + null, + null, + null, + null, + null); + } + private TestFileStore createStore(TestKeyValueGenerator.GeneratorMode mode, int buckets) throws Exception { ThreadLocalRandom random = ThreadLocalRandom.current(); From 482f1b1f287be8ac2beb28ec98b2f2c8ca1a6c8d Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 18:28:04 +0800 Subject: [PATCH 05/11] [common] Harden FileIO contract integration --- .../java/org/apache/paimon/fs/FileIOTest.java | 43 ++++++++++++------ .../fs/RenamingTwoPhaseOutputStreamTest.java | 17 +++++++ .../table/format/FormatTableCommit.java | 8 ++-- .../table/format/FormatTableCommitTest.java | 15 ++++++- .../utils/FileSystemBranchManagerTest.java | 26 +++++++---- .../java/org/apache/paimon/s3/S3FileIO.java | 3 +- .../apache/paimon/s3/S3MultiPartUpload.java | 8 +++- .../paimon/s3/S3MultiPartUploadCommitter.java | 2 +- .../fs/S3RemoteFileChangedExceptionTest.java | 44 +++++++++++++++++++ .../paimon/s3/S3MultiPartUploadTest.java | 2 +- 10 files changed, 135 insertions(+), 33 deletions(-) create mode 100644 paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/fs/S3RemoteFileChangedExceptionTest.java diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java index db77750d4272..ada55a09c153 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOTest.java @@ -96,13 +96,13 @@ public void testGetSchemelessPathUsesLocalFileIO() throws IOException { public void testGetUsesResolvingFileIOWhenEnabled() throws IOException { Options options = new Options(); options.set(CatalogOptions.RESOLVING_FILE_IO_ENABLED, true); + Path path = new Path(tempDir.resolve("resolving").toUri()); - FileIO fileIO = - FileIO.get( - new Path(tempDir.resolve("resolving").toUri()), - CatalogContext.create(options)); + FileIO fileIO = FileIO.get(path, CatalogContext.create(options)); assertThat(fileIO).isInstanceOf(ResolvingFileIO.class); + fileIO.writeFile(path, "configured", false); + assertThat(fileIO.readFileUtf8(path)).isEqualTo("configured"); } @Test @@ -145,15 +145,18 @@ public void testDiscoverLoadersRejectsDuplicateSchemes() { } @Test - public void testFailedPreferredLoaderFallsBackToAccessibleLoader() throws IOException { + public void testPreferredLoaderWithMissingOptionsFallsBackToAccessibleLoader() + throws IOException { TrackingLoader preferred = new TrackingLoader("preferred", "required-by-preferred"); TrackingLoader fallback = new TrackingLoader("fallback"); Path path = new Path("unregistered:///warehouse"); - FileIO selected = - FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); + TrackingLocalFileIO selected = + (TrackingLocalFileIO) + FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); - assertThat(selected).isSameAs(fallback.fileIO); + assertThat(selected.owner).isEqualTo("fallback"); + assertThat(selected.configured).isTrue(); } @Test @@ -162,10 +165,12 @@ public void testAccessiblePreferredLoaderIsSelectedBeforeFallback() throws IOExc TrackingLoader fallback = new TrackingLoader("fallback"); Path path = new Path("unregistered:///warehouse"); - FileIO selected = - FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); + TrackingLocalFileIO selected = + (TrackingLocalFileIO) + FileIO.get(path, CatalogContext.create(new Options(), preferred, fallback)); - assertThat(selected).isSameAs(preferred.fileIO); + assertThat(selected.owner).isEqualTo("preferred"); + assertThat(selected.configured).isTrue(); } @Test @@ -308,7 +313,6 @@ private static class TrackingLoader implements FileIOLoader { private static final long serialVersionUID = 1L; private final String scheme; - private final TrackingLocalFileIO fileIO; private final String requiredOption; private TrackingLoader(String scheme) { @@ -317,7 +321,6 @@ private TrackingLoader(String scheme) { private TrackingLoader(String scheme, String requiredOption) { this.scheme = scheme; - this.fileIO = new TrackingLocalFileIO(); this.requiredOption = requiredOption; } @@ -335,7 +338,7 @@ public List requiredOptions() { @Override public FileIO load(Path path) { - return fileIO; + return new TrackingLocalFileIO(scheme); } } @@ -343,6 +346,18 @@ private static class TrackingLocalFileIO extends LocalFileIO { private static final long serialVersionUID = 1L; + private final String owner; + private boolean configured; + + private TrackingLocalFileIO(String owner) { + this.owner = owner; + } + + @Override + public void configure(CatalogContext context) { + configured = true; + } + @Override public boolean exists(Path path) { return true; diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java index 1a66abf99cce..14f85b6ecd1b 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/RenamingTwoPhaseOutputStreamTest.java @@ -169,6 +169,23 @@ void testDiscardRemovesOnlyItsStagedFile() throws IOException { assertThat(fileIO.readFileUtf8(targetPath)).isEqualTo("published"); } + @Test + void testOverwriteDoesNotDeleteTargetWhenStagedFileIsMissing() throws IOException { + fileIO.writeFile(targetPath, "old", false); + RenamingTwoPhaseOutputStream stream = + new RenamingTwoPhaseOutputStream(fileIO, targetPath, true); + stream.write("new".getBytes()); + TwoPhaseOutputStream.Committer committer = stream.closeForCommit(); + + Path stagingDir = new Path(targetPath.getParent(), "_temporary"); + FileStatus[] stagedFiles = fileIO.listStatus(stagingDir); + assertThat(stagedFiles).hasSize(1); + fileIO.delete(stagedFiles[0].getPath(), false); + + assertThatThrownBy(() -> committer.commit(fileIO)).isInstanceOf(IOException.class); + assertThat(fileIO.readFileUtf8(targetPath)).isEqualTo("old"); + } + @Test void testCloseWithoutCommit() throws IOException { RenamingTwoPhaseOutputStream stream = diff --git a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java index 1b7548969c14..ec2039c812c9 100644 --- a/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java +++ b/paimon-core/src/main/java/org/apache/paimon/table/format/FormatTableCommit.java @@ -106,7 +106,9 @@ public FormatTableCommit( @Override public void commit(List commitMessages) { - List committed = new ArrayList<>(); + // Format writers own UUID-based target paths. A remote commit may publish before throwing, + // so every attempted target belongs to this failed batch and must be rolled back. + List attempted = new ArrayList<>(); try { List committers = new ArrayList<>(); for (CommitMessage commitMessage : commitMessages) { @@ -154,8 +156,8 @@ public void commit(List commitMessages) { } for (TwoPhaseOutputStream.Committer committer : committers) { + attempted.add(committer); committer.commit(this.fileIO); - committed.add(committer); if (partitionKeys != null && !partitionKeys.isEmpty() && (hiveCatalog != null || partitionManager != null)) { @@ -192,7 +194,7 @@ public void commit(List commitMessages) { } } catch (Exception e) { - for (TwoPhaseOutputStream.Committer committer : committed) { + for (TwoPhaseOutputStream.Committer committer : attempted) { fileIO.deleteQuietly(committer.targetPath()); } this.abort(commitMessages); diff --git a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java index c7a557b4498a..4727f45b60c1 100644 --- a/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/table/format/FormatTableCommitTest.java @@ -43,10 +43,12 @@ import static org.assertj.core.api.Assertions.entry; import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; /** Tests for {@link FormatTableCommit}. */ class FormatTableCommitTest { @@ -93,11 +95,19 @@ void testPartitionRegistrationFailureDiscardsTheFilesItWrote() throws Exception } @Test - void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { + void testFileCommitFailureDiscardsPublishedTarget() throws Exception { LocalFileIO fileIO = LocalFileIO.create(); Path tablePath = new Path(tempDir.toUri()); + Path targetPath = new Path(tablePath, "year=2025/month=10/partial.csv"); TwoPhaseOutputStream.Committer committer = mock(TwoPhaseOutputStream.Committer.class); - doThrow(new IOException("data commit failed")).when(committer).commit(fileIO); + doAnswer( + ignored -> { + fileIO.writeFile(targetPath, "partial", false); + throw new IOException("data commit failed"); + }) + .when(committer) + .commit(fileIO); + when(committer.targetPath()).thenReturn(targetPath); FormatTablePartitionManager partitionManager = mock(FormatTablePartitionManager.class); FormatTableCommit commit = new FormatTableCommit( @@ -118,6 +128,7 @@ void testFileCommitFailureStillDiscardsUncommittedFiles() throws Exception { .isInstanceOf(RuntimeException.class) .hasRootCauseMessage("data commit failed"); + assertThat(fileIO.exists(targetPath)).isFalse(); verify(committer).discard(fileIO); verify(partitionManager, never()).createPartitions(anyList(), eq(true)); } diff --git a/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java b/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java index 174e8ad2bcbc..f2c3a3157457 100644 --- a/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java +++ b/paimon-core/src/test/java/org/apache/paimon/utils/FileSystemBranchManagerTest.java @@ -50,7 +50,7 @@ class FileSystemBranchManagerTest { @BeforeEach void before() throws Exception { tablePath = new Path(tempDir.toUri().toString()); - fileIO = new StrictContractFileIO(FileIOFinder.find(tablePath)); + fileIO = FileIOFinder.find(tablePath); // Create schema Schema schema = @@ -71,7 +71,6 @@ void before() throws Exception { branchManager = new FileSystemBranchManager( fileIO, tablePath, snapshotManager, tagManager, schemaManager, null); - assertThat(fileIO).isInstanceOf(StrictContractFileIO.class); } @Test @@ -138,17 +137,28 @@ void testRenameBranchFromTag() { @Test void testRenameBranchPreservesData() { + FileIO strictFileIO = new StrictContractFileIO(fileIO); + SchemaManager strictSchemaManager = new SchemaManager(strictFileIO, tablePath); + FileSystemBranchManager strictBranchManager = + new FileSystemBranchManager( + strictFileIO, + tablePath, + new SnapshotManager(strictFileIO, tablePath, null, null, null), + new TagManager(strictFileIO, tablePath), + strictSchemaManager, + null); + // Create a branch - branchManager.createBranch("test_branch"); - assertThat(branchManager.branchExists("test_branch")).isTrue(); + strictBranchManager.createBranch("test_branch"); + assertThat(strictBranchManager.branchExists("test_branch")).isTrue(); // Rename the branch - branchManager.renameBranch("test_branch", "renamed_branch"); + strictBranchManager.renameBranch("test_branch", "renamed_branch"); // Verify the renamed branch exists and the original does not - assertThat(branchManager.branchExists("test_branch")).isFalse(); - assertThat(branchManager.branchExists("renamed_branch")).isTrue(); - assertThat(schemaManager.copyWithBranch("renamed_branch").latest()) + assertThat(strictBranchManager.branchExists("test_branch")).isFalse(); + assertThat(strictBranchManager.branchExists("renamed_branch")).isTrue(); + assertThat(strictSchemaManager.copyWithBranch("renamed_branch").latest()) .isEqualTo(schemaManager.latest()); } diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java index 827251837342..008d8893066a 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java @@ -81,8 +81,7 @@ public TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrite if (!overwrite && this.exists(path)) { throw new IOException("File " + path + " already exists."); } - return new S3TwoPhaseOutputStream( - new S3MultiPartUpload(fs, fs.getConf()), hadoopPath, path); + return new S3TwoPhaseOutputStream(new S3MultiPartUpload(fs), hadoopPath, path); } // add additional config entries from the IO config to the Hadoop config diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java index ead6015fedba..65b5b393040c 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java @@ -47,13 +47,17 @@ public class S3MultiPartUpload private final S3AFileSystem s3a; private final WriteOperationHelper s3accessHelper; - public S3MultiPartUpload(S3AFileSystem s3a, Configuration conf) { + public S3MultiPartUpload(S3AFileSystem s3a) { checkNotNull(s3a); - checkNotNull(conf); this.s3accessHelper = s3a.createWriteOperationHelper(s3a.getActiveAuditSpan()); this.s3a = s3a; } + public S3MultiPartUpload(S3AFileSystem s3a, Configuration conf) { + this(s3a); + checkNotNull(conf); + } + @Override public Path workingDirectory() { return s3a.getWorkingDirectory(); diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java index 6551f5e8a9b8..9db3c9fc65fe 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java @@ -49,6 +49,6 @@ public S3MultiPartUploadCommitter( S3FileIO s3FileIO = (S3FileIO) fileIO; org.apache.hadoop.fs.Path hadoopPath = s3FileIO.path(targetPath); S3AFileSystem fs = (S3AFileSystem) s3FileIO.getFileSystem(hadoopPath); - return new S3MultiPartUpload(fs, fs.getConf()); + return new S3MultiPartUpload(fs); } } diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/fs/S3RemoteFileChangedExceptionTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/fs/S3RemoteFileChangedExceptionTest.java new file mode 100644 index 000000000000..e54efe1cf351 --- /dev/null +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/fs/S3RemoteFileChangedExceptionTest.java @@ -0,0 +1,44 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.fs; + +import org.apache.hadoop.fs.s3a.RemoteFileChangedException; +import org.junit.jupiter.api.Test; + +import static org.apache.paimon.fs.RecordingFileIO.Method.EXISTS; +import static org.apache.paimon.fs.RecordingFileIO.Method.NEW_INPUT_STREAM; +import static org.assertj.core.api.Assertions.assertThat; + +/** Tests S3-specific retry behavior of {@link FileIO}. */ +class S3RemoteFileChangedExceptionTest { + + @Test + void overwrittenReadRetriesRemoteFileChangedException() throws Exception { + RecordingFileIO fileIO = new RecordingFileIO(); + Path path = new Path("s3://bucket/overwritten"); + fileIO.putFile(path, "stable"); + fileIO.failNext( + NEW_INPUT_STREAM, + new RemoteFileChangedException(path.toString(), "read", "object changed")); + + assertThat(fileIO.readOverwrittenFileUtf8(path)).contains("stable"); + assertThat(fileIO.callCount(NEW_INPUT_STREAM)).isEqualTo(2); + assertThat(fileIO.callCount(EXISTS)).isLessThanOrEqualTo(1); + } +} diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java index 03702ff29c13..4a4d40cef6a9 100644 --- a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java @@ -77,7 +77,7 @@ void testUploadPartRequestKeepsPartCoordinates() throws Exception { private static UploadPartRequest newUploadPartRequest(Configuration conf) throws IOException { try (S3AFileSystem fs = new S3AFileSystem()) { fs.initialize(URI.create("s3a://" + BUCKET + "/"), conf); - S3MultiPartUpload upload = new S3MultiPartUpload(fs, conf); + S3MultiPartUpload upload = new S3MultiPartUpload(fs); return upload.newUploadPartRequest(OBJECT_NAME, UPLOAD_ID, 3, 1024); } } From 629ac72d861277263326a94dcfa31ab363c2b321 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 19:25:12 +0800 Subject: [PATCH 06/11] [common][s3] Fix S3 FileIO contract failures --- .../paimon/fs/FileIOContractTestBase.java | 9 +- .../paimon/s3/HadoopCompliantFileIO.java | 7 +- .../apache/paimon/s3/S3AtomicWriteTest.java | 96 +++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java index 3bb178962de5..f724a4f4c71a 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java @@ -286,9 +286,12 @@ void testFileStatusProvidesConsistentModificationTime() throws IOException { FileStatus directStatus = contractFileIO().getFileStatus(file); FileStatus listedStatus = statusFor(contractFileIO().listStatus(contractBasePath()), file); - assertThat(directStatus.getModificationTime()).isGreaterThan(1_000_000_000_000L); - assertThat(listedStatus.getModificationTime()) - .isEqualTo(directStatus.getModificationTime()); + long directModificationTime = directStatus.getModificationTime(); + long listedModificationTime = listedStatus.getModificationTime(); + assertThat(directModificationTime).isGreaterThan(1_000_000_000_000L); + assertThat(listedModificationTime).isGreaterThan(1_000_000_000_000L); + assertThat(Math.abs(listedModificationTime - directModificationTime)) + .isLessThanOrEqualTo(1_000L); } @Test diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/HadoopCompliantFileIO.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/HadoopCompliantFileIO.java index a662e8a07592..db18cd77ae98 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/HadoopCompliantFileIO.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/HadoopCompliantFileIO.java @@ -27,6 +27,7 @@ import org.apache.hadoop.fs.FSDataInputStream; import org.apache.hadoop.fs.FSDataOutputStream; +import org.apache.hadoop.fs.FileAlreadyExistsException; import org.apache.hadoop.fs.FileSystem; import java.io.IOException; @@ -120,7 +121,11 @@ public boolean mkdirs(Path path) throws IOException { public boolean rename(Path src, Path dst) throws IOException { org.apache.hadoop.fs.Path hadoopSrc = path(src); org.apache.hadoop.fs.Path hadoopDst = path(dst); - return getFileSystem(hadoopSrc).rename(hadoopSrc, hadoopDst); + try { + return getFileSystem(hadoopSrc).rename(hadoopSrc, hadoopDst); + } catch (FileAlreadyExistsException e) { + return false; + } } protected org.apache.hadoop.fs.Path path(Path path) { diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java new file mode 100644 index 000000000000..c97d80a1066c --- /dev/null +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java @@ -0,0 +1,96 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.paimon.s3; + +import org.apache.paimon.fs.Path; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.fs.FileAlreadyExistsException; +import org.apache.hadoop.fs.FileSystem; +import org.apache.hadoop.fs.FilterFileSystem; +import org.junit.jupiter.api.Test; + +import java.io.IOException; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +class S3AtomicWriteTest { + + @Test + void testExistingTargetReturnsFalseAndCleansTemporaryFile() throws IOException { + Path target = new Path("s3://bucket/path/file"); + FailingRenameFileIO fileIO = + new FailingRenameFileIO(new FileAlreadyExistsException(target.toString())); + + assertThat(fileIO.tryToWriteAtomic(target, "replacement")).isFalse(); + assertThat(fileIO.renameDestination).isEqualTo(target); + assertThat(fileIO.deletedPath).isEqualTo(fileIO.renameSource); + } + + @Test + void testUnrelatedRenameFailureStillPropagatesAndCleansTemporaryFile() throws IOException { + Path target = new Path("s3://bucket/path/file"); + IOException failure = new IOException("rename failed"); + FailingRenameFileIO fileIO = new FailingRenameFileIO(failure); + + assertThatThrownBy(() -> fileIO.tryToWriteAtomic(target, "replacement")) + .isSameAs(failure); + assertThat(fileIO.renameDestination).isEqualTo(target); + assertThat(fileIO.deletedPath).isEqualTo(fileIO.renameSource); + } + + private static class FailingRenameFileIO extends S3FileIO { + + private final FileSystem fileSystem; + private final IOException renameFailure; + private Path renameSource; + private Path renameDestination; + private Path deletedPath; + + private FailingRenameFileIO(IOException renameFailure) throws IOException { + this.renameFailure = renameFailure; + fileSystem = + new FilterFileSystem(FileSystem.getLocal(new Configuration())) { + @Override + public boolean rename( + org.apache.hadoop.fs.Path src, org.apache.hadoop.fs.Path dst) + throws IOException { + renameSource = new Path(src.toUri()); + renameDestination = new Path(dst.toUri()); + throw FailingRenameFileIO.this.renameFailure; + } + }; + } + + @Override + public void writeFile(Path path, String content, boolean overwrite) {} + + @Override + protected FileSystem createFileSystem(org.apache.hadoop.fs.Path path) { + return fileSystem; + } + + @Override + public boolean delete(Path path, boolean recursive) { + deletedPath = path; + return true; + } + } +} From 8e3afff628c3752ca2ab86cb3ce1043d30e5600b Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Tue, 11 Aug 2026 19:28:58 +0800 Subject: [PATCH 07/11] [s3] Fix atomic write test formatting --- .../src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java index c97d80a1066c..8d4a6819aa49 100644 --- a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3AtomicWriteTest.java @@ -50,8 +50,7 @@ void testUnrelatedRenameFailureStillPropagatesAndCleansTemporaryFile() throws IO IOException failure = new IOException("rename failed"); FailingRenameFileIO fileIO = new FailingRenameFileIO(failure); - assertThatThrownBy(() -> fileIO.tryToWriteAtomic(target, "replacement")) - .isSameAs(failure); + assertThatThrownBy(() -> fileIO.tryToWriteAtomic(target, "replacement")).isSameAs(failure); assertThat(fileIO.renameDestination).isEqualTo(target); assertThat(fileIO.deletedPath).isEqualTo(fileIO.renameSource); } From bd93fd61b06fd472430a65225cf0259c1b36acfb Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Wed, 12 Aug 2026 01:20:55 +0800 Subject: [PATCH 08/11] [common] Document FileIO behavior --- docs/docs/program-api/file-io.md | 128 ++++++++++++++++++ docs/sidebars.js | 1 + .../java/org/apache/paimon/fs/FileIO.java | 4 +- .../paimon/fs/FileIOContractTestBase.java | 2 +- .../java/org/apache/paimon/s3/S3FileIO.java | 3 +- .../apache/paimon/s3/S3MultiPartUpload.java | 8 +- .../paimon/s3/S3MultiPartUploadCommitter.java | 2 +- .../paimon/s3/S3MultiPartUploadTest.java | 2 +- 8 files changed, 138 insertions(+), 12 deletions(-) create mode 100644 docs/docs/program-api/file-io.md diff --git a/docs/docs/program-api/file-io.md b/docs/docs/program-api/file-io.md new file mode 100644 index 000000000000..16ed4c52acdf --- /dev/null +++ b/docs/docs/program-api/file-io.md @@ -0,0 +1,128 @@ +--- +title: "FileIO Behavior" +sidebar_position: 4 +--- + + + +# FileIO Behavior + +`FileIO` abstracts local filesystems, distributed filesystems, and object stores. Every +implementation must provide the behavior below; callers may rely on it when the stated +preconditions hold. The contract does not require POSIX or Hadoop filesystem compatibility. It also +does not require a separate metadata lookup when an operation can return the required result +itself. + +## Required operations + +| API | Required behavior | +| --- | --- | +| `isObjectStore` | Identifies whether the implementation has object-store characteristics. Callers must not use it to infer semantics that are not stated here. | +| `newInputStream` | Returns an independent seekable stream. Reads return the stored bytes, `getPos` tracks the cursor, seeking to a valid offset works, and reads at end of file return `-1` without advancing the cursor. Opening a missing path or a path that names a directory may fail when the stream is created or on its first read. | +| `newOutputStream` | A successful close publishes exactly the bytes written. `overwrite=false` must not replace an existing file; failure may be reported while opening, writing, or closing. `overwrite=true` replaces the old content. Creating a nested file also makes its logical parent directories available. | +| `getFileStatus` | Returns the path, type, byte length, and modification time for an existing path. A missing path throws `FileNotFoundException`. Modification time is expressed as epoch milliseconds, but exact precision is implementation-specific. | +| `exists` | Returns `true` for an existing file or directory and `false` for a missing path. Callers should not invoke it immediately before an operation that already reports the required outcome. | +| `listStatus` | For a known directory, returns a non-null array containing its direct children. Empty directories return an empty array. Result order is not defined. | +| `delete` | Deleting an existing file or empty directory returns `true`. Deleting a non-empty directory with `recursive=false` throws `IOException` without changing the tree; with `recursive=true`, it removes the complete tree. The return value for a missing path is not defined. | +| `mkdirs` | Makes the requested directory hierarchy available and returns `true`, including when the directory already exists. A file at the target or in its parent chain causes an `IOException`. Implementations do not have to materialize object-store directory markers for every parent. | +| `rename` | The defined success case has an existing source, a distinct missing destination, and an existing destination parent in the same `FileIO`. It returns `true`, removes the source name, and preserves the complete file content or directory tree at the exact destination. | + +## Default methods + +The methods below are implemented by `FileIO` itself. Implementations may override them to reduce I/O, +but the observable result must remain the same. + +| API | Required behavior | +| --- | --- | +| `listFiles`, `listFilesIterative` | Return files under a known directory. Non-recursive listing returns direct files; recursive listing includes files below nested directories. Iteration may perform work lazily. | +| `listDirectories` | Returns only the direct child directories of a known directory. | +| `getFileSize`, `isDir` | Return the corresponding field from `getFileStatus`; they do not require an additional existence check. | +| `checkOrMkdirs` | Accepts an existing directory or creates a missing one. An existing file is rejected. | +| `deleteQuietly`, `deleteFilesQuietly`, `deleteDirectoryQuietly` | Attempt the requested deletion and suppress `IOException`. Directory deletion is recursive; file deletion is not. An implementation may avoid probing a missing target. | +| `readFileUtf8` | Decodes content as UTF-8 and closes the input stream. Preservation of original line separators is not part of this contract. | +| `writeFile`, `overwriteFileUtf8`, `overwriteHintFile` | Write UTF-8 content and close the output stream. `writeFile` forwards its overwrite intent; the overwrite helpers replace visible content. | +| `tryToWriteAtomic` | For a missing target, publishes the supplied content and returns `true`. If the target already exists, returns `false`, preserves its content, and cleans up temporary data. Cross-client atomicity requires support from the storage system; a metadata check followed by a write is not sufficient. | +| `copyFile` | Copies the source bytes to the exact destination. `overwrite=false` preserves an existing destination and reports failure; `overwrite=true` replaces it. | +| `copyFiles` | If every direct child of the source directory is a file, copies each child to the destination directory and forwards the overwrite mode. | +| `readOverwrittenFileUtf8` | Returns the current UTF-8 content, returns an empty `Optional` for a missing file, and retries the remote-file-change failures recognized by the implementation. | +| `newTwoPhaseOutputStream` | When supported, staged data becomes visible at the target only after `commit`. `discard` removes only that writer's staged data, and `clean` does not affect another writer. The overwrite flag has the same meaning as for `newOutputStream`, and a committer remains serializable. | + +## Lifecycle, discovery, and optional capabilities + +`configure`, `setRuntimeContext`, and `close` manage an implementation's configuration and +resources. After serialization and deserialization, a `FileIO` must remain usable once any required +runtime options have been supplied again through `setRuntimeContext`. + +`get`, `discoverLoaders`, and `checkAccess` select and configure a loader for a path. Loader +selection prefers an accessible configured loader, then a discovered scheme loader, a configured +fallback, and finally Hadoop. Duplicate loaders for one scheme are rejected. Callers must not +depend on the number or order of metadata requests used during loader selection. + +`archive`, `restoreArchive`, `unarchive`, and `createBlobPresignedUrl` are optional capabilities. +Their default methods throw `UnsupportedOperationException`; implementations must document the behavior +of capabilities they implement. + +## Intentionally unspecified behavior + +Paimon code must not depend on the following behavior unless a narrower API or capability defines +it: + +- listing a missing path, a file, or the filesystem root; +- listing order or a snapshot-consistent listing during concurrent mutation; +- `rename` atomicity, a missing source, an existing destination, identical paths, a missing + destination parent, or cross-filesystem rename; +- Hadoop's move-into-directory behavior when the destination is an existing directory; +- `copyFiles` when a direct child of the source is a directory; +- physical object-store directory markers, stable directory modification times, or file and prefix + collision rules; +- exact exception subclasses where the API declares only `IOException`; +- visibility before a stream closes, or the exact point at which a deferred operation reports an + error; +- consistency, durability, or recovery after a network failure with an unknown remote outcome. + +These omissions are intentional. Adding a dependency on one of them requires an explicit FileIO +contract change and tests for every supported implementation; it must not be inferred from one +implementation. + +## Object-store request cost + +The contract constrains results, not an implementation sequence. Implementations may use conditional +writes, known file length, status returned by a listing, or native batch operations to avoid +redundant `HEAD` and `LIST` requests. Contract tests must keep setup and postcondition checks outside +any implementation-specific request-counting window. + +## Related filesystem models + +- [Hadoop's filesystem contract tests](https://hadoop.apache.org/docs/r3.4.0/hadoop-project-dist/hadoop-common/filesystem/testing.html) + provide operation-oriented suites and explicit filesystem differences. Passing them does not prove + distributed consistency, atomicity, idempotency, scalability, or durability. +- [Iceberg FileIO](https://iceberg.apache.org/docs/latest/fileio/) uses a narrower file-level model + and does not require rename for table state changes. Its + [OutputFile](https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/io/OutputFile.html) + separates create from create-or-overwrite intent. +- [POSIX rename](https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html) defines + atomic namespace replacement. POSIX inode, link, permission, and open-file semantics are not part + of the `FileIO` contract. + +`FileIOContractTestBase` covers required operations. `FileIODefaultMethodTest`, `FileIOTest`, and +`FileIOReturnTypeTest` cover default methods, lifecycle and discovery, default optional-capability +failures, and returned types. `FileIOContractCoverageTest` fails if a public `FileIO` method is not +assigned to a test group. These tests define single-operation preconditions and postconditions; +concurrency and fault recovery require separate tests. diff --git a/docs/sidebars.js b/docs/sidebars.js index be7294db7b03..3ead8cd14948 100644 --- a/docs/sidebars.js +++ b/docs/sidebars.js @@ -269,6 +269,7 @@ const sidebars = { "program-api/rest-api", "program-api/flink-api", "program-api/java-api", + "program-api/file-io", "program-api/catalog-api", "program-api/cpp-api", "program-api/rust-api", diff --git a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java index 2b0dcec3f760..e095a4b6c44a 100644 --- a/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java +++ b/paimon-common/src/main/java/org/apache/paimon/fs/FileIO.java @@ -233,8 +233,8 @@ default FileStatus[] listDirectories(Path path) throws IOException { * 'mkdir -p'. Existence of the directory hierarchy is not an error. * * @param path the directory/directories to be created - * @return true if at least one new directory has been created, false - * otherwise + * @return true if the directory hierarchy exists after this call, including when + * it already existed, false otherwise * @throws IOException thrown if an I/O error occurs while creating the directory */ boolean mkdirs(Path path) throws IOException; diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java index f724a4f4c71a..9ffff36d3ba3 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java @@ -35,7 +35,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; -/** Opt-in provider-neutral contract tests for {@link FileIO}. */ +/** Contract tests shared by {@link FileIO} implementations. */ public abstract class FileIOContractTestBase extends FileIOBehaviorTestBase { private static final byte[] DEFAULT_CONTENT = new byte[] {1, 2, 3, 4, 5, 6, 7, 8}; diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java index 008d8893066a..827251837342 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3FileIO.java @@ -81,7 +81,8 @@ public TwoPhaseOutputStream newTwoPhaseOutputStream(Path path, boolean overwrite if (!overwrite && this.exists(path)) { throw new IOException("File " + path + " already exists."); } - return new S3TwoPhaseOutputStream(new S3MultiPartUpload(fs), hadoopPath, path); + return new S3TwoPhaseOutputStream( + new S3MultiPartUpload(fs, fs.getConf()), hadoopPath, path); } // add additional config entries from the IO config to the Hadoop config diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java index 65b5b393040c..ead6015fedba 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUpload.java @@ -47,17 +47,13 @@ public class S3MultiPartUpload private final S3AFileSystem s3a; private final WriteOperationHelper s3accessHelper; - public S3MultiPartUpload(S3AFileSystem s3a) { + public S3MultiPartUpload(S3AFileSystem s3a, Configuration conf) { checkNotNull(s3a); + checkNotNull(conf); this.s3accessHelper = s3a.createWriteOperationHelper(s3a.getActiveAuditSpan()); this.s3a = s3a; } - public S3MultiPartUpload(S3AFileSystem s3a, Configuration conf) { - this(s3a); - checkNotNull(conf); - } - @Override public Path workingDirectory() { return s3a.getWorkingDirectory(); diff --git a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java index 9db3c9fc65fe..6551f5e8a9b8 100644 --- a/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java +++ b/paimon-filesystems/paimon-s3-impl/src/main/java/org/apache/paimon/s3/S3MultiPartUploadCommitter.java @@ -49,6 +49,6 @@ public S3MultiPartUploadCommitter( S3FileIO s3FileIO = (S3FileIO) fileIO; org.apache.hadoop.fs.Path hadoopPath = s3FileIO.path(targetPath); S3AFileSystem fs = (S3AFileSystem) s3FileIO.getFileSystem(hadoopPath); - return new S3MultiPartUpload(fs); + return new S3MultiPartUpload(fs, fs.getConf()); } } diff --git a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java index 4a4d40cef6a9..03702ff29c13 100644 --- a/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java +++ b/paimon-filesystems/paimon-s3-impl/src/test/java/org/apache/paimon/s3/S3MultiPartUploadTest.java @@ -77,7 +77,7 @@ void testUploadPartRequestKeepsPartCoordinates() throws Exception { private static UploadPartRequest newUploadPartRequest(Configuration conf) throws IOException { try (S3AFileSystem fs = new S3AFileSystem()) { fs.initialize(URI.create("s3a://" + BUCKET + "/"), conf); - S3MultiPartUpload upload = new S3MultiPartUpload(fs); + S3MultiPartUpload upload = new S3MultiPartUpload(fs, conf); return upload.newUploadPartRequest(OBJECT_NAME, UPLOAD_ID, 3, 1024); } } From 7ea57f880fb2b723239e66ad3492d232885355fe Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Wed, 12 Aug 2026 01:50:11 +0800 Subject: [PATCH 09/11] [docs] Refine FileIO API documentation --- docs/docs/program-api/file-io.md | 259 ++++++++++++++++++------------- 1 file changed, 154 insertions(+), 105 deletions(-) diff --git a/docs/docs/program-api/file-io.md b/docs/docs/program-api/file-io.md index 16ed4c52acdf..a1f62168a27a 100644 --- a/docs/docs/program-api/file-io.md +++ b/docs/docs/program-api/file-io.md @@ -1,5 +1,5 @@ --- -title: "FileIO Behavior" +title: "FileIO API" sidebar_position: 4 --- @@ -22,107 +22,156 @@ specific language governing permissions and limitations under the License. --> -# FileIO Behavior - -`FileIO` abstracts local filesystems, distributed filesystems, and object stores. Every -implementation must provide the behavior below; callers may rely on it when the stated -preconditions hold. The contract does not require POSIX or Hadoop filesystem compatibility. It also -does not require a separate metadata lookup when an operation can return the required result -itself. - -## Required operations - -| API | Required behavior | -| --- | --- | -| `isObjectStore` | Identifies whether the implementation has object-store characteristics. Callers must not use it to infer semantics that are not stated here. | -| `newInputStream` | Returns an independent seekable stream. Reads return the stored bytes, `getPos` tracks the cursor, seeking to a valid offset works, and reads at end of file return `-1` without advancing the cursor. Opening a missing path or a path that names a directory may fail when the stream is created or on its first read. | -| `newOutputStream` | A successful close publishes exactly the bytes written. `overwrite=false` must not replace an existing file; failure may be reported while opening, writing, or closing. `overwrite=true` replaces the old content. Creating a nested file also makes its logical parent directories available. | -| `getFileStatus` | Returns the path, type, byte length, and modification time for an existing path. A missing path throws `FileNotFoundException`. Modification time is expressed as epoch milliseconds, but exact precision is implementation-specific. | -| `exists` | Returns `true` for an existing file or directory and `false` for a missing path. Callers should not invoke it immediately before an operation that already reports the required outcome. | -| `listStatus` | For a known directory, returns a non-null array containing its direct children. Empty directories return an empty array. Result order is not defined. | -| `delete` | Deleting an existing file or empty directory returns `true`. Deleting a non-empty directory with `recursive=false` throws `IOException` without changing the tree; with `recursive=true`, it removes the complete tree. The return value for a missing path is not defined. | -| `mkdirs` | Makes the requested directory hierarchy available and returns `true`, including when the directory already exists. A file at the target or in its parent chain causes an `IOException`. Implementations do not have to materialize object-store directory markers for every parent. | -| `rename` | The defined success case has an existing source, a distinct missing destination, and an existing destination parent in the same `FileIO`. It returns `true`, removes the source name, and preserves the complete file content or directory tree at the exact destination. | - -## Default methods - -The methods below are implemented by `FileIO` itself. Implementations may override them to reduce I/O, -but the observable result must remain the same. - -| API | Required behavior | -| --- | --- | -| `listFiles`, `listFilesIterative` | Return files under a known directory. Non-recursive listing returns direct files; recursive listing includes files below nested directories. Iteration may perform work lazily. | -| `listDirectories` | Returns only the direct child directories of a known directory. | -| `getFileSize`, `isDir` | Return the corresponding field from `getFileStatus`; they do not require an additional existence check. | -| `checkOrMkdirs` | Accepts an existing directory or creates a missing one. An existing file is rejected. | -| `deleteQuietly`, `deleteFilesQuietly`, `deleteDirectoryQuietly` | Attempt the requested deletion and suppress `IOException`. Directory deletion is recursive; file deletion is not. An implementation may avoid probing a missing target. | -| `readFileUtf8` | Decodes content as UTF-8 and closes the input stream. Preservation of original line separators is not part of this contract. | -| `writeFile`, `overwriteFileUtf8`, `overwriteHintFile` | Write UTF-8 content and close the output stream. `writeFile` forwards its overwrite intent; the overwrite helpers replace visible content. | -| `tryToWriteAtomic` | For a missing target, publishes the supplied content and returns `true`. If the target already exists, returns `false`, preserves its content, and cleans up temporary data. Cross-client atomicity requires support from the storage system; a metadata check followed by a write is not sufficient. | -| `copyFile` | Copies the source bytes to the exact destination. `overwrite=false` preserves an existing destination and reports failure; `overwrite=true` replaces it. | -| `copyFiles` | If every direct child of the source directory is a file, copies each child to the destination directory and forwards the overwrite mode. | -| `readOverwrittenFileUtf8` | Returns the current UTF-8 content, returns an empty `Optional` for a missing file, and retries the remote-file-change failures recognized by the implementation. | -| `newTwoPhaseOutputStream` | When supported, staged data becomes visible at the target only after `commit`. `discard` removes only that writer's staged data, and `clean` does not affect another writer. The overwrite flag has the same meaning as for `newOutputStream`, and a committer remains serializable. | - -## Lifecycle, discovery, and optional capabilities - -`configure`, `setRuntimeContext`, and `close` manage an implementation's configuration and -resources. After serialization and deserialization, a `FileIO` must remain usable once any required -runtime options have been supplied again through `setRuntimeContext`. - -`get`, `discoverLoaders`, and `checkAccess` select and configure a loader for a path. Loader -selection prefers an accessible configured loader, then a discovered scheme loader, a configured -fallback, and finally Hadoop. Duplicate loaders for one scheme are rejected. Callers must not -depend on the number or order of metadata requests used during loader selection. - -`archive`, `restoreArchive`, `unarchive`, and `createBlobPresignedUrl` are optional capabilities. -Their default methods throw `UnsupportedOperationException`; implementations must document the behavior -of capabilities they implement. - -## Intentionally unspecified behavior - -Paimon code must not depend on the following behavior unless a narrower API or capability defines -it: - -- listing a missing path, a file, or the filesystem root; -- listing order or a snapshot-consistent listing during concurrent mutation; -- `rename` atomicity, a missing source, an existing destination, identical paths, a missing - destination parent, or cross-filesystem rename; -- Hadoop's move-into-directory behavior when the destination is an existing directory; -- `copyFiles` when a direct child of the source is a directory; -- physical object-store directory markers, stable directory modification times, or file and prefix - collision rules; -- exact exception subclasses where the API declares only `IOException`; -- visibility before a stream closes, or the exact point at which a deferred operation reports an - error; -- consistency, durability, or recovery after a network failure with an unknown remote outcome. - -These omissions are intentional. Adding a dependency on one of them requires an explicit FileIO -contract change and tests for every supported implementation; it must not be inferred from one -implementation. - -## Object-store request cost - -The contract constrains results, not an implementation sequence. Implementations may use conditional -writes, known file length, status returned by a listing, or native batch operations to avoid -redundant `HEAD` and `LIST` requests. Contract tests must keep setup and postcondition checks outside -any implementation-specific request-counting window. - -## Related filesystem models - -- [Hadoop's filesystem contract tests](https://hadoop.apache.org/docs/r3.4.0/hadoop-project-dist/hadoop-common/filesystem/testing.html) - provide operation-oriented suites and explicit filesystem differences. Passing them does not prove - distributed consistency, atomicity, idempotency, scalability, or durability. -- [Iceberg FileIO](https://iceberg.apache.org/docs/latest/fileio/) uses a narrower file-level model - and does not require rename for table state changes. Its - [OutputFile](https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/io/OutputFile.html) - separates create from create-or-overwrite intent. -- [POSIX rename](https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html) defines - atomic namespace replacement. POSIX inode, link, permission, and open-file semantics are not part - of the `FileIO` contract. - -`FileIOContractTestBase` covers required operations. `FileIODefaultMethodTest`, `FileIOTest`, and -`FileIOReturnTypeTest` cover default methods, lifecycle and discovery, default optional-capability -failures, and returned types. `FileIOContractCoverageTest` fails if a public `FileIO` method is not -assigned to a test group. These tests define single-operation preconditions and postconditions; -concurrency and fault recovery require separate tests. +# FileIO API + +`FileIO` is Paimon's interface for file I/O on local file systems, distributed file systems, and +object stores. This page is for code that calls `FileIO` directly and for developers implementing a +new `FileIO` implementation. Most users only need +[Filesystems](../maintenance/filesystems), which describes the dependencies and options for the +built-in implementations. + +The behavior below is common to the supported implementations. If a case is not described, callers +should not assume that all storage systems handle it in the same way. + +## Read and Write Files + +`newInputStream(...)` opens a new `SeekableInputStream`. Each stream has its own position, which +starts at `0` and is returned by `getPos()`. The stream supports forward and backward seeks from `0` +through the file length. Reading at the end of the file returns `-1` and does not change the +position. Opening a missing path or a directory can fail either when the stream is opened or on the +first read. + +`newOutputStream(path, overwrite)` opens a `PositionOutputStream`. Its `getPos()` value is the +number of bytes written. After the stream closes successfully, the target contains exactly those +bytes. The `overwrite` argument controls how an existing target is handled: + +- `false` preserves the existing file and reports an `IOException`. The error can occur while + opening the stream, writing data, or closing it. +- `true` replaces the existing content. + +`flush()` writes buffered data to the underlying stream, but only a successful `close()` guarantees +that the data is persistent and visible. Writing a file below a missing directory also makes its +parent paths visible as directories through `exists(...)` and `getFileStatus(...)`. Object store +implementations do not need to create a physical directory marker for every parent. + +## File Status and Listing + +`getFileStatus(...)` returns a `FileStatus` for an existing path. `getPath()` returns the path, +`isDir()` distinguishes a directory from a file, `getLen()` returns a file's length, and +`getModificationTime()` returns the number of milliseconds since the Unix epoch. Modification-time +precision depends on the storage system. `getAccessTime()` and `getOwner()` may be unavailable; their +default values are `0` and `null`. A missing path throws `FileNotFoundException`. + +The listing methods are defined for existing directories: + +- `listStatus(...)` returns the direct files and directories. It returns an empty array for an empty + directory. +- `listFiles(...)` and `listFilesIterative(...)` return files only. With recursive listing enabled, + they also return files in nested directories. The iterator may load entries as it is consumed. +- `listDirectories(...)` returns direct directories only. + +Listing order is not guaranteed. `getFileSize(...)` and `isDir(...)` return the corresponding value +from `getFileStatus(...)`, without requiring a separate `exists(...)` call. + +## File and Directory Operations + +- `exists(...)` returns `true` for an existing file or directory and `false` for a missing path. +- `mkdirs(...)` creates the requested directory and any missing parents. It returns `true` when the + directory already exists. A file at the target path or in its parent path causes an + `IOException`. +- `delete(path, recursive)` returns `true` after deleting an existing file or empty directory. + Deleting a non-empty directory with `recursive=false` throws `IOException` and leaves the + directory unchanged. With `recursive=true`, it deletes the complete directory tree. The return + value for a missing path is implementation-specific. +- `rename(...)` moves a file or directory to the exact destination path. Call it with an existing + source, a different destination that does not exist, and an existing destination parent in the + same underlying file system. On success, it returns `true`, removes the source path, and preserves + the file content or complete directory tree. +- `copyFile(...)` copies the source bytes to the exact destination. An existing destination is + replaced only when `overwrite=true`. When a source directory contains files only, + `copyFiles(...)` applies the same behavior to each direct file. + +`checkOrMkdirs(...)` accepts an existing directory or creates a missing one, but throws +`IllegalArgumentException` for an existing file. `deleteQuietly(...)`, `deleteFilesQuietly(...)`, +and `deleteDirectoryQuietly(...)` suppress `IOException`; directory deletion is recursive, while +file deletion is not. + +## UTF-8 File Helpers + +`writeFile(...)` writes UTF-8 text using the requested overwrite mode. `overwriteFileUtf8(...)` and +`overwriteHintFile(...)` replace the current content. All three methods close the output stream. +Use `overwriteHintFile(...)` only for hint files whose temporary absence during an overwrite is +acceptable. + +`readFileUtf8(...)` reads UTF-8 text and closes the input stream. It reads the file line by line and +does not preserve line separators. `readOverwrittenFileUtf8(...)` returns an empty `Optional` for a +missing file and retries the remote-file-change errors recognized by the implementation. + +`tryToWriteAtomic(...)` returns `true` when it publishes content to a missing target. If the target +already exists, it returns `false`, keeps the existing content, and removes temporary data. Its +default implementation writes to a temporary file and then renames it, while storage implementations +may use native conditional writes. Atomicity between clients therefore depends on the storage +system. + +## Two-Phase Writes + +`newTwoPhaseOutputStream(...)` writes data to a staging path. `closeForCommit()` returns a +serializable committer. The staged data is not visible at the target before `commit(...)` is called; +a successful `commit(...)` publishes it. If the commit throws an exception, the target state is not +guaranteed. The `overwrite` argument has the same meaning as it does for `newOutputStream(...)`. + +`discard(...)` removes only the data staged by that writer. After a successful commit, `clean(...)` +can remove resources that the committer no longer needs, but it must not remove another writer's +data. + +The default implementation stages a temporary file and commits it with `rename(...)`. A storage +system can override the method, for example to use multipart upload. + +## Loading and Configuration + +`FileIO.get(...)` selects and configures an implementation from a path and `CatalogContext`. When +`resolving-file-io.enabled` is enabled, it returns a `ResolvingFileIO`, which selects an underlying +`FileIO` for each path. Otherwise, a path without a URI scheme uses `LocalFileIO`. For a path with a +scheme, selection considers an accessible configured preferred loader, a discovered loader for the +scheme, a configured fallback, and finally Hadoop. + +Applications can obtain the storage for an existing table through `Table.fileIO()`. Code that only +has a path and a `CatalogContext` can call `FileIO.get(...)`. `discoverLoaders()` and +`checkAccess(...)` support this selection and are not normally called directly. + +`configure(...)` receives catalog-level settings, `setRuntimeContext(...)` receives runtime +settings, and `close()` releases resources owned by the implementation. The default implementations +of `setRuntimeContext(...)` and `close()` do nothing. `isObjectStore()` is an +implementation-specific hint; callers should not use it to infer the behavior of other methods. + +`FileIO` implementations are serializable and thread-safe. Runtime-only state must be restored by +`setRuntimeContext(...)` after deserialization when an implementation requires it. + +## Optional Operations + +`archive(...)`, `restoreArchive(...)`, `unarchive(...)`, and `createBlobPresignedUrl(...)` are +optional. Their default implementations throw `UnsupportedOperationException`. + +## Behavior Not Defined by FileIO + +`FileIO` does not define the following behavior. Code intended to work with multiple storage systems +must not depend on: + +- listing a missing path, a file, or the file system root; +- listing order or a consistent listing while another client changes the directory; +- `rename(...)` with a missing source, an existing or identical destination, a missing destination + parent, or paths from different file systems; +- atomic `rename(...)` or Hadoop's behavior of moving an item into an existing destination + directory; +- `copyFiles(...)` when the source directory contains another directory; +- physical directory markers, stable directory modification times, or file and prefix collisions + on object stores; +- an exception subtype more specific than the type declared by the method, or the point at which a + deferred operation reports an error; or +- visibility before an output stream closes, or recovery after a network failure whose result is + unknown. + +The API defines results, not the storage requests used to produce them. Implementations can use +conditional writes, known file lengths, status values returned by listings, and batch operations to +avoid unnecessary metadata requests on object stores. The API does not require a preliminary +`exists(...)` or `getFileStatus(...)` call when an operation can provide the required result itself. From 521f814e50e55fb3dcaa937d85acca43f7eb2721 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Wed, 12 Aug 2026 02:36:07 +0800 Subject: [PATCH 10/11] [docs] Describe current FileIO usage --- docs/docs/program-api/file-io.md | 174 ++++++++++++++++++++++++------- 1 file changed, 137 insertions(+), 37 deletions(-) diff --git a/docs/docs/program-api/file-io.md b/docs/docs/program-api/file-io.md index a1f62168a27a..31664ad7a7ce 100644 --- a/docs/docs/program-api/file-io.md +++ b/docs/docs/program-api/file-io.md @@ -30,8 +30,15 @@ new `FileIO` implementation. Most users only need [Filesystems](../maintenance/filesystems), which describes the dependencies and options for the built-in implementations. -The behavior below is common to the supported implementations. If a case is not described, callers -should not assume that all storage systems handle it in the same way. +The method sections below describe behavior common to the supported implementations. The final +section describes how Paimon currently calls the API. Current usage does not narrow the public +interface: a call remains valid when its method contract allows it, even if Paimon does not make +that call today. + +These contracts describe observable results, not a required sequence of storage requests. An +implementation can use conditional writes, metadata returned by listings, known file lengths, or +batch operations. It does not need a preliminary `exists(...)` or `getFileStatus(...)` call when an +operation can produce the required result directly. ## Read and Write Files @@ -94,7 +101,7 @@ from `getFileStatus(...)`, without requiring a separate `exists(...)` call. `checkOrMkdirs(...)` accepts an existing directory or creates a missing one, but throws `IllegalArgumentException` for an existing file. `deleteQuietly(...)`, `deleteFilesQuietly(...)`, and `deleteDirectoryQuietly(...)` suppress `IOException`; directory deletion is recursive, while -file deletion is not. +file deletion is not. These quiet helpers are best-effort and do not return a success result. ## UTF-8 File Helpers @@ -129,49 +136,142 @@ system can override the method, for example to use multipart upload. ## Loading and Configuration -`FileIO.get(...)` selects and configures an implementation from a path and `CatalogContext`. When -`resolving-file-io.enabled` is enabled, it returns a `ResolvingFileIO`, which selects an underlying -`FileIO` for each path. Otherwise, a path without a URI scheme uses `LocalFileIO`. For a path with a -scheme, selection considers an accessible configured preferred loader, a discovered loader for the -scheme, a configured fallback, and finally Hadoop. +`FileIO.get(...)` selects an implementation from a path and `CatalogContext`. When +`resolving-file-io.enabled` is enabled, it returns a configured `ResolvingFileIO`, which selects an +underlying `FileIO` for each path. Otherwise, a path without a URI scheme returns `LocalFileIO` +directly. Its `configure(...)` method is a no-op. + +For a path with a scheme, `FileIO.get(...)` first checks the configured preferred loader. If that +loader is absent or inaccessible, it looks for a discovered loader with the same scheme. Before +using the preferred or discovered loader, it checks `requiredOptions()`: each returned group lists +aliases for one required option, and at least one alias from every group must occur in the catalog +options, matched case-insensitively. A loader with a missing required option is skipped. Selection +then checks the configured fallback loader and finally Hadoop. The final loader creates a new +`FileIO`, which `FileIO.get(...)` configures before returning it. Applications can obtain the storage for an existing table through `Table.fileIO()`. Code that only has a path and a `CatalogContext` can call `FileIO.get(...)`. `discoverLoaders()` and `checkAccess(...)` support this selection and are not normally called directly. -`configure(...)` receives catalog-level settings, `setRuntimeContext(...)` receives runtime -settings, and `close()` releases resources owned by the implementation. The default implementations -of `setRuntimeContext(...)` and `close()` do nothing. `isObjectStore()` is an -implementation-specific hint; callers should not use it to infer the behavior of other methods. +Call `configure(...)` on factory- or loader-created instances that have not yet received a +`CatalogContext`. A `FileIO` returned by `Table.fileIO()` is already configured and must not be +configured again. Some table-bound implementations, including `RESTTokenFileIO`, reject +reconfiguration. + +`setRuntimeContext(...)` supplies optional job-level file system settings. Paimon's Flink +integration calls it only when `filesystem.job-level-settings.enabled` is enabled. It is not an +automatic callback after deserialization, and its default implementation does nothing. `close()` +releases resources owned by an implementation; its default also does nothing. -`FileIO` implementations are serializable and thread-safe. Runtime-only state must be restored by -`setRuntimeContext(...)` after deserialization when an implementation requires it. +`FileIO` implementations are serializable and thread-safe. `isObjectStore()` is an implementation +hint and does not define the behavior of other methods. ## Optional Operations `archive(...)`, `restoreArchive(...)`, `unarchive(...)`, and `createBlobPresignedUrl(...)` are optional. Their default implementations throw `UnsupportedOperationException`. -## Behavior Not Defined by FileIO - -`FileIO` does not define the following behavior. Code intended to work with multiple storage systems -must not depend on: - -- listing a missing path, a file, or the file system root; -- listing order or a consistent listing while another client changes the directory; -- `rename(...)` with a missing source, an existing or identical destination, a missing destination - parent, or paths from different file systems; -- atomic `rename(...)` or Hadoop's behavior of moving an item into an existing destination - directory; -- `copyFiles(...)` when the source directory contains another directory; -- physical directory markers, stable directory modification times, or file and prefix collisions - on object stores; -- an exception subtype more specific than the type declared by the method, or the point at which a - deferred operation reports an error; or -- visibility before an output stream closes, or recovery after a network failure whose result is - unknown. - -The API defines results, not the storage requests used to produce them. Implementations can use -conditional writes, known file lengths, status values returned by listings, and batch operations to -avoid unnecessary metadata requests on object stores. The API does not require a preliminary -`exists(...)` or `getFileStatus(...)` call when an operation can provide the required result itself. +## Paimon FileIO Usage + +This section records the call shapes and results used by Paimon's production code. It helps new +callers choose the same preconditions and recovery rules. It does not replace the method contracts +above or remove behavior from methods that have no current caller. + +### Status and Listing + +Paimon lists paths expected to be directories. They usually come from configuration, a previous +status or listing result, or an `exists(...)` check. A configured warehouse or object-table +location can itself be the file system root; callers treat that location as an ordinary directory. +Core paths do not intentionally pass a regular file to a listing method. + +Callers do not depend on listing order or on one snapshot-consistent result while another client is +changing the directory. Correctness-sensitive traversal handles a directory that disappears after +its parent was listed by accepting an empty result or catching `FileNotFoundException` and skipping +that subtree; other `IOException` values fail the operation. Best-effort orphan cleanup may instead +treat any listing `IOException` as an empty result and skip that subtree. Some callers that want a +missing directory to mean an empty listing check `exists(...)` first. This listing practice is +separate from the public `getFileStatus(...)` rule: a missing path from `getFileStatus(...)` must +throw `FileNotFoundException`. + +Paimon consumes modification times for both files and directories, including cleanup cutoffs and a +branch directory's reported creation time. The value must follow the `FileStatus` contract, while +its precision and changes during concurrent updates remain file-system-specific. Paimon relies on +logical parent directories being visible, but does not inspect or require physical directory +marker objects. + +### Output and Copying + +Both `newOutputStream(...)` modes are used. Paimon uses `overwrite=false` for UUID- or +version-derived files expected not to exist; a failed create must preserve an existing target. It +uses `overwrite=true` for replaceable state and copy destinations. Writers rely on exact +`getPos()` values and successful `close()` as the publication boundary. Some branch-copy paths also +create output below parents that have not been created explicitly. + +Current `copyFiles(...)` callers copy flat snapshot, schema, and tag metadata directories during +branch fast-forward. They use the same `FileIO` for source and destination and pass +`overwrite=true`. Files are copied one at a time; callers do not assume that a failure rolls back +files copied earlier. This call shape does not remove the public `overwrite=false` behavior from +`copyFile(...)` or `copyFiles(...)`. + +### Directories, Deletion, and Rename + +Paimon uses `mkdirs(...)` with directory-semantic paths, including paths that may already exist. It +relies on parent creation and treats `false` as a creation failure where the result is checked. +Production code does not intentionally pass an existing file or a child of a file. + +Strict `delete(...)` callers normally start with an existing, owned path. They use +`recursive=false` for files or directories expected to be empty and `recursive=true` for complete +owned trees. No caller relies on one particular return value for a missing path: it either ignores +that result or combines a `false` result with `exists(...)`. Quiet deletion is used only where +best-effort cleanup is acceptable. + +Raw `rename(...)` calls use an existing source, a distinct exact destination expected not to exist, +the same `FileIO` and underlying file system, and an existing or pre-created destination parent. A +`true` result confirms the move, and some callers check that result. Branch rename currently ignores +the result and assumes that the selected file system provides atomic rename; it has no coordination +fallback. Existing-destination recovery is handled by the higher-level conditional and two-phase +write protocols, not by changing this raw rename shape. Core workflows do not intentionally use +identical paths or move an item into an existing destination directory; those shapes appear only +through optional virtual file system passthroughs. + +Snapshot publication is the workflow that adds external locking and content checks when atomic +rename is unavailable. Paimon uses `isObjectStore()` when choosing the default catalog-lock setting, +but the value alone does not change the `rename(...)` contract. The blob-descriptor source-table path +also calls `isObjectStore()` before serialization to initialize lazy credentials. That side effect is +implementation-specific and is not part of the `FileIO` contract. + +### Conditional and Two-Phase Publication + +Schemas, snapshots, and Iceberg metadata use `tryToWriteAtomic(...)` with an absent target as the +normal case. `true` means this attempt published its content. `false` means the attempt did not +publish because a target already exists; that target can be concurrent or stale. Callers inspect the +existing content, retry, or use an external lock as required by their metadata protocol. Iceberg +metadata may delete a nonmatching stale target and retry. Callers do not use this method as an +overwrite operation for arbitrary existing state. + +Format-table writers are the current production users of `newTwoPhaseOutputStream(...)`. They pass +`overwrite=false` and use writer-owned UUID target paths. They rely on staged data remaining hidden, +a serializable committer from `closeForCommit()`, publication by `commit(...)`, and writer-scoped +`discard(...)` and `clean(...)` operations. The public API still supports `overwrite=true` even +though this workflow does not use it. + +A remote publication can succeed before reporting an exception, so recovery depends on ownership. +Paimon preserves an ambiguous mutable target when the caller does not own a unique path. The +format-table commit path records every attempted committer and can delete an attempted target after +failure only because each UUID path belongs to that failed batch. This is a format-table recovery +rule, not permission for arbitrary two-phase callers to delete an uncertain target. + +### Lifecycle and Optional Operations + +Most table code receives an already configured `FileIO` from `Table.fileIO()`. Factory-created +instances are either retained by an owner or should be closed by the code that owns their resource +scope. `CachingFileIO.close()` closes its delegate and then releases its shared cache-manager +reference. + +`archive(...)`, `restoreArchive(...)`, and `unarchive(...)` currently have no production caller or +implementation, so their default `UnsupportedOperationException` behavior remains in effect. +`createBlobPresignedUrl(...)` is an active optional operation used by the Blob API and Flink and +Spark SQL functions. Its callers use the `FileIO` and table root from the same loaded table, pass a +table-owned blob descriptor, and do not assume that every `FileIO` supports the operation. Spark +validates a positive whole-second validity before the call. Flink forwards the supplied `Duration`, +so supporting implementations must reject unsupported validity values. From 2923a613f51c9e468e1e38d260c5fb131ce5fe36 Mon Sep 17 00:00:00 2001 From: Sun Dapeng Date: Thu, 13 Aug 2026 00:12:46 +0800 Subject: [PATCH 11/11] [common][docs] Define object-store directory lifetime --- docs/docs/learn-paimon/understand-files.mdx | 4 +- docs/docs/maintenance/manage-partitions.md | 9 +- docs/docs/maintenance/manage-snapshots.mdx | 4 +- docs/docs/program-api/file-io.md | 10 +- .../paimon/fs/FileIOContractTestBase.java | 98 +++++++++++++++++++ 5 files changed, 117 insertions(+), 8 deletions(-) diff --git a/docs/docs/learn-paimon/understand-files.mdx b/docs/docs/learn-paimon/understand-files.mdx index 5d0a81296acc..9bf816b2bbff 100644 --- a/docs/docs/learn-paimon/understand-files.mdx +++ b/docs/docs/learn-paimon/understand-files.mdx @@ -369,7 +369,9 @@ Let's say all 4 snapshots in the above diagram are about to expire. The expire p If any directories are left empty after the deletion process, they will be deleted as well, but only when `snapshot.clean-empty-directories` is enabled (default is `false`). -By default, empty directories are kept on disk. See [Manage Snapshots](../maintenance/manage-snapshots#expire-snapshots). +By default, Paimon does not actively delete empty directories or their markers. On an object store, +an implicit prefix may still cease to be visible after its last object is deleted. See +[Manage Snapshots](../maintenance/manage-snapshots#expire-snapshots). Let's say another snapshot, `snapshot-5` is created and snapshot expiration is triggered. `snapshot-1` to `snapshot-4` are diff --git a/docs/docs/maintenance/manage-partitions.md b/docs/docs/maintenance/manage-partitions.md index b5a471a6b951..dd9a3aa31eb7 100644 --- a/docs/docs/maintenance/manage-partitions.md +++ b/docs/docs/maintenance/manage-partitions.md @@ -49,10 +49,11 @@ __Note:__ After the partition expires, it is logically deleted and the latest sn files in the file system are not immediately physically deleted, it depends on when the corresponding snapshot expires. See [Expire Snapshots](./manage-snapshots#expire-snapshots). -Also, even after the data files are physically deleted by snapshot expiration, the empty partition directories are -**not** removed by default. To clean up empty directories, set -`'snapshot.clean-empty-directories' = 'true'` on the table. Please note that on object stores (e.g. OSS, S3) -this may cause performance issues, which is why the option defaults to `false`. +Also, even after snapshot expiration physically deletes the data files, Paimon does not actively remove empty +partition directories or their markers by default. An implicit object-store prefix may nevertheless cease to be +visible after its last object is deleted. To make Paimon additionally try to remove visible empty directories and +markers, set `'snapshot.clean-empty-directories' = 'true'` on the table. This may cause performance issues on +object stores (e.g. OSS, S3), which is why the option defaults to `false`. ::: diff --git a/docs/docs/maintenance/manage-snapshots.mdx b/docs/docs/maintenance/manage-snapshots.mdx index 69b972cb2699..237a73ddf287 100644 --- a/docs/docs/maintenance/manage-snapshots.mdx +++ b/docs/docs/maintenance/manage-snapshots.mdx @@ -88,14 +88,14 @@ Snapshot expiration is controlled by the following table properties. No false Boolean - Whether to try to delete empty directories (e.g. partition and bucket directories) left behind after the data files are deleted during snapshot expiration. Defaults to false: empty directories are kept. Enabling it has caveats: HDFS may print exceptions in NameNode, and object stores (OSS/S3) may suffer performance issues due to the extra prefix operations required to list and delete directory markers. + Whether Paimon tries to delete empty directories (e.g. partition and bucket directories) left behind after data files are deleted during snapshot expiration. The default is false, so Paimon does not actively delete those directories or their markers. An object-store prefix that was never explicitly created may still cease to be visible after its last object is deleted. Enabling the option has caveats: HDFS may print exceptions in NameNode, and object stores (OSS/S3) may suffer performance issues due to the extra prefix operations required to list and delete directory markers. When the number of snapshots is less than `snapshot.num-retained.min`, no snapshots will be expired(even the condition `snapshot.time-retained` meet), after which `snapshot.num-retained.max` and `snapshot.time-retained` will be used to control the snapshot expiration until the remaining snapshot meets the condition. -Note that snapshot expiration is also what physically deletes data files dropped by [partition expiration](./manage-partitions#expiring-partitions). However, the empty partition and bucket directories left behind after the data files are deleted are **not** removed by default. To clean them up, enable `snapshot.clean-empty-directories` (see the option above). This is off by default because on object stores (OSS/S3) the prefix operations needed to delete directory markers can be expensive. +Note that snapshot expiration is also what physically deletes data files dropped by [partition expiration](./manage-partitions#expiring-partitions). By default, Paimon does not actively remove empty partition and bucket directories or their markers. On object stores, an implicit prefix may nevertheless cease to be visible when its last object is deleted. To make Paimon additionally try to remove visible empty directories and markers, enable `snapshot.clean-empty-directories` (see the option above). This is off by default because the required prefix operations can be expensive on object stores (OSS/S3). The following example show more details(`snapshot.num-retained.min` is 2, `snapshot.time-retained` is 1h, `snapshot.num-retained.max` is 5): diff --git a/docs/docs/program-api/file-io.md b/docs/docs/program-api/file-io.md index 31664ad7a7ce..3fe653cc06dc 100644 --- a/docs/docs/program-api/file-io.md +++ b/docs/docs/program-api/file-io.md @@ -61,6 +61,13 @@ that the data is persistent and visible. Writing a file below a missing director parent paths visible as directories through `exists(...)` and `getFileStatus(...)`. Object store implementations do not need to create a physical directory marker for every parent. +A missing directory successfully created by `mkdirs(...)` before its descendants are written is +explicit: deleting or moving its last child does not delete that directory. A parent that becomes +visible only because a descendant was written is implicit. It must remain visible while any +descendant exists, but after the last descendant is deleted or moved away it may either remain as +an empty directory or become missing. This difference does not require callers to inspect physical +directory markers. + ## File Status and Listing `getFileStatus(...)` returns a `FileStatus` for an existing path. `getPath()` returns the path, @@ -93,7 +100,8 @@ from `getFileStatus(...)`, without requiring a separate `exists(...)` call. - `rename(...)` moves a file or directory to the exact destination path. Call it with an existing source, a different destination that does not exist, and an existing destination parent in the same underlying file system. On success, it returns `true`, removes the source path, and preserves - the file content or complete directory tree. + the file content or complete directory tree. Moving the last descendant out of an explicit source + parent leaves that parent as an empty directory; an implicit source parent may become missing. - `copyFile(...)` copies the source bytes to the exact destination. An existing destination is replaced only when `overwrite=true`. When a source directory contains files only, `copyFiles(...)` applies the same behavior to each direct file. diff --git a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java index 9ffff36d3ba3..857bd8be06ab 100644 --- a/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java +++ b/paimon-common/src/test/java/org/apache/paimon/fs/FileIOContractTestBase.java @@ -427,6 +427,48 @@ void testDeleteReturnsTrueForExistingTargets() throws IOException { assertThat(contractFileIO().delete(directory, false)).isTrue(); } + @Test + void testDeleteLastChildKeepsExplicitParentDirectory() throws IOException { + Path parent = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(parent); + Path child = new Path(parent, randomName()); + writeBytes(child, DEFAULT_CONTENT, false); + + assertThat(contractFileIO().delete(child, false)).isTrue(); + + assertThat(contractFileIO().getFileStatus(parent).isDir()).isTrue(); + assertThat(contractFileIO().listStatus(parent)).isEmpty(); + } + + @Test + void testDeleteLastChildAllowsImplicitParentToDisappear() throws IOException { + Path parent = new Path(contractBasePath(), randomName()); + Path child = new Path(parent, randomName()); + writeBytes(child, DEFAULT_CONTENT, false); + assertThat(contractFileIO().getFileStatus(parent).isDir()).isTrue(); + + assertThat(contractFileIO().delete(child, false)).isTrue(); + + assertMissingOrEmptyDirectory(parent); + } + + @Test + void testDeleteChildKeepsImplicitParentVisibleWhileSiblingExists() throws IOException { + Path parent = new Path(contractBasePath(), randomName()); + Path deleted = new Path(parent, randomName()); + Path sibling = new Path(parent, randomName()); + writeBytes(deleted, new byte[] {1}, false); + writeBytes(sibling, new byte[] {2}, false); + + assertThat(contractFileIO().delete(deleted, false)).isTrue(); + + assertThat(contractFileIO().getFileStatus(parent).isDir()).isTrue(); + assertThat(readBytes(sibling)).containsExactly(2); + assertThat(contractFileIO().listStatus(parent)) + .extracting(FileStatus::getPath) + .containsExactly(sibling); + } + // ------------------------------------------------------------------------ // Rename // ------------------------------------------------------------------------ @@ -443,6 +485,54 @@ void testRenameFileMovesExactBytesToMissingDestination() throws IOException { assertThat(readBytes(destination)).containsExactly(content); } + @Test + void testRenameLastChildKeepsExplicitSourceParentDirectory() throws IOException { + Path sourceParent = new Path(contractBasePath(), randomName()); + contractFileIO().mkdirs(sourceParent); + Path source = new Path(sourceParent, randomName()); + Path destination = new Path(contractBasePath(), randomName()); + writeBytes(source, DEFAULT_CONTENT, false); + + assertThat(contractFileIO().rename(source, destination)).isTrue(); + + assertThat(contractFileIO().getFileStatus(sourceParent).isDir()).isTrue(); + assertThat(contractFileIO().listStatus(sourceParent)).isEmpty(); + assertThat(readBytes(destination)).containsExactly(DEFAULT_CONTENT); + } + + @Test + void testRenameLastChildAllowsImplicitSourceParentToDisappear() throws IOException { + Path sourceParent = new Path(contractBasePath(), randomName()); + Path source = new Path(sourceParent, randomName()); + Path destination = new Path(contractBasePath(), randomName()); + writeBytes(source, DEFAULT_CONTENT, false); + assertThat(contractFileIO().getFileStatus(sourceParent).isDir()).isTrue(); + + assertThat(contractFileIO().rename(source, destination)).isTrue(); + + assertMissingOrEmptyDirectory(sourceParent); + assertThat(readBytes(destination)).containsExactly(DEFAULT_CONTENT); + } + + @Test + void testRenameChildKeepsImplicitSourceParentVisibleWhileSiblingExists() throws IOException { + Path sourceParent = new Path(contractBasePath(), randomName()); + Path source = new Path(sourceParent, randomName()); + Path sibling = new Path(sourceParent, randomName()); + Path destination = new Path(contractBasePath(), randomName()); + writeBytes(source, new byte[] {1}, false); + writeBytes(sibling, new byte[] {2}, false); + + assertThat(contractFileIO().rename(source, destination)).isTrue(); + + assertThat(contractFileIO().getFileStatus(sourceParent).isDir()).isTrue(); + assertThat(readBytes(sibling)).containsExactly(2); + assertThat(contractFileIO().listStatus(sourceParent)) + .extracting(FileStatus::getPath) + .containsExactly(sibling); + assertThat(readBytes(destination)).containsExactly(1); + } + @Test void testRenameDirectoryMovesExactTreeToMissingDestination() throws IOException { Path source = new Path(contractBasePath(), randomName()); @@ -794,6 +884,14 @@ private void assertOpenOrFirstReadFails(Path path) throws IOException { } } + private void assertMissingOrEmptyDirectory(Path path) throws IOException { + if (!contractFileIO().exists(path)) { + return; + } + assertThat(contractFileIO().getFileStatus(path).isDir()).isTrue(); + assertThat(contractFileIO().listStatus(path)).isEmpty(); + } + private static byte[] readAll(SeekableInputStream in) throws IOException { ByteArrayOutputStream out = new ByteArrayOutputStream(); byte[] buffer = new byte[4];