laskoviymishka commented on code in PR #2970: URL: https://github.com/apache/iceberg-rust/pull/2970#discussion_r3756592225
########## crates/property-macro/README.md: ########## @@ -0,0 +1,264 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Iceberg property derive macro + +`Properties` parses a typed struct from a flat `HashMap<String, String>` and +can generate opt-in read-only getters. It deliberately does not generate +property-map serialization or implement `Default`, `Serialize`, `Deserialize`, +or any other trait. + +## Generated API + +For every annotated struct, `#[derive(Properties)]` generates this inherent +constructor: + +```text +impl MyProperties { + pub fn from_properties( + properties: &HashMap<String, String>, + ) -> iceberg::Result<Self>; +} +``` + +`from_properties` borrows the source map, parses every modeled property, and +uses its annotated default when a property is absent. Unknown keys are ignored. +An invalid value returns an `iceberg::Error` with `ErrorKind::DataInvalid` and a +message containing its primary property key. + +Adding `getter` to a field generates a public immutable accessor with the field +name. Primitive `Copy` types, references, pointers, and compositions of those +types return `T`; other types return `&T`. Because a procedural macro cannot +resolve trait implementations, a user-defined `Copy` type returns `&T`. +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. + +## Complete example + +This example covers exact keys and defaults, optional values, case-insensitive +booleans, prefixed maps, nested groups, custom single-value parsing, custom +multi-key parsing, lists of additional keys, read-only getters, ignored unknown +keys, and contextual errors. + +```rust +use std::collections::HashMap; + +use iceberg::{Error, ErrorKind}; +use iceberg_property_macro::Properties; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FANOUT: &str = "write.datafusion.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const LOCATION: &str = "write.data.path"; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_location(value: &str) -> iceberg::Result<String> { + let location = value.trim().trim_end_matches('/'); + if location.is_empty() { + Err(Error::new( + ErrorKind::DataInvalid, + "location must not be empty", + )) + } else { + Ok(location.to_string()) + } +} + +fn parse_dimensions( + properties: &HashMap<String, String>, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> iceberg::Result<(u64, u64, u64)> { + if additional_keys.len() != 2 { + return Err(Error::new( + ErrorKind::DataInvalid, + "dimensions require height and depth keys", + )); + } + let parse = |key: &str, default| { + properties + .get(key) + .map(|value| { + value.parse::<u64>().map_err(|error| { + Error::new(ErrorKind::DataInvalid, error.to_string()) + }) + }) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +#[derive(Debug, Properties)] +struct CommitProperties { + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, getter)] + retries: usize, +} + +#[derive(Debug, Properties)] +struct TableLikeProperties { + /// Nested groups parse from the same flat property map. + #[property(nested, getter)] + commit: CommitProperties, + + /// Option<T> distinguishes an absent property from a present value. + #[property(key = OWNER, default = None, getter)] + owner: Option<String>, + + /// This DataFusion-specific boolean is parsed case-insensitively. + /// Its `true` default is engine-specific, rather than an Iceberg-wide default. + #[property(key = FANOUT, default = true, getter)] + fanout_enabled: bool, + + /// A prefix captures suffix/value pairs into a typed map. + #[property(prefix = COLUMN_FPP_PREFIX, getter)] + column_fpp: HashMap<String, f64>, + + /// A single-key parser can validate and normalize a property value. + #[property( + key = LOCATION, + default = "warehouse", Review Comment: `write.data.path` doesn't actually default to `"warehouse"` — there's no fixed-string default for it; the data location derives from the table location (`<table-location>/data`) when the key is absent. The literal happens to match the in-memory catalog's warehouse constant, which makes it read like a real default, so someone copying this into an actual properties struct would silently write data files to the wrong path. Could we use a value that's obviously illustrative, or add a comment that the real absent case is computed from the table location? ########## crates/iceberg/src/lib.rs: ########## @@ -65,6 +65,7 @@ #[macro_use] extern crate derive_builder; extern crate core; +extern crate self as iceberg; Review Comment: Could we drop a one-line comment on this? Nothing inside the `iceberg` crate uses the alias — it's here purely so the `#[derive(Properties)]` output in other crates can resolve `::iceberg::Error`. Without a note, the next person doing dependency cleanup sees an unused self-alias, finds zero internal references, and deletes it, silently breaking the macro. Something like `// required so #[derive(Properties)] output can name ::iceberg::...; see crates/property-macro` would save that. ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,657 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter, +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + if option_name == "getter" { + return Ok(Self::Getter); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields + .iter() + .map(parse_field) + .collect::<syn::Result<Vec<_>>>()?; + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::iceberg::Result<Self> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if prefix.is_some() && default.is_some() { + return Err(Error::new_spanned( + field, + "prefix fields collect matching properties and cannot declare default in #[property(...)]", + )); + } + if key.is_some() && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties key fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if parse_properties_with.is_some() && additional_keys.is_none() { + return Err(Error::new_spanned( + field, + "parse_properties_with requires additional_keys in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> syn::Result<TokenStream2> { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return Ok(quote!(#ident: <#ty>::from_properties(properties)?)); + } + + let ty = &field.ty; + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().ok_or_else(|| { + Error::new_spanned( + &field.ident, + "parse_properties_with fields must declare key", + ) + })?; + let additional_keys = field.additional_keys.as_ref().ok_or_else(|| { + Error::new_spanned( + &field.ident, + "parse_properties_with fields must declare additional_keys", + ) + })?; + let default = typed_default(field)?; + return Ok(quote! { + #ident: { + let parsed: ::iceberg::Result<#ty> = #parse_properties_with( + properties, + #key, + &[#(#additional_keys),*], + #default, + ); + parsed.map_err(|error| error.with_context("property", #key))? + } + }); + } + + if let Some(prefix) = &field.prefix { + let value_type = field.map_value_type.as_ref().ok_or_else(|| { + Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + ) + })?; + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return Ok(quote! { + #ident: properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {key}: {error}"), + ) + }) + }) + }) + .collect::<::iceberg::Result<::std::collections::HashMap<_, _>>>()? + }); + } + + let key = field + .key + .as_ref() + .ok_or_else(|| Error::new_spanned(&field.ident, "property fields must declare key"))?; + let default = typed_default(field)?; + let parse = match (&field.parse_with, &field.option_inner_type) { + (Some(parse_with), Some(inner_type)) => quote! { + { + let parsed: ::iceberg::Result<#inner_type> = #parse_with(value); + Some(parsed.map_err(|error| error.with_context("property", #key))?) + } + }, + (Some(parse_with), None) => quote! { + { + let parsed: ::iceberg::Result<#ty> = #parse_with(value); + parsed.map_err(|error| error.with_context("property", #key))? + } + }, + (None, Some(inner_type)) if is_bool(inner_type) => quote! { + Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) + })?) + }, + (None, Some(inner_type)) => quote! { + Some(value.parse::<#inner_type>().map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) + })?) + }, + (None, None) if is_bool(ty) => quote! { + value.to_ascii_lowercase().parse::<#ty>().map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) + })? + }, + (None, None) => quote! { + value.parse::<#ty>().map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) + })? + }, + }; + + Ok(quote! { + #ident: match properties.get(#key) { + Some(value) => #parse, + None => #default, + } + }) +} + +fn typed_default(field: &PropertyField) -> syn::Result<TokenStream2> { + let ty = &field.ty; + let default = field.default.as_ref().ok_or_else(|| { + Error::new_spanned(&field.ident, "property key fields must declare default") + })?; + let default = default_value(default, ty); + Ok(quote!({ + let value: #ty = #default; + value + })) +} + +fn default_value(default: &Expr, ty: &Type) -> TokenStream2 { + if matches!( + default, + Expr::Lit(ExprLit { + lit: Lit::Str(_), + .. + }) | Expr::Path(_) + ) { + quote!(::std::convert::Into::<#ty>::into(#default)) + } else { + quote!(#default) + } +} + +fn option_inner_type(ty: &Type) -> Option<Type> { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "Option" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let Some(GenericArgument::Type(inner_type)) = arguments.args.first() else { + return None; + }; + + Some(inner_type.clone()) +} + +fn hash_map_value_type(ty: &Type) -> Option<Type> { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "HashMap" { Review Comment: Both this and `option_inner_type` just above match on the last path segment, so `hashbrown::HashMap<String, T>`, or a field typed through `use other_crate::Option`, passes the macro's own type check — then the generated code, which hardcodes `::std::collections::HashMap` and `Some(...)`, fails to compile deep inside the expansion rather than at the annotation. The compiler still catches it, so nothing misbehaves silently — it's just a confusing error location. Either a `std`-path guard here or a one-line "must be `std` `HashMap`/`Option`" note in the README would cover it. Not blocking, wdyt? ########## crates/property-macro/tests/properties.rs: ########## @@ -0,0 +1,304 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; +use std::str::FromStr; + +use iceberg::{Error, ErrorKind}; +use iceberg_property_macro::Properties; +use serde::{Deserialize, Serialize}; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.datafusion.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_dimensions( + properties: &HashMap<String, String>, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> iceberg::Result<(u64, u64, u64)> { + if additional_keys.len() != 2 { + return Err(Error::new( + ErrorKind::DataInvalid, + "dimensions require height and depth keys", + )); + } + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| { + value + .parse::<u64>() + .map_err(|error| Error::new(ErrorKind::DataInvalid, error.to_string())) + }) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +#[derive(Debug, Properties)] +struct TestProperties { + #[property(key = RETRIES, default = 4, getter)] + retries: u64, + + #[property(key = OWNER, default = None, getter)] + owner: Option<String>, + + #[property(key = FORMAT, default = "parquet", getter)] + format: String, + + #[property(key = FANOUT_ENABLED, default = true, getter)] + fanout_enabled: bool, + + #[property( + prefix = COLUMN_FPP_PREFIX, + getter + )] + column_fpp: HashMap<String, f64>, + + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + getter + )] + dimensions: (u64, u64, u64), +} + +#[test] +fn reads_defaults_through_generated_getters() { + let properties = TestProperties::from_properties(&HashMap::new()).unwrap(); + + assert_eq!(properties.retries(), 4); + assert_eq!(properties.owner(), &None); + assert_eq!(properties.format(), "parquet"); + assert!(properties.fanout_enabled()); + assert!(properties.column_fpp().is_empty()); + assert_eq!(properties.dimensions(), (640, 480, 320)); +} + +#[test] +fn reads_overrides_and_ignores_unknown_properties() { + let raw = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ("unknown".to_string(), "ignored".to_string()), + ]); + let properties = TestProperties::from_properties(&raw).unwrap(); + + assert_eq!(properties.retries(), 8); + assert_eq!(properties.owner().as_deref(), Some("iceberg")); + assert_eq!(properties.format(), "orc"); + assert!(!properties.fanout_enabled()); + assert_eq!(properties.column_fpp()["id"], 0.01); + assert_eq!(properties.dimensions(), (1920, 1080, 720)); +} + +#[test] +fn reports_the_property_with_an_invalid_value() { + let numeric_error = TestProperties::from_properties(&HashMap::from([( + RETRIES.to_string(), + "many".to_string(), + )])) + .unwrap_err(); + assert_eq!(numeric_error.kind(), ErrorKind::DataInvalid); + assert!(numeric_error.message().contains(RETRIES)); + + let boolean_error = TestProperties::from_properties(&HashMap::from([( + FANOUT_ENABLED.to_string(), + "sometimes".to_string(), + )])) + .unwrap_err(); + assert_eq!(boolean_error.kind(), ErrorKind::DataInvalid); + assert!(boolean_error.message().contains(FANOUT_ENABLED)); + + let prefixed_key = format!("{COLUMN_FPP_PREFIX}id"); + let prefix_error = TestProperties::from_properties(&HashMap::from([( + prefixed_key.clone(), + "low".to_string(), + )])) + .unwrap_err(); + assert_eq!(prefix_error.kind(), ErrorKind::DataInvalid); + assert!(prefix_error.message().contains(&prefixed_key)); + + let dimensions_error = + TestProperties::from_properties(&HashMap::from([(WIDTH.to_string(), "wide".to_string())])) + .unwrap_err(); + assert_eq!(dimensions_error.kind(), ErrorKind::DataInvalid); + assert!(format!("{dimensions_error}").contains(WIDTH)); +} + +#[derive(Debug, Properties)] +struct CommitProperties { + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, getter)] + retries: u64, +} + +#[derive(Debug, Properties)] +struct NestedProperties { + #[property(nested, getter)] + commit: CommitProperties, +} + +#[test] +fn nested_properties_read_the_same_flat_map() { + let raw = HashMap::from([(RETRIES.to_string(), "9".to_string())]); + let properties = NestedProperties::from_properties(&raw).unwrap(); + + assert_eq!(properties.commit().retries(), 9); +} + +fn parse_non_empty(value: &str) -> iceberg::Result<String> { + let value = value.trim(); + if value.is_empty() { + Err(Error::new( + ErrorKind::DataInvalid, + "value must not be empty", + )) + } else { + Ok(value.to_string()) + } +} + +#[derive(Debug, Properties)] +struct ValidatedProperties { + #[property( + key = "location", + default = "default", + parse_with = parse_non_empty, + getter + )] + location: String, +} + +#[test] +fn custom_single_value_parser_can_validate_and_normalize() { + let defaults = ValidatedProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(defaults.location(), "default"); + + let parsed = ValidatedProperties::from_properties(&HashMap::from([( + "location".to_string(), + " path ".to_string(), + )])) + .unwrap(); + assert_eq!(parsed.location(), "path"); + + let error = ValidatedProperties::from_properties(&HashMap::from([( + "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: location")); +} + +#[derive(Debug, Properties)] +struct OptionalValidatedProperties { + #[property( + key = "optional-location", + default = None, + parse_with = parse_non_empty, + getter + )] + location: Option<String>, +} + +#[test] +fn custom_single_value_parser_wraps_present_optional_values() { + let defaults = OptionalValidatedProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(defaults.location(), &None); + + let parsed = OptionalValidatedProperties::from_properties(&HashMap::from([( + "optional-location".to_string(), + " path ".to_string(), + )])) + .unwrap(); + assert_eq!(parsed.location().as_deref(), Some("path")); Review Comment: The `Option<T>` + `parse_with` fix from last round looks right, but this test only covers absent → `None` and present → `Some`. The error branch — a present value where `parse_non_empty` returns `Err` — never runs, so the `Some(parsed.map_err(...)?)` arm's `with_context` is unexercised. Could we add a case that sets `optional-location` to whitespace and asserts `DataInvalid` plus the key in the message? That closes the loop on the exact combination we flagged. -- 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]
