dannycjones commented on code in PR #2469:
URL: https://github.com/apache/iceberg-rust/pull/2469#discussion_r3383230824
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -123,7 +125,43 @@ pub struct TableProperties {
pub gc_enabled: bool,
}
+impl Default for TableProperties {
+ fn default() -> Self {
+ Self::try_from(&HashMap::new()).expect("default properties should
always be valid")
+ }
+}
+
impl TableProperties {
+ /// Get a property value by key.
+ pub fn get(&self, key: &str) -> Option<&String> {
+ self.raw.get(key)
+ }
+
+ /// Check if a property key exists.
+ pub fn contains_key(&self, key: &str) -> bool {
+ self.raw.contains_key(key)
+ }
+
+ /// Iterate over all raw properties.
+ pub fn iter(&self) -> impl Iterator<Item = (&String, &String)> {
+ self.raw.iter()
+ }
+
+ /// Return the number of raw properties.
+ pub fn len(&self) -> usize {
+ self.raw.len()
+ }
+
+ /// Return whether the raw properties map is empty.
+ pub fn is_empty(&self) -> bool {
+ self.raw.is_empty()
+ }
+
+ /// Return a reference to the raw properties map.
+ pub fn as_raw(&self) -> &HashMap<String, String> {
+ &self.raw
+ }
Review Comment:
My gut is to avoid leaking the dependency on `HashMap` after we've
constructed `TableProperties`, and instead expose it via the streaming method
and `get`, `contains_key`.
I see we do need it for serialization. I think we should add `pub(crate) fn
into_inner(self) -> HashMap<String, String>` for that use case.
--
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]
