Copilot commented on code in PR #2037:
URL: https://github.com/apache/iceberg-rust/pull/2037#discussion_r2698321558
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -163,9 +163,9 @@ impl Transaction {
return Ok(self.table);
}
- let table_props = self.table.metadata().table_properties()?;
+ let table_props = self.table.metadata().table_properties();
- let backoff = Self::build_backoff(table_props)?;
+ let backoff = Self::build_backoff(table_props.clone())?;
Review Comment:
This clones the entire `TableProperties` struct (including the potentially
large `other` HashMap) just to pass it to `build_backoff()`. Since
`build_backoff()` only reads a few fields, passing a reference would be more
efficient.
```suggestion
let backoff = Self::build_backoff(&table_props)?;
```
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -68,6 +70,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:
The `new()` method silently swallows errors via `unwrap_or_default()`, which
could hide parsing failures for invalid property values. This differs from the
original behavior where `table_properties()` returned a `Result`. Consider
returning `Result<Self, anyhow::Error>` to preserve error handling
capabilities, or at minimum document that invalid values fall back to defaults.
```suggestion
pub fn new(props: HashMap<String, String>) -> Result<Self,
anyhow::Error> {
Self::try_from(&props)
```
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -187,6 +194,7 @@ impl TryFrom<&HashMap<String, String>> for TableProperties {
TableProperties::PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED,
TableProperties::PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED_DEFAULT,
)?,
+ other: props.clone(),
Review Comment:
The `other` field is set to a full clone of the input `props` HashMap, but
this includes all properties (even the ones that were already parsed into
struct fields). This creates unnecessary duplication and memory overhead.
Consider filtering out the explicitly parsed properties before storing in
`other`.
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -51,6 +51,8 @@ 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.
+ pub other: HashMap<String, String>,
}
impl TableProperties {
Review Comment:
Making the `other` field public exposes internal implementation details and
allows external code to directly modify properties, potentially causing
inconsistencies. Consider making this field private and providing accessor
methods if needed.
```suggestion
other: HashMap<String, String>,
}
impl TableProperties {
/// Returns a reference to the map of additional, non-standard table
properties.
pub fn other(&self) -> &HashMap<String, String> {
&self.other
}
/// Returns a mutable reference to the map of additional, non-standard
table properties.
pub fn other_mut(&mut self) -> &mut HashMap<String, String> {
&mut self.other
}
```
--
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]
