wangyong9999 commented on code in PR #374:
URL: https://github.com/apache/paimon-cpp/pull/374#discussion_r4357244261
##########
src/paimon/core/manifest/snapshot_live_manifest_entries.cpp:
##########
@@ -34,7 +34,7 @@
namespace paimon {
namespace {
-constexpr int32_t kMagic = 0x534d4543; // SMEC
+constexpr int32_t kMagic = 0x534d4544; // SMED: includes per-snapshot total
file counts
Review Comment:
Thanks, this is correct: the old magic masked the intended corruption paths.
Fixed in ddcdf50a by using the current magic and writing total_data_files
before the entry count. The version, truncated-payload, and partition cases now
use the current cache layout. Full-tree pre-commit passed; focused runtime
validation is in progress.
##########
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:
Agreed. In ddcdf50a the accumulation is inside the non-cache branch. Cache
misses compute the count in ReadManifestEntriesWithCache, while exact hits
restore it from the cached snapshot. This removes the duplicate traversal
without changing the skipped-file metric.
##########
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:
Agreed after checking the callers. In ddcdf50a Put requires
total_data_files, tests supply it explicitly, and exact hits no longer check a
-1 sentinel. The cache magic still rejects the previous payload layout. This
changes an internal helper, not the public API.
##########
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:
Removed in ddcdf50a. The guide now describes observable cache behavior and
metrics; the transient-format compatibility explanation remains in the PR
description.
--
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]
