zjw1111 commented on code in PR #189:
URL: https://github.com/apache/paimon-cpp/pull/189#discussion_r3746515896
##########
src/paimon/common/fs/object_store_file_system.cpp:
##########
@@ -388,6 +388,17 @@ Result<std::unique_ptr<InputStream>>
ObjectStoreFileSystem::Open(const std::stri
ToUri(object_path),
metadata.value().size);
}
+Result<std::unique_ptr<InputStream>> ObjectStoreFileSystem::Open(const
std::string& path,
+ int64_t
file_size) const {
+ PAIMON_RETURN_NOT_OK(ValidateValueNonNegative(file_size, "file size"));
+ PAIMON_ASSIGN_OR_RAISE(ObjectStorePath object_path, ParsePath(path));
+ if (object_path.key.empty()) {
Review Comment:
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.
##########
src/paimon/common/reader/prefetch_file_batch_reader_impl.cpp:
##########
@@ -62,6 +62,19 @@ Result<std::unique_ptr<PrefetchFileBatchReaderImpl>>
PrefetchFileBatchReaderImpl
const std::shared_ptr<Executor>& executor, bool initialize_read_ranges,
PrefetchCacheMode prefetch_cache_mode, const CacheConfig& cache_config,
const std::shared_ptr<MemoryPool>& pool) {
+ return Create(data_file_path, /*data_file_size=*/-1, reader_builder, fs,
Review Comment:
`ObjectStoreFileSystem::Open(path, file_size)` starts with
`ValidateValueNonNegative(file_size, "file size")`, so this `-1` sentinel makes
the legacy overload fail with `Status::Invalid("file size -1 is less than 0")`
on any object-store filesystem, instead of quietly falling back to
`Open(path)`. It is not caught today only because every remaining caller of
this overload is a test using `LocalFileSystem` / `MockFileSystem`, where the
base-class default ignores the size — so the sentinel happens to be harmless
there and CI stays green.
Instead of defining what a negative size means, could you drop the old
`Create(data_file_path, reader_builder, ...)` overload entirely and migrate the
remaining callers (the prefetch reader tests and
`apply_deletion_vector_batch_reader_test.cpp`) to the new one that takes
`data_file_size`? That leaves a single code path and removes the sentinel
altogether.
##########
include/paimon/fs/file_system.h:
##########
@@ -193,6 +193,15 @@ 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 file for reading with a known file size.
+ ///
+ /// File systems that can use the size to avoid metadata requests may
override this method.
+ /// The default implementation ignores the size and opens the file
normally.
+ virtual Result<std::unique_ptr<InputStream>> Open(const std::string& path,
Review Comment:
Could you expand this doc comment to spell out the contract? The
neighbouring methods use `@param` / `@return`, and a few things are currently
left undefined:
- whether the caller must guarantee that `file_size` matches the object
exactly, and what happens when it does not (silently truncated or out-of-range
reads deep inside the format reader, rather than a clean error at open time);
- whether a negative value means "size unknown", and whether `0` is a legal
value.
The second point matters because the implementations already disagree: this
default accepts any value, while `ObjectStoreFileSystem::Open` rejects
negatives. Writing the contract down here would keep future filesystem
implementations consistent.
Two smaller things while you are in this header, both optional:
- `LocalFileSystem`, `JindoFileSystem` and `MockFileSystem` declare only the
single-argument `Open`, which hides this overload for anyone holding a concrete
type (calls through `FileSystem*` are fine, which is why everything still
compiles). Adding `using FileSystem::Open;` to those classes would avoid a
confusing error later.
- Any decorating filesystem that forwards only `Open(path)` stays correct
but silently loses the optimization. `ResolvingFileSystem` is handled in this
PR; a note in the doc comment that wrappers should forward both overloads would
help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
