Copilot commented on code in PR #875:
URL: https://github.com/apache/iceberg-cpp/pull/875#discussion_r4401703432
##########
src/iceberg/file_io.h:
##########
@@ -154,6 +157,33 @@ class ICEBERG_EXPORT FileIO {
virtual Result<std::string> ReadFile(const std::string& file_location,
std::optional<size_t> length);
+ /// \brief Create an input handle backed by the metadata content cache when
enabled.
+ ///
+ /// This is intended for immutable Iceberg manifest lists and manifests.
Data files
+ /// and table metadata JSON should use NewInputFile or ReadFile directly.
+ Result<std::unique_ptr<InputFile>> NewCachedInputFile(
+ std::string file_location, std::optional<size_t> length = std::nullopt);
Review Comment:
The PR description states the cache is for “metadata JSON, manifest lists,
and manifests”, but this new API comment says “table metadata JSON should use
NewInputFile or ReadFile directly.” Please align the documentation with the
intended behavior: either update this comment to include metadata JSON as
supported for caching, or adjust the PR description if metadata JSON caching is
intentionally excluded.
##########
src/iceberg/file_io.cc:
##########
@@ -69,21 +221,91 @@ Result<std::string> FileIO::ReadFile(const std::string&
file_location,
ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location));
ICEBERG_ASSIGN_OR_RAISE(read_size, input_file->Size());
}
- if (read_size < 0) {
- return Invalid("Invalid negative file size {} for {}", read_size,
file_location);
+ return ReadInputFile(*input_file, read_size, file_location);
+}
+
+Result<std::unique_ptr<InputFile>> FileIO::NewCachedInputFile(
+ std::string file_location, std::optional<size_t> length) {
+ std::unique_ptr<InputFile> input_file;
+ if (length.has_value()) {
+ ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location, *length));
+ } else {
+ ICEBERG_ASSIGN_OR_RAISE(input_file, NewInputFile(file_location));
}
- auto size = static_cast<size_t>(read_size);
- std::string content(size, '\0');
- ICEBERG_ASSIGN_OR_RAISE(auto stream, input_file->Open());
- Status read_status = {};
- if (size > 0) {
- auto bytes = std::as_writable_bytes(std::span(content.data(),
content.size()));
- read_status = stream->ReadFully(/*position=*/0, bytes);
+ auto cache = GetMetadataCache();
+ if (cache == nullptr || !cache->options().enabled) {
+ return input_file;
}
- ICEBERG_RETURN_UNEXPECTED(
- FinishWithCloseStatus(std::move(read_status), stream->Close()));
- return content;
+
+ if (auto cached = cache->GetIfPresent(input_file->location())) {
+ return std::make_unique<CachedInputFile>(std::move(input_file),
std::move(cache),
+
static_cast<int64_t>(cached->size()));
Review Comment:
`cached->size()` is a `size_t` that is narrowed to `int64_t` without bounds
checking. If a cached entry size exceeds `int64_t` max, this conversion is
implementation-defined and can wrap negative, which then propagates into
`CachedInputFile` sizing and can lead to incorrect behavior (including
huge/negative sizes). Consider guarding `cached->size()` against
`std::numeric_limits<int64_t>::max()` and either (a) return an error, or (b)
bypass caching and return the underlying `input_file` in that case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
