lxy-9602 commented on code in PR #190:
URL: https://github.com/apache/paimon-cpp/pull/190#discussion_r3742538685
##########
src/paimon/core/operation/abstract_file_store_write.cpp:
##########
@@ -302,6 +302,10 @@ Result<std::shared_ptr<RestoreFiles>>
AbstractFileStoreWrite::ScanExistingFileMe
if (dv_maintainer_factory_) {
index_file_handler = dv_maintainer_factory_->GetIndexFileHandler();
}
+ // Paimon Java currently drops value stats during writer restore. This is
a known bug: a
+ // restored file can become a compact-after ADD via metadata-only level
upgrade and lose its
+ // stats (https://github.com/apache/paimon/issues/7026). C++ intentionally
does not align with
+ // that behavior; stats are dropped later only when the final entry kind
is DELETE.
Review Comment:
Nice catch! Could you please add a PK compaction end-to-end test in
`pk_compaction_inte_test.cpp` to demonstrate that level upgrades do not lose
their stats?
##########
src/paimon/core/operation/commit/manifest_entry_changes.h:
##########
@@ -39,7 +39,11 @@ namespace paimon {
/// Detailed changes from `CommitMessage`s.
class ManifestEntryChanges {
public:
- explicit ManifestEntryChanges(int32_t default_num_bucket);
+ /// Create a change collector.
+ ///
+ /// @param default_num_bucket Bucket count used when a commit message
omits it.
+ /// @param drop_delete_file_stats Whether DELETE data-file entries should
omit value stats.
+ explicit ManifestEntryChanges(int32_t default_num_bucket, bool
drop_delete_file_stats = false);
Review Comment:
Could we avoid using a default argument here? In production code, it’s
better to pass the parameter explicitly.
--
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]
