feat: impl metrics config #488
Conversation
src/iceberg/metrics_config.h
Outdated
| kFull, | ||
| }; | ||
|
|
||
| static Result<std::shared_ptr<MetricsMode>> FromString(const std::string& mode); |
There was a problem hiding this comment.
| static Result<std::shared_ptr<MetricsMode>> FromString(const std::string& mode); | |
| static Result<std::shared_ptr<MetricsMode>> FromString(std::string_view mode); |
src/iceberg/metrics_config.h
Outdated
|
|
||
| static const std::shared_ptr<MetricsMode>& None(); | ||
| static const std::shared_ptr<MetricsMode>& Counts(); | ||
| static const std::shared_ptr<MetricsMode>& Truncate(); |
There was a problem hiding this comment.
Is it useful to keep this parameterless Truncate?
src/iceberg/metrics_config.h
Outdated
| std::unordered_map<std::string, std::shared_ptr<MetricsMode>> column_modes, | ||
| std::shared_ptr<MetricsMode> default_mode); |
There was a problem hiding this comment.
| std::unordered_map<std::string, std::shared_ptr<MetricsMode>> column_modes, | |
| std::shared_ptr<MetricsMode> default_mode); | |
| std::unordered_map<std::string, std::string>> properties); |
Should we use this as the function signature? I think MetricsConfig is created directly from the table property map.
There was a problem hiding this comment.
BTW, should we make it a private ctor and add a Make function to create one by validating all inputs?
There was a problem hiding this comment.
The constructor will not be called directly, defined as private
src/iceberg/metrics_config.h
Outdated
|
|
||
| namespace iceberg { | ||
|
|
||
| class ICEBERG_EXPORT MetricsMode : public util::Formattable { |
There was a problem hiding this comment.
Is it an overkill to use class hierarchy for it? Can we do this instead?
struct MetricsMode {
enum class Kind : uint8_t { kNone, ... };
Kind kind;
std::variant<std::monostate, int32_t> param;
};
src/iceberg/metrics_config.h
Outdated
| static const std::shared_ptr<MetricsConfig>& Default(); | ||
|
|
||
| /// \brief Creates a metrics config from a table. | ||
| static Result<std::shared_ptr<MetricsConfig>> Make(std::shared_ptr<Table> table); |
There was a problem hiding this comment.
| static Result<std::shared_ptr<MetricsConfig>> Make(std::shared_ptr<Table> table); | |
| static Result<std::unique_ptr<MetricsConfig>> Make(const Table& table); |
src/iceberg/metrics_config.cc
Outdated
| return MetricsMode::Full(); | ||
| } | ||
|
|
||
| if (mode.starts_with(kTruncatePrefix) && mode.ends_with(")")) { |
There was a problem hiding this comment.
It should also usse case insensitive comparison.
src/iceberg/metrics_config.cc
Outdated
| public: | ||
| explicit Visitor(int32_t limit) : limit_(limit) {} | ||
|
|
||
| Status Visit(const std::shared_ptr<Type>& type) { |
There was a problem hiding this comment.
Similarly, it looks weird to use const std::shared_ptr<Type>& and const NestedType& inconsistently. Can you fix this? You may find VisitTypeInline useful.
| if (!ShouldContinue()) { | ||
| break; | ||
| } | ||
| if (field.type()->is_primitive()) { |
There was a problem hiding this comment.
Add a TODO comment to support variant type in the future.
WZhuo
force-pushed
the
metric_config
branch
2 times, most recently
from
3877187 to
8a562d6
Compare
2 participants
No description provided.