wangyong9999 commented on code in PR #267:
URL: https://github.com/apache/paimon-cpp/pull/267#discussion_r3895171422
##########
src/paimon/common/global_index/btree/btree_global_indexer.cpp:
##########
@@ -57,7 +91,8 @@ Result<std::unique_ptr<BTreeGlobalIndexer>>
BTreeGlobalIndexer::Create(
double high_priority_pool_ratio,
OptionsUtils::GetValueFromMap<double>(options,
BtreeDefs::kBtreeIndexHighPriorityPoolRatio,
BtreeDefs::kDefaultBtreeIndexHighPriorityPoolRatio));
- auto cache_manager = std::make_shared<CacheManager>(cache_size,
high_priority_pool_ratio);
+ std::shared_ptr<CacheManager> cache_manager =
+ GetSharedCacheManager(cache_size, high_priority_pool_ratio);
Review Comment:
This manager is shared across all indexers with the same size settings, but
page keys contain only `file_path/offset/length`. Two custom `FileSystem` or
reader backends can expose the same logical path with different bytes, so the
second scan can reuse the first backend's pages without opening its own file
and return wrong matches. Include a stable backend namespace in the key, or
scope the manager by backend.
##########
src/paimon/common/global_index/btree/btree_global_indexer.cpp:
##########
@@ -42,9 +45,40 @@
#include "paimon/executor.h"
#include "paimon/global_index/bitmap_global_index_result.h"
#include "paimon/memory/bytes.h"
+#include "paimon/memory/memory_pool.h"
#include "paimon/utils/roaring_bitmap64.h"
namespace paimon {
+namespace {
+
+struct SharedCacheManager {
+ SharedCacheManager(int64_t cache_size, double high_priority_pool_ratio)
+ : cache_pool(GetDefaultPool()),
+ cache_manager(std::make_shared<CacheManager>(cache_size,
high_priority_pool_ratio)) {}
+
+ // Keep the allocator alive until after cache_manager releases all cached
pages.
+ std::shared_ptr<MemoryPool> cache_pool;
+ std::shared_ptr<CacheManager> cache_manager;
+};
+
+std::shared_ptr<CacheManager> GetSharedCacheManager(int64_t cache_size,
+ double
high_priority_pool_ratio) {
+ using CacheConfig = std::pair<int64_t, double>;
+ static std::mutex mutex;
+ static std::map<CacheConfig, SharedCacheManager> cache_managers;
Review Comment:
`cache_managers` owns a full LRU for every distinct table-level
`(cache_size, ratio)` forever. Each cache is bounded, but the process total is
not; reading tables with different settings keeps all previous budgets
resident. Please use one process-wide budget, or bound and evict this registry.
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -150,12 +150,12 @@ Result<std::shared_ptr<FileStoreScan::RawPlan>>
FileStoreScan::CreatePlan() cons
snapshot.has_value() && scan_mode_ == ScanMode::ALL &&
core_options_.GetScanManifestEntryCacheMaxSnapshots() > 0 &&
core_options_.GetCache() != nullptr && !table_path_.empty() &&
- !row_range_index_.has_value() && bucket_filter_.has_value();
+ !row_range_index_.has_value();
Review Comment:
The whole-table key is only `(table_path, branch)`, and a hit is accepted by
numeric snapshot ID. After a table is dropped and recreated at the same path,
snapshot IDs restart while the caller-owned cache can still hold the old entry,
so an ordinary scan can return stale splits. Include a table or snapshot
generation identity (for example the manifest-list name) in the cached
entry/key, or invalidate it on recreation.
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -347,16 +347,21 @@ Status FileStoreScan::ReadManifestEntriesWithCache(
}
*cache_hit = false;
- // Rebuild the target snapshot bucket from all manifests and write the
live entries back to the
- // cache.
- std::vector<ManifestFileMeta> bucket_manifest_metas;
- for (const auto& meta : all_manifest_metas) {
- if (MayContainBucket(meta, bucket)) {
- bucket_manifest_metas.push_back(meta);
+ if (bucket) {
+ std::vector<ManifestFileMeta> bucket_manifest_metas;
+ for (const auto& meta : all_manifest_metas) {
+ if (MayContainBucket(meta, bucket.value())) {
+ bucket_manifest_metas.push_back(meta);
+ }
}
+ PAIMON_RETURN_NOT_OK(
+ ReadAndMergeBucketFileEntries(bucket_manifest_metas,
bucket.value(), manifest_entries));
+ } else {
+ std::vector<ManifestEntry> unmerged_entries;
+ PAIMON_RETURN_NOT_OK(
+ ReadFileEntries(all_manifest_metas, &unmerged_entries,
/*apply_scan_filter=*/false));
Review Comment:
On this no-bucket miss we materialize every manifest entry, then serialize
up to `max_snapshots` full table states. If that value exceeds the cache
capacity, `Put` fails silently and every later scan of the same snapshot
repeats the full read, merge, copy, and serialization. Only use this path when
the value can be retained; otherwise fall back to
`filtered_manifest_file_metas`, or store snapshots separately so older states
can be evicted.
--
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]
