Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 48 additions & 5 deletions include/paimon/fs/file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <functional>
#include <memory>
#include <string>
#include <utility>
#include <vector>

#include "paimon/result.h"
Expand Down Expand Up @@ -151,7 +152,7 @@ class PAIMON_EXPORT BasicFileStatus {
virtual std::string GetPath() const = 0;
};

/// Extended file status information interface.
/// Extended file status information.
///
/// This class extends BasicFileStatus to provide comprehensive file system metadata including file
/// size, modification time, and other attributes. It's used for operations that require detailed
Expand All @@ -161,21 +162,45 @@ class PAIMON_EXPORT FileStatus {
FileStatus() = default;
virtual ~FileStatus() = default;

/// Sentinel returned by `GetModificationTime()` when the modification time is not known.
static constexpr int64_t kUnknownModificationTime = -1;

/// Create a file status from caller-supplied metadata.
/// @param path The path of the file or directory.
/// @param length The size of the file in bytes. It may be negative only when the size is
/// unknown.
/// @param is_dir Whether the path represents a directory. Defaults to false.
FileStatus(std::string path, int64_t length, bool is_dir = false)
: path_(std::move(path)), length_(length), is_dir_(is_dir) {}

/// Get the size of the file in bytes.
/// @note For directories, this method is undefined behavior.
virtual int64_t GetLen() const = 0;
virtual int64_t GetLen() const {
return length_;
}

/// Check if this entry represents a directory.
virtual bool IsDir() const = 0;
virtual bool IsDir() const {
return is_dir_;
}

/// Get the path of this file or directory.
virtual std::string GetPath() const = 0;
virtual std::string GetPath() const {
return path_;
}

/// Get the last modification time of the file.
///
/// @return A long value representing the time the file was last modified, measured in
/// milliseconds since the epoch (UTC January 1, 1970).
virtual int64_t GetModificationTime() const = 0;
virtual int64_t GetModificationTime() const {
return kUnknownModificationTime;
}

private:
std::string path_;
int64_t length_ = -1;
bool is_dir_ = false;
};

/// Abstract file system interface.
Expand All @@ -193,6 +218,24 @@ class PAIMON_EXPORT FileSystem {
/// failure (e.g., file not found, permission denied).
virtual Result<std::unique_ptr<InputStream>> Open(const std::string& path) const = 0;

/// Open an existing regular file for reading with known file metadata.
/// @param file_status The trusted status of the file to open. Its path and length must
/// identify an existing regular file. Its length must be non-negative;
/// zero is valid for an empty file.
/// @return Result containing a unique pointer to `InputStream` on success, or error status on
/// failure (e.g., invalid file size, file not found, permission denied).
/// @note File systems may rely on `file_status` to skip metadata requests. The caller must
/// not expect this method to validate the path, file type, or size. A stale or
/// incorrect status, or a file removed after planning, can cause reads to end early or
/// fail when read instead of failing at open time. Wrapping file systems should forward
/// both `Open` overloads.
virtual Result<std::unique_ptr<InputStream>> Open(const FileStatus& file_status) const {
if (file_status.GetLen() < 0) {
return Status::Invalid("file size must be non-negative");
}
return Open(file_status.GetPath());
}

/// Create a new file for writing.
/// @param path The file path to create.
/// @param overwrite If true, overwrite existing file; if false, fail if file exists.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,8 @@ class ApplyBitmapIndexBatchReaderTest : public ::testing::Test,
ASSERT_OK_AND_ASSIGN(
file_batch_reader,
PrefetchFileBatchReaderImpl::Create(
/*data_file_path=*/"DUMMY", &reader_builder, fs_, prefetch_batch_count,
batch_size, prefetch_batch_count * 2,
/*data_file_path=*/"DUMMY", /*data_file_size=*/0, &reader_builder, fs_,
prefetch_batch_count, batch_size, prefetch_batch_count * 2,
/*enable_adaptive_prefetch_strategy=*/false, executor_,
/*initialize_read_ranges=*/true,
/*prefetch_cache_mode=*/PrefetchCacheMode::ALWAYS, CacheConfig(), pool_));
Expand Down
14 changes: 14 additions & 0 deletions src/paimon/common/fs/file_system_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,20 @@ TEST_P(FileSystemTest, TestSimpleWriteAndRead) {
ASSERT_OK(in_stream->Close());
}

TEST_P(FileSystemTest, TestOpenWithKnownFileSize) {
const std::string content = "abcdefghijk";
const std::string file_path = test_root_ + "/file.data";
ASSERT_OK(fs_->WriteFile(file_path, content, /*overwrite=*/true));

FileStatus file_status(file_path, static_cast<int64_t>(content.size()));
ASSERT_OK_AND_ASSIGN(auto input_stream, fs_->Open(file_status));
ASSERT_OK_AND_ASSIGN(int64_t file_size, input_stream->Length());
ASSERT_EQ(file_size, content.size());
ASSERT_OK(input_stream->Close());

ASSERT_TRUE(fs_->Open(FileStatus(file_path, /*length=*/-1)).status().IsInvalid());
}

TEST_P(FileSystemTest, TestWriteMultipleTimes) {
std::vector<std::string> content_vec = {"abc", "defg", "hi", "j", "k"};
std::string content = "abcdefghijk";
Expand Down
13 changes: 13 additions & 0 deletions src/paimon/common/fs/object_store_file_system.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,19 @@ Result<std::unique_ptr<InputStream>> ObjectStoreFileSystem::Open(const std::stri
ToUri(object_path), metadata.value().size);
}

Result<std::unique_ptr<InputStream>> ObjectStoreFileSystem::Open(
const FileStatus& file_status) const {
const std::string path = file_status.GetPath();
const int64_t file_size = file_status.GetLen();
PAIMON_RETURN_NOT_OK(ValidateValueNonNegative(file_size, "file size"));
PAIMON_ASSIGN_OR_RAISE(ObjectStorePath object_path, ParsePath(path));
if (object_path.key.empty()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A question rather than a request: the single-argument Open falls back to DirectoryExists when HeadObject reports the key as missing, so that opening a common prefix reports "... is a directory" rather than a not-found error. Here the key.empty() check only catches the bucket-root case, and because no HeadObject is issued, a path that is really a directory-like prefix will be accepted and produce a stream that fails only later, during the read.

Is the intent that a caller supplying a trusted file_size can never be pointing at a directory, so the extra check is unnecessary? That seems reasonable to me, I would just like it stated in the API doc. The same reasoning applies to existence: this overload no longer detects a file removed between planning and reading, turning a clean NotExist at open time into a harder-to-read failure mid-read. Also an acceptable trade-off for the optimization, but worth documenting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in file_system.h

return Status::Invalid(fmt::format("{} is a directory", path));
}
return std::make_unique<ObjectStoreInputStream>(client_, read_ahead_limiter_, object_path,
ToUri(object_path), file_size);
}

Result<std::unique_ptr<FileStatus>> ObjectStoreFileSystem::GetFileStatus(
const std::string& path) const {
PAIMON_ASSIGN_OR_RAISE(ObjectStorePath object_path, ParsePath(path));
Expand Down
1 change: 1 addition & 0 deletions src/paimon/common/fs/object_store_file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class PAIMON_EXPORT ObjectStoreFileSystem : public FileSystem {
~ObjectStoreFileSystem() override = default;

Result<std::unique_ptr<InputStream>> Open(const std::string& path) const override;
Result<std::unique_ptr<InputStream>> Open(const FileStatus& file_status) const override;
Result<std::unique_ptr<FileStatus>> GetFileStatus(const std::string& path) const override;
Status ListDir(const std::string& directory,
std::vector<std::unique_ptr<BasicFileStatus>>* file_status_list) const override;
Expand Down
9 changes: 9 additions & 0 deletions src/paimon/common/fs/object_store_file_system_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,15 @@ TEST(ObjectStoreFileSystemTest, TestOpenBucketRootIsDirectory) {
ASSERT_EQ(client->list_calls_, 0);
}

TEST(ObjectStoreFileSystemTest, TestOpenWithKnownLengthSkipsHead) {
auto client = std::make_shared<MockObjectStoreClient>();
client->objects_["file"] = "data";
ObjectStoreFileSystem fs("s3", client);
ASSERT_OK_AND_ASSIGN(auto stream, fs.Open(FileStatus("s3://bucket/file", 4)));
ASSERT_EQ(stream->Length().value(), 4);
ASSERT_EQ(client->head_calls_, 0);
}

TEST(ObjectStoreFileSystemTest, TestPathWithLeadingSlashes) {
auto client = std::make_shared<MockObjectStoreClient>();
client->objects_["file"] = "data";
Expand Down
7 changes: 7 additions & 0 deletions src/paimon/common/fs/resolving_file_system.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ Result<std::unique_ptr<InputStream>> ResolvingFileSystem::Open(const std::string
return fs->Open(path);
}

Result<std::unique_ptr<InputStream>> ResolvingFileSystem::Open(
const FileStatus& file_status) const {
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<FileSystem> fs,
GetRealFileSystem(file_status.GetPath()));
return fs->Open(file_status);
}

Result<std::unique_ptr<OutputStream>> ResolvingFileSystem::Create(const std::string& path,
bool overwrite) const {
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<FileSystem> fs, GetRealFileSystem(path));
Expand Down
1 change: 1 addition & 0 deletions src/paimon/common/fs/resolving_file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class ResolvingFileSystem : public FileSystem {
~ResolvingFileSystem() override = default;

Result<std::unique_ptr<InputStream>> Open(const std::string& path) const override;
Result<std::unique_ptr<InputStream>> Open(const FileStatus& file_status) const override;
Result<std::unique_ptr<OutputStream>> Create(const std::string& path,
bool overwrite) const override;

Expand Down
24 changes: 13 additions & 11 deletions src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ std::pair<int64_t, int64_t> ComputeBatchSliceByReadRange(
} // namespace

Result<std::unique_ptr<PrefetchFileBatchReaderImpl>> PrefetchFileBatchReaderImpl::Create(
const std::string& data_file_path, const ReaderBuilder* reader_builder,
const std::string& data_file_path, int64_t data_file_size, const ReaderBuilder* reader_builder,
const std::shared_ptr<FileSystem>& fs, uint32_t prefetch_max_parallel_num, int32_t batch_size,
uint32_t prefetch_batch_count, bool enable_adaptive_prefetch_strategy,
const std::shared_ptr<Executor>& executor, bool initialize_read_ranges,
Expand All @@ -83,20 +83,22 @@ Result<std::unique_ptr<PrefetchFileBatchReaderImpl>> PrefetchFileBatchReaderImpl

std::shared_ptr<ReadAheadCache> cache;
if (prefetch_cache_mode != PrefetchCacheMode::NEVER) {
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<InputStream> input_stream, fs->Open(data_file_path));
PAIMON_ASSIGN_OR_RAISE(std::shared_ptr<InputStream> input_stream,
fs->Open(FileStatus(data_file_path, data_file_size)));
cache = std::make_shared<ReadAheadCache>(input_stream, cache_config, pool);
}
std::vector<std::future<Result<std::unique_ptr<FileBatchReader>>>> futures;
for (uint32_t i = 0; i < prefetch_max_parallel_num; i++) {
futures.push_back(Via(executor.get(),
[&fs, &data_file_path, &reader_builder,
&cache]() -> Result<std::unique_ptr<FileBatchReader>> {
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<InputStream> input_stream,
fs->Open(data_file_path));
auto cache_input_stream = std::make_shared<CacheInputStream>(
std::move(input_stream), cache);
return reader_builder->Build(cache_input_stream);
}));
futures.push_back(
Via(executor.get(),
[&fs, &data_file_path, data_file_size, &reader_builder,
&cache]() -> Result<std::unique_ptr<FileBatchReader>> {
PAIMON_ASSIGN_OR_RAISE(std::unique_ptr<InputStream> input_stream,
fs->Open(FileStatus(data_file_path, data_file_size)));
auto cache_input_stream =
std::make_shared<CacheInputStream>(std::move(input_stream), cache);
return reader_builder->Build(cache_input_stream);
}));
}
std::vector<std::shared_ptr<PrefetchFileBatchReader>> readers;
for (auto& file_batch_reader : CollectAll(futures)) {
Expand Down
12 changes: 6 additions & 6 deletions src/paimon/common/reader/prefetch_file_batch_reader_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ class Metrics;
class PrefetchFileBatchReaderImpl : public PrefetchFileBatchReader {
public:
static Result<std::unique_ptr<PrefetchFileBatchReaderImpl>> Create(
const std::string& data_file_path, const ReaderBuilder* reader_builder,
const std::shared_ptr<FileSystem>& fs, uint32_t prefetch_max_parallel_num,
int32_t batch_size, uint32_t prefetch_batch_count, bool enable_adaptive_prefetch_strategy,
const std::shared_ptr<Executor>& executor, bool initialize_read_ranges,
PrefetchCacheMode prefetch_cache_mode, const CacheConfig& cache_config,
const std::shared_ptr<MemoryPool>& pool);
const std::string& data_file_path, int64_t data_file_size,
const ReaderBuilder* reader_builder, const std::shared_ptr<FileSystem>& fs,
uint32_t prefetch_max_parallel_num, int32_t batch_size, uint32_t prefetch_batch_count,
bool enable_adaptive_prefetch_strategy, const std::shared_ptr<Executor>& executor,
bool initialize_read_ranges, PrefetchCacheMode prefetch_cache_mode,
const CacheConfig& cache_config, const std::shared_ptr<MemoryPool>& pool);

~PrefetchFileBatchReaderImpl() override;

Expand Down
Loading
Loading