lxy-9602 commented on code in PR #430:
URL: https://github.com/apache/paimon-cpp/pull/430#discussion_r4305571990
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -396,44 +388,81 @@ Status FileStoreScan::ReadManifestEntries(const
std::vector<ManifestFileMeta>& m
// bucket counts, with the current count and schema in the cache key. Exact
hits
// can be returned directly; cache misses rebuild the target snapshot bucket
from the target
// snapshot's data manifests.
-Status FileStoreScan::ReadManifestEntriesWithCache(
+Result<uint64_t> FileStoreScan::ReadManifestEntriesWithCache(
const Snapshot& snapshot, const std::vector<ManifestFileMeta>&
all_manifest_metas,
int32_t bucket, std::vector<ManifestEntry>* manifest_entries, bool*
cache_hit) const {
Duration cache_load_duration;
- PAIMON_ASSIGN_OR_RAISE(SnapshotLiveManifestEntries cached_entries,
- LoadSnapshotLiveManifestEntries(bucket));
+ const int32_t max_snapshots =
core_options_.GetScanManifestEntryCacheMaxSnapshots();
+ SnapshotLiveManifestEntries cached_entries(max_snapshots);
+ bool predicate_failed = false;
+ auto row_filter = CreateManifestEntryRowFilter(&predicate_failed);
+ bool partition_filtered = false;
+ uint64_t candidate_entries = 0;
+ *cache_hit = false;
+ Result<std::shared_ptr<CacheValue>> value = core_options_.GetCache()->Get(
+ CreateSnapshotLiveManifestEntriesCacheKey(bucket),
+ [](const std::shared_ptr<CacheKey>&) ->
Result<std::shared_ptr<CacheValue>> {
+ return std::shared_ptr<CacheValue>();
+ });
Review Comment:
Thanks a lot for your contribution—this PR looks high-quality and
well-tested.
I have one question. I looked at the Java code: the cache key is the
manifest file path, and internally it segments and builds indexes by
(partition, bucket, totalBucket).
One thing seems a bit odd: the current cache is built per (snapshot-bucket),
and during queries we filter by partition. But the data is organized as
(snapshot-partition-bucket). The current cache key drops the partition level;
the value contains all partition for a bucket. What’s the rationale for this
design?
This isn’t blocking the current PR. I’d also like to know whether the cache
will be restructured in the future to match Java’s (partition, bucket)-level
hierarchy.
--
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]
