Copilot commented on code in PR #875:
URL: https://github.com/apache/iceberg-cpp/pull/875#discussion_r4470072742
##########
src/iceberg/file_io.cc:
##########
@@ -19,15 +19,98 @@
#include "iceberg/file_io.h"
+#include <algorithm>
+#include <cstdint>
+#include <cstring>
#include <limits>
+#include <memory>
+#include <mutex>
+#include <optional>
+#include <string>
+#include <unordered_map>
#include <utility>
+#include "iceberg/util/cache_internal.h"
#include "iceberg/util/macros.h"
+#include "iceberg/util/string_util.h"
namespace iceberg {
+namespace internal {
+
+using MetadataContent = std::shared_ptr<const std::string>;
+
+// Weighs cached metadata content by its size in bytes.
+struct MetadataContentWeigher {
+ int64_t operator()(const std::string& /*location*/,
+ const MetadataContent& content) const {
+ return static_cast<int64_t>(content->size());
+ }
Review Comment:
A zero-length content gets weight 0 here, so `max-total-bytes` never evicts
it. An unbounded sequence of empty manifest paths can therefore grow the LRU's
map/list without limit even though this cache is advertised as bounded. Skip
zero-length files or assign every cached entry a positive minimum weight (and
account for that in the limit).
##########
src/iceberg/file_io.cc:
##########
@@ -69,21 +327,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));
}
Review Comment:
This resolves the backing `InputFile` before checking the cache, so a cache
hit still depends on `NewInputFile` succeeding. That breaks the intended reuse
when a resolver cannot load a delegate (for example, an expired vended
credential or a deleted object): the bytes are already cached, but
`ResolvingFileIO::FileIOForPath`/`ArrowS3FileIO::FileIOForPath` can fail first.
Check the cache before resolving the backing file, while preserving the
canonical key used for normalized locations, so only misses need storage
resolution.
--
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]
