wangyong9999 commented on code in PR #374:
URL: https://github.com/apache/paimon-cpp/pull/374#discussion_r4357004754
##########
src/paimon/core/manifest/snapshot_live_manifest_entries.h:
##########
@@ -45,11 +45,13 @@ class SnapshotLiveManifestEntries {
struct Selection {
std::vector<ManifestEntry> entries;
uint64_t candidate_entries = 0;
+ int64_t total_data_files = -1;
};
explicit SnapshotLiveManifestEntries(int32_t max_snapshots);
- void Put(int64_t snapshot_id, std::vector<ManifestEntry>&& entries);
+ void Put(int64_t snapshot_id, std::vector<ManifestEntry>&& entries,
+ int64_t total_data_files = -1);
Review Comment:
The only production `Put` caller always passes the file count, and the magic
bump already rejects old values, so the `-1` default exists only for tests.
Make `total_data_files` a required argument and drop the `total_data_files >=
0` check in the hit condition (`file_store_scan.cpp:423`). The sentinel adds a
third state that is never reached in practice.
##########
docs/source/user_guide/manifest_entry_cache.rst:
##########
@@ -120,3 +120,7 @@ The scan metrics expose existing counters for the last scan:
- ``lastScannedManifests``: how many manifest files were loaded during this
scan before manifest entry decoding.
+
+On an exact cache hit, ``lastScannedManifests`` is zero. The cached table file
+count preserves the skipped-file metric even when manifest lists are bypassed.
+This changes only transient cache values; persisted table formats are
unchanged.
Review Comment:
"This changes only transient cache values; persisted table formats are
unchanged." explains the PR, not the feature. I'd drop it from the user guide.
##########
src/paimon/core/operation/file_store_scan.cpp:
##########
@@ -240,11 +242,13 @@ Result<std::shared_ptr<FileStoreScan::RawPlan>>
FileStoreScan::CreatePlan() cons
entry = entry.CopyWithoutStats();
}
}
- const int64_t all_data_files = std::accumulate(
- all_manifest_file_metas.begin(), all_manifest_file_metas.end(),
int64_t{0},
- [](const int64_t sum, const ManifestFileMeta& manifest_file_meta) {
- return sum + manifest_file_meta.NumAddedFiles() -
manifest_file_meta.NumDeletedFiles();
- });
+ if (!snapshot_cache_hit) {
Review Comment:
On a cache miss `all_data_files` is already computed inside
`ReadManifestEntriesWithCache`, and this block computes it again from the same
metas. Move this accumulate into the non-cache `else` branch so it is computed
exactly once per path, and drop the `!snapshot_cache_hit` condition.
--
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]
