liurenjie1024 commented on code in PR #2037:
URL: https://github.com/apache/iceberg-rust/pull/2037#discussion_r2703312519
##########
crates/iceberg/src/spec/table_metadata.rs:
##########
@@ -358,16 +357,13 @@ impl TableMetadata {
/// Returns properties of table.
#[inline]
pub fn properties(&self) -> &HashMap<String, String> {
Review Comment:
I think we should remove this method.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -35,7 +35,7 @@ where
}
/// TableProperties that contains the properties of a table.
-#[derive(Debug)]
+#[derive(Debug, Clone, PartialEq, Eq, Default, serde::Serialize,
serde::Deserialize)]
Review Comment:
We have dedicated ser/de for `TableMetadata`, we should not add it here.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -110,7 +110,7 @@ impl TableMetadataBuilder {
), // Overwritten immediately by add_default_partition_spec
default_partition_type: StructType::new(vec![]),
last_partition_id: UNPARTITIONED_LAST_ASSIGNED_ID,
- properties: HashMap::new(),
Review Comment:
I think in `TableMetadataBuilder`, we should still use `HashMap<String,
String>` given it's a mutable api. We could add an extra field in
`TableMetadataBuilder`.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -68,6 +71,11 @@ impl TableProperties {
pub const PROPERTY_UUID: &str = "uuid";
/// Reserved table property for the total number of snapshots.
pub const PROPERTY_SNAPSHOT_COUNT: &str = "snapshot-count";
+ /// Creates a new TableProperties from a HashMap of raw strings.
+ pub fn new(props: HashMap<String, String>) -> Self {
+ Self::try_from(&props).unwrap_or_default()
Review Comment:
We should not unwrap, just implement `TryFrom`.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -51,6 +51,9 @@ pub struct TableProperties {
pub write_target_file_size_bytes: usize,
/// Whether to use `FanoutWriter` for partitioned tables.
pub write_datafusion_fanout_enabled: bool,
+ /// Any other properties that are not explicitly captured in named fields.
+ #[serde(flatten)]
+ pub additional_properties: HashMap<String, String>,
Review Comment:
I don't think we need this property.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
