lxy-9602 commented on code in PR #284:
URL: https://github.com/apache/paimon-cpp/pull/284#discussion_r3954068171
##########
src/paimon/common/io/cache/cache_key.h:
##########
@@ -26,6 +26,11 @@
namespace paimon {
+// Cache inferred candidates separately from explicit buckets and other bucket
layouts.
+std::shared_ptr<CacheKey> CreateInferredSnapshotLiveManifestEntriesCacheKey(
+ const std::string& table_path, const std::string& branch, int32_t bucket,
int32_t total_buckets,
+ int64_t schema_id);
+
Review Comment:
Please avoid introducing global static functions in Paimon.
##########
src/paimon/common/io/cache/cache_key.cpp:
##########
@@ -87,14 +86,14 @@ std::shared_ptr<CacheKey> CacheKey::ForKind(const
std::string& file_path, int64_
std::shared_ptr<CacheKey> CacheKey::ForSnapshotLiveManifestEntries(const
std::string& table_path,
const
std::string& branch,
int32_t
bucket) {
- return std::make_shared<SnapshotLiveManifestEntriesCacheKey>(table_path,
branch, bucket);
+ return std::make_shared<SnapshotLiveManifestEntriesCacheKey>(table_path,
branch, bucket,
+
/*total_buckets=*/0,
+
/*schema_id=*/0);
}
Review Comment:
Could we explicitly distinguish explicit and inferred cache keys instead of
using `0` as a placeholder? `schema_id = 0` is valid and looks like real
metadata.
For example, the internal key could provide `ForExplicit` and `ForInferred`
static factories, store an explicit mode, and use `std::optional` for
inferred-only fields. This avoids both placeholder metadata values and the
namespace-level factory function.
```cpp
class SnapshotLiveManifestEntriesCacheKey {
public:
static std::shared_ptr<CacheKey> ForExplicit(
const std::string& path, const std::string& branch, int32_t bucket);
static std::shared_ptr<CacheKey> ForInferred(
const std::string& path, const std::string& branch, int32_t bucket,
int32_t total_buckets, int64_t schema_id);
};
```
--
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]
