Copilot commented on code in PR #875:
URL: https://github.com/apache/iceberg-cpp/pull/875#discussion_r4400281450
##########
src/iceberg/file_io.cc:
##########
@@ -69,21 +221,84 @@ 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(file_location)) {
+ return std::make_unique<CachedInputFile>(std::move(input_file),
std::move(cache),
+
static_cast<int64_t>(cached->size()));
+ }
Review Comment:
`NewCachedInputFile` checks the cache using `file_location`, but
`CachedInputFile` later uses `input_file_->location()` (stored as `location_`)
as the cache key. If `InputFile::location()` normalizes/rewrites the path
(common with resolving IOs, scheme normalization, etc.), this can cause missed
hits and even duplicate cache entries for the same underlying object. Use a
single canonical cache key derived from the created `InputFile` (e.g.,
`std::string(input_file->location())`) consistently for `GetIfPresent`, `Get`,
and invalidation behavior.
##########
src/iceberg/arrow/arrow_io.cc:
##########
@@ -423,29 +424,46 @@ class ArrowInputFile : public InputFile {
std::string_view location() const override { return location_; }
Result<int64_t> Size() const override {
+ std::lock_guard lock(mutex_);
if (file_size_.has_value()) {
return *file_size_;
}
::arrow::fs::FileInfo file_info(path_, ::arrow::fs::FileType::File);
ICEBERG_ARROW_ASSIGN_OR_RETURN(auto input, fs_->OpenInputFile(file_info));
ICEBERG_ARROW_ASSIGN_OR_RETURN(auto size, input->GetSize());
+ file_size_ = size;
+ pending_input_ = std::move(input);
Review Comment:
`Size()` now opens and retains a `RandomAccessFile` in `pending_input_`
until the next `Open()`/`OpenArrowFile()` call. If callers frequently call
`Size()` (or `InputFile::Size()` is used for planning) without subsequently
opening the file, this can hold file handles / remote resources longer than
necessary and increase FD pressure. Consider either (a) not retaining the
handle unless you know `Open()` will follow immediately, (b) retaining it only
under a small bounded scope (e.g., via a short-lived cache), or (c) explicitly
releasing/closing `pending_input_` after extracting size when `Open()` isn’t
guaranteed.
##########
src/iceberg/file_io.h:
##########
@@ -154,6 +157,30 @@ 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.
Review Comment:
PR description says the cache covers “metadata JSON, manifest lists, and
manifests”, but the comment here explicitly says table metadata JSON should not
use the cached path, and the reviewed call-site changes only flip
`cache_content=true` for manifests / manifest lists. Please align either the PR
description or this API contract (and/or add the missing routing for metadata
JSON if that’s intended in this PR).
--
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]
