From 58f6e2bf06402284372a63e5b668e91b3573aec4 Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:52:15 +0800 Subject: [PATCH 1/3] fix: Address property macro review follow-ups Address the final inline comments from #2970 by documenting generated-code requirements, correcting the illustrative data path default, and covering the optional custom-parser error path. --- crates/iceberg/src/lib.rs | 1 + crates/property-macro/README.md | 10 ++++++++-- crates/property-macro/tests/properties.rs | 9 +++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/lib.rs b/crates/iceberg/src/lib.rs index 43473cc35e..b3a409fa8c 100644 --- a/crates/iceberg/src/lib.rs +++ b/crates/iceberg/src/lib.rs @@ -65,6 +65,7 @@ #[macro_use] extern crate derive_builder; extern crate core; +// Required so `#[derive(Properties)]` output can name `::iceberg::Error`. extern crate self as iceberg; mod error; diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index e85e9194b0..7bcebb390d 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -50,6 +50,10 @@ Documentation attributes on the field are copied to the generated getter. The macro generates no setters, backing fields, or conversion back to a property map. +`Option` key fields and `HashMap` prefix fields must use the +corresponding types from `std`. The macro recognizes those field shapes +syntactically and generates code using the standard-library variants. + ## Complete example This example covers exact keys and defaults, optional values, case-insensitive @@ -144,7 +148,9 @@ struct TableLikeProperties { /// A single-key parser can validate and normalize a property value. #[property( key = LOCATION, - default = "warehouse", + // Iceberg computes the real absent case as `/data`. + // This literal is only an illustrative default for the macro example. + default = "/data", parse_with = parse_location, getter )] @@ -167,7 +173,7 @@ fn main() -> iceberg::Result<()> { assert_eq!(defaults.owner(), &None); assert!(defaults.fanout_enabled()); assert!(defaults.column_fpp().is_empty()); - assert_eq!(defaults.location(), "warehouse"); + assert_eq!(defaults.location(), "/data"); assert_eq!(defaults.dimensions(), (640, 480, 320)); let raw = HashMap::from([ diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 2ced3b7f73..1b8edff146 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -249,6 +249,15 @@ fn custom_single_value_parser_wraps_present_optional_values() { )])) .unwrap(); assert_eq!(parsed.location().as_deref(), Some("path")); + + let error = OptionalValidatedProperties::from_properties(&HashMap::from([( + "optional-location".to_string(), + " ".to_string(), + )])) + .unwrap_err(); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert_eq!(error.message(), "value must not be empty"); + assert!(format!("{error}").contains("property: optional-location")); } #[derive(Debug, Clone, Copy, PartialEq, Eq)] From 5b94532b7218e81103b0966daaf03694edfd95fb Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Wed, 12 Aug 2026 11:13:20 +0800 Subject: [PATCH 2/3] docs: correct data path fallback example --- crates/property-macro/README.md | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 7bcebb390d..84c9d982f2 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -145,16 +145,13 @@ struct TableLikeProperties { #[property(prefix = COLUMN_FPP_PREFIX, getter)] column_fpp: HashMap, - /// A single-key parser can validate and normalize a property value. + /// A single-key parser can validate and normalize an explicitly configured path. #[property( key = LOCATION, - // Iceberg computes the real absent case as `/data`. - // This literal is only an illustrative default for the macro example. - default = "/data", - parse_with = parse_location, - getter + default = None, + parse_with = parse_location )] - location: String, + location: Option, /// A full-map parser can model one field with multiple property keys. #[property( @@ -167,6 +164,14 @@ struct TableLikeProperties { dimensions: (u64, u64, u64), } +impl TableLikeProperties { + fn location(&self) -> &str { + self.location + .as_deref() + .unwrap_or("/data") + } +} + fn main() -> iceberg::Result<()> { let defaults = TableLikeProperties::from_properties(&HashMap::new())?; assert_eq!(defaults.commit().retries(), 4); From 70f6cfea7c285eac58cef960ab48daea0e58da5c Mon Sep 17 00:00:00 2001 From: Ray Liu <257669749+blackmwk@users.noreply.github.com> Date: Wed, 12 Aug 2026 11:37:21 +0800 Subject: [PATCH 3/3] docs: retain data path getter --- crates/property-macro/README.md | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 84c9d982f2..8ebf7054c7 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -145,11 +145,14 @@ struct TableLikeProperties { #[property(prefix = COLUMN_FPP_PREFIX, getter)] column_fpp: HashMap, - /// A single-key parser can validate and normalize an explicitly configured path. + /// An explicitly configured data path override. + /// `None` means callers should use `/data`. + /// A single-key parser validates and normalizes configured values. #[property( key = LOCATION, default = None, - parse_with = parse_location + parse_with = parse_location, + getter )] location: Option, @@ -164,21 +167,13 @@ struct TableLikeProperties { dimensions: (u64, u64, u64), } -impl TableLikeProperties { - fn location(&self) -> &str { - self.location - .as_deref() - .unwrap_or("/data") - } -} - fn main() -> iceberg::Result<()> { let defaults = TableLikeProperties::from_properties(&HashMap::new())?; assert_eq!(defaults.commit().retries(), 4); assert_eq!(defaults.owner(), &None); assert!(defaults.fanout_enabled()); assert!(defaults.column_fpp().is_empty()); - assert_eq!(defaults.location(), "/data"); + assert_eq!(defaults.location(), &None); assert_eq!(defaults.dimensions(), (640, 480, 320)); let raw = HashMap::from([ @@ -198,7 +193,10 @@ fn main() -> iceberg::Result<()> { assert_eq!(properties.owner().as_deref(), Some("iceberg")); assert!(!properties.fanout_enabled()); assert_eq!(properties.column_fpp()["id"], 0.01); - assert_eq!(properties.location(), "s3://bucket/table"); + assert_eq!( + properties.location().as_deref(), + Some("s3://bucket/table") + ); assert_eq!(properties.dimensions(), (1920, 1080, 720)); let error = TableLikeProperties::from_properties(&HashMap::from([(