feat(spec): add table_properties.rs to spec
#1733
Conversation
There was a problem hiding this comment.
Thanks @kaushiksrini for this pr. Although this is quite interesting, I feel that this design is a little hard to maintain. What I expected is sth like following:
pub struct TableProperties {
#[key="commit.retry.num-retries"]
#[default="4"]
pub commit_num_retries: usize,
...
}
impl TryFrom<&HashMap<String, String>>> for TableProperties {
// parse by entry key or use default value
}Note we don't have a defult method for TableProperties, since some values don't have default methods. See https://github.com/apache/iceberg/blob/b15f4ac1795d25f9db5aa1559b677d57d1bacceb/core/src/main/java/org/apache/iceberg/TableProperties.java#L25 for reference.
|
Thanks for clarifying @liurenjie1024 ! From the description, I thought we would want to reuse the values set in I implemented them as declarative macros here, which don't require an extra crate, but agreed it's more to manage. The latest push uses static types to simplify things; happy to take on the procedural-macro implementation if that works best. |
| /// Property key for max number of partitions to keep summary stats for. | ||
| pub const PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT: &str = "write.summary.partition-limit"; | ||
| /// Default value for the max number of partitions to keep summary stats for. | ||
| pub const PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT_DEFAULT: u64 = 0; |
There was a problem hiding this comment.
Can we also move the properties above to the new module?
3 participants
Which issue does this PR close?
table_properties.rsto hold all properties #1505 .What changes are included in this PR?
table_properties.rsto hold and validate properties and set default values. Uses macros to simplify setting new properties.Are these changes tested?
Yes