Copilot commented on code in PR #875:
URL: https://github.com/apache/iceberg-cpp/pull/875#discussion_r3795562924
##########
src/iceberg/snapshot.cc:
##########
@@ -32,6 +35,67 @@
namespace iceberg {
+namespace internal {
+
+class SnapshotCacheData {
+ public:
+ Result<std::reference_wrapper<SnapshotCache::ManifestsCache>> Get(
+ const Snapshot* snapshot, std::shared_ptr<FileIO> file_io) {
+ std::shared_ptr<Entry> entry;
+ {
+ std::unique_lock lock(mutex_);
+ while (true) {
+ auto [it, inserted] = entries_.try_emplace(snapshot->manifest_list);
+ if (inserted) {
+ it->second = std::make_shared<Entry>();
+ }
+ entry = it->second;
+ if (entry->value.has_value()) {
+ return std::ref(*entry->value);
+ }
+ if (!entry->loading) {
+ entry->loading = true;
+ break;
+ }
+ entry->loaded.wait(lock, [&entry] { return !entry->loading; });
+ }
+ }
+
+ auto loaded = SnapshotCache::InitManifestsCache(snapshot,
std::move(file_io));
+ std::lock_guard lock(mutex_);
+ auto it = entries_.find(snapshot->manifest_list);
+ if (!loaded.has_value()) {
+ if (it != entries_.end() && it->second == entry) {
+ entries_.erase(it);
+ }
+ entry->loading = false;
+ entry->loaded.notify_all();
+ return std::unexpected<Error>(std::move(loaded).error());
+ }
+
+ entry->value = std::move(loaded).value();
+ entry->loading = false;
+ entry->loaded.notify_all();
+ return std::ref(*entry->value);
+ }
+
+ private:
+ struct Entry {
+ bool loading = false;
+ std::optional<SnapshotCache::ManifestsCache> value;
+ std::condition_variable loaded;
+ };
+
+ std::mutex mutex_;
+ std::unordered_map<std::string, std::shared_ptr<Entry>> entries_;
+};
Review Comment:
`SnapshotCacheData::entries_` is an unbounded map keyed by
`snapshot->manifest_list` with no eviction/expiration. In long-lived processes
or catalogs scanning many tables/snapshots, this can grow without bound (each
entry holds a full `ManifestsCache`, i.e., parsed manifest-list results). If
the intent is cross-query reuse, consider adding a bound (LRU/weight-based like
`MetadataCache`) and/or tying entry lifecycle to metadata cache invalidation
(e.g., when a manifest-list content entry is evicted/invalidated, evict the
parsed entry too).
##########
src/iceberg/snapshot.cc:
##########
@@ -32,6 +35,67 @@
namespace iceberg {
+namespace internal {
+
+class SnapshotCacheData {
+ public:
+ Result<std::reference_wrapper<SnapshotCache::ManifestsCache>> Get(
+ const Snapshot* snapshot, std::shared_ptr<FileIO> file_io) {
+ std::shared_ptr<Entry> entry;
+ {
+ std::unique_lock lock(mutex_);
+ while (true) {
+ auto [it, inserted] = entries_.try_emplace(snapshot->manifest_list);
+ if (inserted) {
+ it->second = std::make_shared<Entry>();
+ }
+ entry = it->second;
+ if (entry->value.has_value()) {
+ return std::ref(*entry->value);
+ }
+ if (!entry->loading) {
+ entry->loading = true;
+ break;
+ }
+ entry->loaded.wait(lock, [&entry] { return !entry->loading; });
+ }
+ }
Review Comment:
`SnapshotCacheData::entries_` is an unbounded map keyed by
`snapshot->manifest_list` with no eviction/expiration. In long-lived processes
or catalogs scanning many tables/snapshots, this can grow without bound (each
entry holds a full `ManifestsCache`, i.e., parsed manifest-list results). If
the intent is cross-query reuse, consider adding a bound (LRU/weight-based like
`MetadataCache`) and/or tying entry lifecycle to metadata cache invalidation
(e.g., when a manifest-list content entry is evicted/invalidated, evict the
parsed entry too).
--
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]
