From 894637b20f51647dc6731a391f286281e4e2d286 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 14:24:12 +0800 Subject: [PATCH 01/11] Add derive properties macro --- Cargo.lock | 11 + Cargo.toml | 1 + crates/property-macro/Cargo.toml | 47 ++ crates/property-macro/README.md | 137 ++++ crates/property-macro/src/lib.rs | 48 ++ crates/property-macro/src/properties.rs | 789 ++++++++++++++++++++++ crates/property-macro/tests/properties.rs | 304 +++++++++ 7 files changed, 1337 insertions(+) create mode 100644 crates/property-macro/Cargo.toml create mode 100644 crates/property-macro/README.md create mode 100644 crates/property-macro/src/lib.rs create mode 100644 crates/property-macro/src/properties.rs create mode 100644 crates/property-macro/tests/properties.rs diff --git a/Cargo.lock b/Cargo.lock index 0f264cc1f5..d48f9beb79 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4007,6 +4007,17 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "iceberg-property-macro" +version = "0.10.0" +dependencies = [ + "proc-macro2", + "quote", + "serde", + "serde_json", + "syn", +] + [[package]] name = "iceberg-sqllogictest" version = "0.10.0" diff --git a/Cargo.toml b/Cargo.toml index a789ef1967..bcdd080347 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,6 +21,7 @@ members = [ "crates/catalog/*", "crates/examples", "crates/iceberg", + "crates/property-macro", "crates/integration_tests", "crates/integrations/*", "crates/sqllogictest", diff --git a/crates/property-macro/Cargo.toml b/crates/property-macro/Cargo.toml new file mode 100644 index 0000000000..075cf73f63 --- /dev/null +++ b/crates/property-macro/Cargo.toml @@ -0,0 +1,47 @@ +# 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. + +[package] +edition = { workspace = true } +homepage = { workspace = true } +name = "iceberg-property-macro" +publish = true +readme = "README.md" +rust-version = { workspace = true } +version = { workspace = true } + +license = { workspace = true } +repository = { workspace = true } + +categories = ["database"] +description = "Property derive macro for Apache Iceberg Rust" +keywords = ["iceberg"] + +[lib] +proc-macro = true + +[dependencies] +proc-macro2 = "1" +quote = "1" +syn = { version = "2", features = ["full"] } + +[dev-dependencies] +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } + +[lints] +workspace = true diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md new file mode 100644 index 0000000000..aab8a9c9c5 --- /dev/null +++ b/crates/property-macro/README.md @@ -0,0 +1,137 @@ + + +# Iceberg property derive macro + +`Properties` generates inherent methods for reading and writing a typed struct +from a flat `HashMap`. It deliberately does not implement +`Default`, `Serialize`, `Deserialize`, or any other trait. + +Leaf fields declare a property key and the default used when that key is absent. +Public accessors are opt-in: + +```rust +use iceberg_property_macro::Properties; + +#[derive(Default, Properties)] +struct WriteProperties { + #[property( + key = "commit.retry.num-retries", + default = 0, + pub(getter), + pub(setter) + )] + retries: u64, +} + +let mut properties = WriteProperties::default(); +properties.set_retries(4); +assert_eq!(*properties.retries(), 4); +``` + +The annotated property default is independent of the value produced by a +derived `Default` implementation. When both are used, keep them aligned. + +## Using a property map with Serde + +Serde's standard derives serialize a struct's fields and cannot infer the +property-map representation from `Properties` attributes. A transparent adapter +keeps that conversion explicit while allowing `Default`, `Serialize`, and +`Deserialize` to remain ordinary derives: + +```rust +use std::collections::HashMap; + +use iceberg_property_macro::Properties; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +#[derive(Debug, Default, PartialEq, Properties)] +struct WriteProperties { + #[property( + key = "commit.retry.num-retries", + default = 0, + pub(getter), + pub(setter) + )] + retries: u64, + + #[property(key = "owner", default = None)] + owner: Option, +} + +mod property_map { + use super::*; + + pub fn serialize(value: &WriteProperties, serializer: S) -> Result + where + S: Serializer, + { + let mut properties = HashMap::new(); + value.write_properties(&mut properties); + properties.serialize(serializer) + } + + pub fn deserialize<'de, D>(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let properties = HashMap::::deserialize(deserializer)?; + WriteProperties::from_properties(&properties).map_err(serde::de::Error::custom) + } +} + +#[derive(Debug, Default, Serialize, Deserialize)] +#[serde(transparent)] +struct PropertyDocument(#[serde(with = "property_map")] WriteProperties); + +fn main() -> Result<(), Box> { + let mut document = PropertyDocument::default(); + document.0.set_retries(4); + + let json = serde_json::to_string(&document)?; + assert_eq!(json, r#"{"commit.retry.num-retries":"4"}"#); + + let decoded: PropertyDocument = serde_json::from_str(&json)?; + assert_eq!(*decoded.0.retries(), 4); + Ok(()) +} +``` + +Property options may be grouped under `#[property(...)]`, which avoids a +collision between the standalone `#[default(...)]` helper and Rust's `Default` +derive. The standalone annotations from the original framework remain +supported. + +`#[prefix(...)]` captures a family of properties in a `HashMap`, +keyed by the suffix after the prefix. `#[nested]` embeds another `Properties` +struct while keeping the property map flat. `#[parse_with(...)]` and +`#[serialize_with(...)]` customize conversion for one exact-key field. The +latter name refers to conversion into a property string and does not require +Serde. + +`#[parse_properties_with(...)]` and `#[write_properties_with(...)]` receive the +complete property map for fields represented by more than one key. +`#[additional_key(...)]` supplies a second key to those hooks. Custom write +hooks receive the field default and are responsible for removing or omitting +default-valued properties. + +Boolean property values are parsed case-insensitively. Other values require +`FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf +fields require `PartialEq` so default values can be omitted. String-literal and +path defaults are converted into their field type with `Into`. diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs new file mode 100644 index 0000000000..018a7c1f04 --- /dev/null +++ b/crates/property-macro/src/lib.rs @@ -0,0 +1,48 @@ +// 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. + +#![doc = include_str!("../README.md")] + +use proc_macro::TokenStream; +use syn::{DeriveInput, parse_macro_input}; + +mod properties; + +/// Derives property-map parsing, writing, and opt-in accessors for a struct. +#[proc_macro_derive( + Properties, + attributes( + key, + additional_key, + prefix, + nested, + default, + parse_with, + serialize_with, + parse_properties_with, + write_properties_with, + property + ) +)] +pub fn derive_properties(input: TokenStream) -> TokenStream { + let input = parse_macro_input!(input as DeriveInput); + + match properties::expand_properties(input) { + Ok(tokens) => tokens.into(), + Err(error) => error.into_compile_error().into(), + } +} diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs new file mode 100644 index 0000000000..564a52bf49 --- /dev/null +++ b/crates/property-macro/src/properties.rs @@ -0,0 +1,789 @@ +// 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::{format_ident, quote}; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Meta, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option, + additional_key: Option, + prefix: Option, + nested: bool, + default: Option, + parse_with: Option, + serialize_with: Option, + parse_properties_with: Option, + write_properties_with: Option, + option_inner_type: Option, + map_value_type: Option, + public_getter: bool, + public_setter: bool, + doc_attributes: Vec, +} + +enum PublicAccessor { + Getter, + Setter, +} + +enum PropertyOption { + Key(Expr), + AdditionalKey(Expr), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + SerializeWith(Path), + ParsePropertiesWith(Path), + WritePropertiesWith(Path), + Accessor(PublicAccessor), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option, + additional_key: Option, + prefix: Option, + nested: bool, + default: Option, + parse_with: Option, + serialize_with: Option, + parse_properties_with: Option, + write_properties_with: Option, + public_getter: bool, + public_setter: bool, +} + +impl Parse for PublicAccessor { + fn parse(input: ParseStream<'_>) -> syn::Result { + input.parse::()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::()?; + if !content.is_empty() { + return Err(content.error("expected getter or setter")); + } + + match accessor.to_string().as_str() { + "getter" => Ok(Self::Getter), + "setter" => Ok(Self::Setter), + _ => Err(Error::new_spanned(accessor, "expected getter or setter")), + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result { + if input.peek(Token![pub]) { + return input.parse().map(Self::Accessor); + } + + let name = input.parse::()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::()?; + let expression = input.parse::()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_key" => Ok(Self::AdditionalKey(expression)), + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "serialize_with" => { + expression_path(expression, "serialize_with").map(Self::SerializeWith) + } + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + "write_properties_with" => { + expression_path(expression, "write_properties_with").map(Self::WritePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result { + 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(parse_property_field) + .collect::>>()?; + let parses = fields.iter().map(parse_field); + let property_writes = fields.iter().map(write_field); + let accessors = fields.iter().map(field_accessors); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub(crate) fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result { + Ok(Self { + #(#parses,)* + }) + } + + pub(crate) fn write_properties( + &self, + properties: &mut ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) { + #(#property_writes)* + } + } + }) +} + +fn parse_property_field(field: &Field) -> syn::Result { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let property_options = property_options(&field.attrs)?; + let key = merge_attribute_option( + attribute_expression_value(&field.attrs, "key")?, + property_options.key, + field, + "key", + )?; + let additional_key = merge_attribute_option( + attribute_expression_value(&field.attrs, "additional_key")?, + property_options.additional_key, + field, + "additional_key", + )?; + let prefix = merge_attribute_option( + attribute_expression_value(&field.attrs, "prefix")?, + property_options.prefix, + field, + "prefix", + )?; + let standalone_nested = marker_attribute(&field.attrs, "nested")?; + if standalone_nested && property_options.nested { + return Err(Error::new_spanned( + field, + "duplicate nested property option", + )); + } + let nested = standalone_nested || property_options.nested; + + 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]", + )); + } + + let default = merge_attribute_option( + attribute_expression_value(&field.attrs, "default")?, + property_options.default, + field, + "default", + )?; + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "#[nested] fields obtain defaults from their own property annotations and cannot declare #[default(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare #[default(...)]", + )); + } + + 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, + "#[prefix(...)] fields must have type HashMap", + )); + } + + let parse_with = merge_attribute_option( + attribute_path_value(&field.attrs, "parse_with")?, + property_options.parse_with, + field, + "parse_with", + )?; + let serialize_with = merge_attribute_option( + attribute_path_value(&field.attrs, "serialize_with")?, + property_options.serialize_with, + field, + "serialize_with", + )?; + let parse_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "parse_properties_with")?, + property_options.parse_properties_with, + field, + "parse_properties_with", + )?; + let write_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "write_properties_with")?, + property_options.write_properties_with, + field, + "write_properties_with", + )?; + + if additional_key.is_some() + && parse_properties_with.is_none() + && write_properties_with.is_none() + { + return Err(Error::new_spanned( + field, + "#[additional_key(...)] requires parse_properties_with or write_properties_with", + )); + } + if (prefix.is_some() || nested) + && (additional_key.is_some() + || parse_with.is_some() + || serialize_with.is_some() + || parse_properties_with.is_some() + || write_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "#[prefix(...)] and #[nested] fields do not support custom parse or write 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", + )); + } + if serialize_with.is_some() && write_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both serialize_with and write_properties_with", + )); + } + + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_key, + prefix, + nested, + default, + parse_with, + serialize_with, + parse_properties_with, + write_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter: property_options.public_getter, + public_setter: property_options.public_setter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(attributes: &[Attribute]) -> syn::Result { + let Some(attribute) = find_attribute(attributes, "property")? else { + return Ok(PropertyOptions::default()); + }; + + let parsed = + attribute.parse_args_with(Punctuated::::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::AdditionalKey(value) => set_property_option( + &mut options.additional_key, + value, + attribute, + "additional_key", + )?, + 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::SerializeWith(value) => set_property_option( + &mut options.serialize_with, + value, + attribute, + "serialize_with", + )?, + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::WritePropertiesWith(value) => set_property_option( + &mut options.write_properties_with, + value, + attribute, + "write_properties_with", + )?, + PropertyOption::Accessor(accessor) => { + let selected = match accessor { + PublicAccessor::Getter => &mut options.public_getter, + PublicAccessor::Setter => &mut options.public_setter, + }; + if *selected { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + *selected = true; + } + } + } + + Ok(options) +} + +fn set_property_option( + target: &mut Option, + 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 merge_attribute_option( + standalone: Option, + grouped: Option, + field: &Field, + name: &str, +) -> syn::Result> { + match (standalone, grouped) { + (Some(_), Some(_)) => Err(Error::new_spanned( + field, + format!("duplicate {name} property option"), + )), + (Some(value), None) | (None, Some(value)) => Ok(Some(value)), + (None, None) => Ok(None), + } +} + +fn field_accessors(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + let getter = field.public_getter.then(|| { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + }); + let setter = field.public_setter.then(|| { + let setter_ident = format_ident!("set_{}", ident); + let setter_doc = format!("Sets `{ident}`."); + quote! { + #[doc = #setter_doc] + pub fn #setter_ident(&mut self, value: #ty) { + self.#ident = value; + } + } + }); + + quote! { + #getter + #setter + } +} + +fn marker_attribute(attributes: &[Attribute], name: &str) -> syn::Result { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(false); + }; + + match &attribute.meta { + Meta::Path(_) => Ok(true), + _ => Err(Error::new_spanned( + attribute, + format!("{name} must use the form #[{name}]"), + )), + } +} + +fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Result> { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(None); + }; + + match &attribute.meta { + Meta::NameValue(name_value) => Ok(Some(name_value.value.clone())), + Meta::List(_) => attribute.parse_args::().map(Some), + _ => Err(Error::new_spanned( + attribute, + format!("{name} must use the form #[{name}(...)]"), + )), + } +} + +fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { + let Some(expression) = attribute_expression_value(attributes, name)? else { + return Ok(None); + }; + + expression_path(expression, name).map(Some) +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result> { + 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) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_key { + Some(additional_key) => { + quote!(#parse_properties_with(properties, #key, #additional_key, #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), + }; + return quote! { + #ident: #parse.map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }; + } + + if let Some(prefix) = &field.prefix { + let value_type = field + .map_value_type + .as_ref() + .expect("prefix fields are validated as maps"); + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return quote! { + #ident: { + let parsed = properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| format!("Invalid value for {key}: {error}")) + }) + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()?; + if parsed.is_empty() { + #default + } else { + parsed + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match (&field.parse_with, &field.option_inner_type) { + (Some(parse_with), _) => quote! { + #parse_with(value).map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, Some(inner_type)) if is_bool(inner_type) => quote! { + Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, Some(inner_type)) => quote! { + Some(value.parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, None) if is_bool(ty) => quote! { + value.to_ascii_lowercase().parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, None) => quote! { + value.parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + }; + + quote! { + #ident: match properties.get(#key) { + Some(value) => #parse, + None => #default, + } + } +} + +fn typed_default(field: &PropertyField) -> TokenStream2 { + let ty = &field.ty; + let default = default_value( + field.default.as_ref().expect("leaf fields have defaults"), + ty, + ); + 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 { + 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 { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "HashMap" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let mut arguments = arguments.args.iter(); + let Some(GenericArgument::Type(key_type)) = arguments.next() else { + return None; + }; + let Some(GenericArgument::Type(value_type)) = arguments.next() else { + return None; + }; + if !is_named_type(key_type, "String") { + return None; + } + + Some(value_type.clone()) +} + +fn is_bool(ty: &Type) -> bool { + is_named_type(ty, "bool") +} + +fn is_named_type(ty: &Type, name: &str) -> bool { + let Type::Path(type_path) = ty else { + return false; + }; + + type_path + .path + .segments + .last() + .is_some_and(|segment| segment.ident == name) +} + +fn write_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + return quote! { + self.#ident.write_properties(properties); + }; + } + + let default = typed_default(field); + + if let Some(write_properties_with) = &field.write_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let write = match &field.additional_key { + Some(additional_key) => { + quote!(#write_properties_with(&self.#ident, properties, #key, #additional_key, &#default)) + } + None => quote!(#write_properties_with(&self.#ident, properties, #key, &#default)), + }; + return quote! { + #write; + }; + } + + if let Some(prefix) = &field.prefix { + return quote! { + properties.retain(|key, _| !key.starts_with(#prefix)); + if self.#ident != #default { + for (suffix, value) in &self.#ident { + let key = format!("{}{}", #prefix, suffix); + properties.insert(key, ::std::string::ToString::to_string(value)); + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let value = match (&field.serialize_with, &field.option_inner_type) { + (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident)), + (None, Some(_)) => quote!(::std::string::ToString::to_string( + self.#ident.as_ref().expect("checked is_some above") + )), + (None, None) => quote!(::std::string::ToString::to_string(&self.#ident)), + }; + let insert = if field.option_inner_type.is_some() { + quote! { + if self.#ident != #default && self.#ident.is_some() { + properties.insert((#key).to_string(), #value); + } + } + } else { + quote! { + if self.#ident != #default { + properties.insert((#key).to_string(), #value); + } + } + }; + + quote! { + properties.remove(#key); + #insert + } +} diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs new file mode 100644 index 0000000000..ae33198a74 --- /dev/null +++ b/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 iceberg_property_macro::Properties; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; + +fn parse_dimensions( + properties: &HashMap, + width_key: &str, + height_key: &str, + default: (u64, u64), +) -> Result<(u64, u64), String> { + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| value.parse::().map_err(|error| error.to_string())) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok((parse(width_key, default.0)?, parse(height_key, default.1)?)) +} + +fn write_dimensions( + dimensions: &(u64, u64), + properties: &mut HashMap, + width_key: &str, + height_key: &str, + default: &(u64, u64), +) { + properties.remove(width_key); + properties.remove(height_key); + if dimensions != default { + properties.insert(width_key.to_string(), dimensions.0.to_string()); + properties.insert(height_key.to_string(), dimensions.1.to_string()); + } +} + +#[derive(Debug, Properties)] +struct TestProperties { + #[key(RETRIES)] + #[default(4)] + pub retries: u64, + + #[key(OWNER)] + #[default(None)] + pub owner: Option, + + #[key(FORMAT)] + #[default("parquet")] + pub format: String, + + #[key(FANOUT_ENABLED)] + #[default(true)] + pub fanout_enabled: bool, + + #[prefix(COLUMN_FPP_PREFIX)] + #[default(HashMap::new())] + pub column_fpp: HashMap, + + #[key(WIDTH)] + #[additional_key(HEIGHT)] + #[default((640, 480))] + #[parse_properties_with(parse_dimensions)] + #[write_properties_with(write_dimensions)] + pub dimensions: (u64, u64), +} + +#[test] +fn reads_defaults_and_overrides() { + let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(defaults.retries, 4); + assert_eq!(defaults.owner, None); + assert_eq!(defaults.format, "parquet"); + assert!(defaults.fanout_enabled); + assert!(defaults.column_fpp.is_empty()); + assert_eq!(defaults.dimensions, (640, 480)); + + let properties = 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()), + ]); + let parsed = TestProperties::from_properties(&properties).unwrap(); + + assert_eq!(parsed.retries, 8); + assert_eq!(parsed.owner.as_deref(), Some("iceberg")); + assert_eq!(parsed.format, "orc"); + assert!(!parsed.fanout_enabled); + assert_eq!(parsed.column_fpp["id"], 0.01); + assert_eq!(parsed.dimensions, (1920, 1080)); +} + +#[test] +fn writes_overrides_and_preserves_unrelated_properties() { + let parsed = TestProperties::from_properties(&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()), + ])) + .unwrap(); + let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); + + parsed.write_properties(&mut properties); + + assert_eq!(properties[RETRIES], "8"); + assert_eq!(properties[OWNER], "iceberg"); + assert_eq!(properties[FORMAT], "orc"); + assert_eq!(properties[FANOUT_ENABLED], "false"); + assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); + assert_eq!(properties[WIDTH], "1920"); + assert_eq!(properties[HEIGHT], "1080"); + assert_eq!(properties["unrelated"], "value"); +} + +#[test] +fn writing_defaults_removes_modeled_properties() { + let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); + let mut properties = 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()), + ("unrelated".to_string(), "value".to_string()), + ]); + + defaults.write_properties(&mut properties); + + assert_eq!( + properties, + HashMap::from([("unrelated".to_string(), "value".to_string())]) + ); +} + +#[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!(numeric_error.contains(RETRIES)); + + let boolean_error = TestProperties::from_properties(&HashMap::from([( + FANOUT_ENABLED.to_string(), + "sometimes".to_string(), + )])) + .unwrap_err(); + assert!(boolean_error.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!(prefix_error.contains(&prefixed_key)); +} + +#[derive(Clone, Debug, Properties)] +struct CommitProperties { + #[key = "commit.retry.num-retries"] + #[default = 4] + pub num_retries: u64, +} + +#[derive(Debug, Properties)] +struct NestedProperties { + #[nested] + pub commit: CommitProperties, +} + +#[test] +fn nested_properties_use_a_flat_property_map() { + let mut properties = NestedProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(properties.commit.num_retries, 4); + + properties.commit.num_retries = 9; + let mut written = HashMap::new(); + properties.write_properties(&mut written); + assert_eq!( + written, + HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) + ); + + let decoded = NestedProperties::from_properties(&written).unwrap(); + assert_eq!(decoded.commit.num_retries, 9); +} + +fn parse_non_empty(value: &str) -> Result { + let value = value.trim(); + if value.is_empty() { + Err("value must not be empty") + } else { + Ok(value.to_string()) + } +} + +fn serialize_trimmed(value: &str) -> String { + value.trim().to_string() +} + +#[derive(Debug, Properties)] +struct ValidatedProperties { + #[key = "location"] + #[default = "default"] + #[parse_with(parse_non_empty)] + #[serialize_with(serialize_trimmed)] + location: String, +} + +#[test] +fn custom_single_value_hooks_can_validate_and_normalize() { + 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, "Invalid value for location: value must not be empty"); + + let properties = ValidatedProperties { + location: " normalized ".to_string(), + }; + let mut written = HashMap::new(); + properties.write_properties(&mut written); + assert_eq!(written["location"], "normalized"); +} + +mod accessor_fixture { + use iceberg_property_macro::Properties; + + #[derive(Debug, Default, Properties)] + pub struct AccessorProperties { + #[doc = "A property with public read and write access."] + #[property(key = "public.both", default = 0, pub(getter), pub(setter))] + both: u64, + + #[property(key = "public.getter", default = "", pub(getter))] + getter_only: String, + + #[property(key = "public.setter", default = false, pub(setter))] + setter_only: bool, + } + + impl AccessorProperties { + pub fn setter_only_for_test(&self) -> bool { + self.setter_only + } + } +} + +#[test] +fn coexists_with_derived_default_and_generates_opt_in_accessors() { + let mut properties = accessor_fixture::AccessorProperties::default(); + + assert_eq!(*properties.both(), 0); + properties.set_both(2); + assert_eq!(*properties.both(), 2); + assert_eq!(properties.getter_only(), ""); + + properties.set_setter_only(true); + assert!(properties.setter_only_for_test()); +} From a1fd056965d0c9fd41ad6e481668a6df94db060c Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 15:38:15 +0800 Subject: [PATCH 02/11] Ready --- crates/property-macro/README.md | 235 ++++++++++++++++++++-- crates/property-macro/src/lib.rs | 4 +- crates/property-macro/src/properties.rs | 147 +++++++++----- crates/property-macro/tests/properties.rs | 96 ++++++--- 4 files changed, 395 insertions(+), 87 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index aab8a9c9c5..4ab30c126f 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -23,26 +23,229 @@ from a flat `HashMap`. It deliberately does not implement `Default`, `Serialize`, `Deserialize`, or any other trait. -Leaf fields declare a property key and the default used when that key is absent. -Public accessors are opt-in: +## Generated methods + +For every annotated struct, `#[derive(Properties)]` generates these inherent +methods: + +```text +impl MyProperties { + pub fn from_properties( + properties: &HashMap, + ) -> Result; + + pub fn write_properties( + &self, + properties: &mut HashMap, + ) -> Result<(), String>; +} +``` + +`from_properties` parses every modeled property, uses its annotated default +when absent, and returns an error containing the primary property key when a +value is invalid. Unknown keys are ignored. + +`write_properties` updates an existing map. It removes modeled keys whose +values equal their annotated defaults, writes non-default values as strings, +and preserves unknown keys. It returns an error when a custom +`serialize_with` or `serialize_properties_with` hook fails. + +## Complete example + +This example exercises the complete generated API: exact keys and defaults, +optional values, case-insensitive booleans, prefixed maps, nested property +groups, custom single-value conversion, custom multi-key conversion, public +accessors, contextual errors, and writing into an existing property map. ```rust +use std::collections::HashMap; + use iceberg_property_macro::Properties; -#[derive(Default, Properties)] -struct WriteProperties { +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FANOUT: &str = "write.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) -> Result { + let location = value.trim().trim_end_matches('/'); + if location.is_empty() { + Err("location must not be empty") + } else { + Ok(location.to_string()) + } +} + +fn serialize_location(value: &str) -> Result { + let location = value.trim().trim_end_matches('/'); + if location.is_empty() { + Err("location must not be empty") + } else { + Ok(location.to_string()) + } +} + +fn parse_dimensions( + properties: &HashMap, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + let parse = |key: &str, default| { + properties + .get(key) + .map(|value| value.parse::().map_err(|error| 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)?, + )) +} + +fn serialize_dimensions( + dimensions: &(u64, u64, u64), + properties: &mut HashMap, + width_key: &str, + additional_keys: &[&str], + default: &(u64, u64, u64), +) -> Result<(), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { + return Err("dimensions must be positive".to_string()); + } + properties.remove(width_key); + properties.remove(additional_keys[0]); + properties.remove(additional_keys[1]); + if dimensions != default { + properties.insert(width_key.to_string(), dimensions.0.to_string()); + properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); + } + Ok(()) +} + +#[derive(Debug, Properties)] +struct CommitProperties { #[property( - key = "commit.retry.num-retries", - default = 0, + key = RETRIES, + default = 4, pub(getter), pub(setter) )] - retries: u64, + retries: usize, } -let mut properties = WriteProperties::default(); -properties.set_retries(4); -assert_eq!(*properties.retries(), 4); +#[derive(Debug, Properties)] +struct TableLikeProperties { + // Nested groups still read and write the same flat property map. + #[property(nested)] + commit: CommitProperties, + + // Option distinguishes an absent property from a present value. + #[property(key = OWNER, default = None, pub(getter), pub(setter))] + owner: Option, + + // Boolean values are parsed case-insensitively. + #[property(key = FANOUT, default = true, pub(getter))] + fanout_enabled: bool, + + // A prefix captures suffix/value pairs into a typed map. + #[property(prefix = COLUMN_FPP_PREFIX, default = HashMap::new(), pub(getter))] + column_fpp: HashMap, + + // Single-key hooks provide validation and custom string conversion. + #[property( + key = LOCATION, + default = "warehouse", + parse_with = parse_location, + serialize_with = serialize_location, + pub(getter), + pub(setter) + )] + location: String, + + // Full-map hooks can model one field with multiple property keys. + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + serialize_properties_with = serialize_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), +} + +fn main() -> Result<(), String> { + // An empty map uses every annotated property default. + 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(), "warehouse"); + assert_eq!(defaults.dimensions(), &(640, 480, 320)); + + let mut raw = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FANOUT.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (LOCATION.to_string(), " s3://bucket/table/ ".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ("unmodeled".to_string(), "preserved".to_string()), + ]); + + let mut properties = TableLikeProperties::from_properties(&raw)?; + assert_eq!(*properties.commit.retries(), 8); + 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.dimensions(), &(1920, 1080, 720)); + + // Generated setters modify private fields. Writing removes modeled values + // reset to their defaults and preserves properties the struct does not own. + properties.commit.set_retries(10); + properties.set_owner(None); + properties.set_location("s3://bucket/new-table/".to_string()); + properties.write_properties(&mut raw)?; + + assert_eq!(raw[RETRIES], "10"); + assert!(!raw.contains_key(OWNER)); + assert_eq!(raw[FANOUT], "false"); + assert_eq!(raw[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); + assert_eq!(raw[LOCATION], "s3://bucket/new-table"); + assert_eq!(raw[WIDTH], "1920"); + assert_eq!(raw[HEIGHT], "1080"); + assert_eq!(raw[DEPTH], "720"); + assert_eq!(raw["unmodeled"], "preserved"); + + // Parsing errors identify the primary property key. + let error = TableLikeProperties::from_properties(&HashMap::from([( + LOCATION.to_string(), + "/".to_string(), + )])) + .unwrap_err(); + assert!(error.contains(LOCATION)); + + Ok(()) +} ``` The annotated property default is independent of the value produced by a @@ -83,7 +286,9 @@ mod property_map { S: Serializer, { let mut properties = HashMap::new(); - value.write_properties(&mut properties); + value + .write_properties(&mut properties) + .map_err(serde::ser::Error::custom)?; properties.serialize(serializer) } @@ -125,11 +330,11 @@ struct while keeping the property map flat. `#[parse_with(...)]` and latter name refers to conversion into a property string and does not require Serde. -`#[parse_properties_with(...)]` and `#[write_properties_with(...)]` receive the +`#[parse_properties_with(...)]` and `#[serialize_properties_with(...)]` receive the complete property map for fields represented by more than one key. -`#[additional_key(...)]` supplies a second key to those hooks. Custom write -hooks receive the field default and are responsible for removing or omitting -default-valued properties. +`#[additional_keys(...)]` supplies a list of secondary keys to those hooks. +Custom serialization hooks return `Result`, receive the field default, and are +responsible for removing or omitting default-valued properties. Boolean property values are parsed case-insensitively. Other values require `FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 018a7c1f04..11b1746d76 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -27,14 +27,14 @@ mod properties; Properties, attributes( key, - additional_key, + additional_keys, prefix, nested, default, parse_with, serialize_with, parse_properties_with, - write_properties_with, + serialize_properties_with, property ) )] diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 564a52bf49..f808195c39 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -28,14 +28,14 @@ struct PropertyField { ident: Ident, ty: Type, key: Option, - additional_key: Option, + additional_keys: Option>, prefix: Option, nested: bool, default: Option, parse_with: Option, serialize_with: Option, parse_properties_with: Option, - write_properties_with: Option, + serialize_properties_with: Option, option_inner_type: Option, map_value_type: Option, public_getter: bool, @@ -50,28 +50,28 @@ enum PublicAccessor { enum PropertyOption { Key(Expr), - AdditionalKey(Expr), + AdditionalKeys(Vec), Prefix(Expr), Nested, Default(Expr), ParseWith(Path), SerializeWith(Path), ParsePropertiesWith(Path), - WritePropertiesWith(Path), + SerializePropertiesWith(Path), Accessor(PublicAccessor), } #[derive(Default)] struct PropertyOptions { key: Option, - additional_key: Option, + additional_keys: Option>, prefix: Option, nested: bool, default: Option, parse_with: Option, serialize_with: Option, parse_properties_with: Option, - write_properties_with: Option, + serialize_properties_with: Option, public_getter: bool, public_setter: bool, } @@ -110,7 +110,9 @@ impl Parse for PropertyOption { let expression = input.parse::()?; match option_name.as_str() { "key" => Ok(Self::Key(expression)), - "additional_key" => Ok(Self::AdditionalKey(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), @@ -120,9 +122,8 @@ impl Parse for PropertyOption { "parse_properties_with" => { expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) } - "write_properties_with" => { - expression_path(expression, "write_properties_with").map(Self::WritePropertiesWith) - } + "serialize_properties_with" => expression_path(expression, "serialize_properties_with") + .map(Self::SerializePropertiesWith), _ => Err(Error::new_spanned(name, "unknown property option")), } } @@ -162,7 +163,7 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result impl #impl_generics #struct_name #type_generics #where_clause { #(#accessors)* - pub(crate) fn from_properties( + pub fn from_properties( properties: &::std::collections::HashMap< ::std::string::String, ::std::string::String, @@ -173,14 +174,15 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result }) } - pub(crate) fn write_properties( + pub fn write_properties( &self, properties: &mut ::std::collections::HashMap< ::std::string::String, ::std::string::String, >, - ) { + ) -> ::std::result::Result<(), ::std::string::String> { #(#property_writes)* + Ok(()) } } }) @@ -198,11 +200,11 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "key", )?; - let additional_key = merge_attribute_option( - attribute_expression_value(&field.attrs, "additional_key")?, - property_options.additional_key, + let additional_keys = merge_attribute_option( + attribute_expression_list(&field.attrs, "additional_keys")?, + property_options.additional_keys, field, - "additional_key", + "additional_keys", )?; let prefix = merge_attribute_option( attribute_expression_value(&field.attrs, "prefix")?, @@ -271,28 +273,28 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "parse_properties_with", )?; - let write_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "write_properties_with")?, - property_options.write_properties_with, + let serialize_properties_with = merge_attribute_option( + attribute_path_value(&field.attrs, "serialize_properties_with")?, + property_options.serialize_properties_with, field, - "write_properties_with", + "serialize_properties_with", )?; - if additional_key.is_some() + if additional_keys.is_some() && parse_properties_with.is_none() - && write_properties_with.is_none() + && serialize_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_key(...)] requires parse_properties_with or write_properties_with", + "#[additional_keys(...)] requires parse_properties_with or serialize_properties_with", )); } if (prefix.is_some() || nested) - && (additional_key.is_some() + && (additional_keys.is_some() || parse_with.is_some() || serialize_with.is_some() || parse_properties_with.is_some() - || write_properties_with.is_some()) + || serialize_properties_with.is_some()) { return Err(Error::new_spanned( field, @@ -305,10 +307,10 @@ fn parse_property_field(field: &Field) -> syn::Result { "fields cannot declare both parse_with and parse_properties_with", )); } - if serialize_with.is_some() && write_properties_with.is_some() { + if serialize_with.is_some() && serialize_properties_with.is_some() { return Err(Error::new_spanned( field, - "fields cannot declare both serialize_with and write_properties_with", + "fields cannot declare both serialize_with and serialize_properties_with", )); } @@ -316,14 +318,14 @@ fn parse_property_field(field: &Field) -> syn::Result { ident, ty: field.ty.clone(), key, - additional_key, + additional_keys, prefix, nested, default, parse_with, serialize_with, parse_properties_with, - write_properties_with, + serialize_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, public_getter: property_options.public_getter, @@ -357,11 +359,11 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { PropertyOption::Key(value) => { set_property_option(&mut options.key, value, attribute, "key")? } - PropertyOption::AdditionalKey(value) => set_property_option( - &mut options.additional_key, + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, value, attribute, - "additional_key", + "additional_keys", )?, PropertyOption::Prefix(value) => { set_property_option(&mut options.prefix, value, attribute, "prefix")? @@ -393,11 +395,11 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { attribute, "parse_properties_with", )?, - PropertyOption::WritePropertiesWith(value) => set_property_option( - &mut options.write_properties_with, + PropertyOption::SerializePropertiesWith(value) => set_property_option( + &mut options.serialize_properties_with, value, attribute, - "write_properties_with", + "serialize_properties_with", )?, PropertyOption::Accessor(accessor) => { let selected = match accessor { @@ -505,6 +507,37 @@ fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Resu } } +fn attribute_expression_list( + attributes: &[Attribute], + name: &str, +) -> syn::Result>> { + let Some(attribute) = find_attribute(attributes, name)? else { + return Ok(None); + }; + + let expressions = match &attribute.meta { + Meta::NameValue(name_value) => expression_list(name_value.value.clone(), name)?, + Meta::List(_) => attribute + .parse_args_with(Punctuated::::parse_terminated)? + .into_iter() + .collect(), + _ => { + return Err(Error::new_spanned( + attribute, + format!("{name} must contain a non-empty list of keys"), + )); + } + }; + + if expressions.is_empty() { + return Err(Error::new_spanned( + attribute, + format!("{name} must contain at least one key"), + )); + } + Ok(Some(expressions)) +} + fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { let Some(expression) = attribute_expression_value(attributes, name)? else { return Ok(None); @@ -523,6 +556,22 @@ fn expression_path(expression: Expr, name: &str) -> syn::Result { } } +fn expression_list(expression: Expr, name: &str) -> syn::Result> { + 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, @@ -552,9 +601,9 @@ fn parse_field(field: &PropertyField) -> TokenStream2 { if let Some(parse_properties_with) = &field.parse_properties_with { let key = field.key.as_ref().expect("exact-key fields have a key"); - let parse = match &field.additional_key { - Some(additional_key) => { - quote!(#parse_properties_with(properties, #key, #additional_key, #default)) + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) } None => quote!(#parse_properties_with(properties, #key, #default)), }; @@ -729,22 +778,24 @@ fn write_field(field: &PropertyField) -> TokenStream2 { let ident = &field.ident; if field.nested { return quote! { - self.#ident.write_properties(properties); + self.#ident.write_properties(properties)?; }; } let default = typed_default(field); - if let Some(write_properties_with) = &field.write_properties_with { + if let Some(serialize_properties_with) = &field.serialize_properties_with { let key = field.key.as_ref().expect("exact-key fields have a key"); - let write = match &field.additional_key { - Some(additional_key) => { - quote!(#write_properties_with(&self.#ident, properties, #key, #additional_key, &#default)) + let serialize = match &field.additional_keys { + Some(additional_keys) => { + quote!(#serialize_properties_with(&self.#ident, properties, #key, &[#(#additional_keys),*], &#default)) } - None => quote!(#write_properties_with(&self.#ident, properties, #key, &#default)), + None => quote!(#serialize_properties_with(&self.#ident, properties, #key, &#default)), }; return quote! { - #write; + #serialize.map_err(|error| { + format!("Failed to serialize {}: {error}", #key) + })?; }; } @@ -762,7 +813,9 @@ fn write_field(field: &PropertyField) -> TokenStream2 { let key = field.key.as_ref().expect("exact-key fields have a key"); let value = match (&field.serialize_with, &field.option_inner_type) { - (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident)), + (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident).map_err(|error| { + format!("Failed to serialize {}: {error}", #key) + })?), (None, Some(_)) => quote!(::std::string::ToString::to_string( self.#ident.as_ref().expect("checked is_some above") )), diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index ae33198a74..8c75f2d635 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -26,13 +26,17 @@ const FANOUT_ENABLED: &str = "write.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, width_key: &str, - height_key: &str, - default: (u64, u64), -) -> Result<(u64, u64), String> { + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } let parse = |property_key: &str, default| { properties .get(property_key) @@ -41,22 +45,35 @@ fn parse_dimensions( .map(|value| value.unwrap_or(default)) }; - Ok((parse(width_key, default.0)?, parse(height_key, default.1)?)) + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) } -fn write_dimensions( - dimensions: &(u64, u64), +fn serialize_dimensions( + dimensions: &(u64, u64, u64), properties: &mut HashMap, width_key: &str, - height_key: &str, - default: &(u64, u64), -) { + additional_keys: &[&str], + default: &(u64, u64, u64), +) -> Result<(), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { + return Err("dimensions must be positive".to_string()); + } properties.remove(width_key); - properties.remove(height_key); + properties.remove(additional_keys[0]); + properties.remove(additional_keys[1]); if dimensions != default { properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(height_key.to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); + properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); } + Ok(()) } #[derive(Debug, Properties)] @@ -82,11 +99,11 @@ struct TestProperties { pub column_fpp: HashMap, #[key(WIDTH)] - #[additional_key(HEIGHT)] - #[default((640, 480))] + #[additional_keys(HEIGHT, DEPTH)] + #[default((640, 480, 320))] #[parse_properties_with(parse_dimensions)] - #[write_properties_with(write_dimensions)] - pub dimensions: (u64, u64), + #[serialize_properties_with(serialize_dimensions)] + pub dimensions: (u64, u64, u64), } #[test] @@ -97,7 +114,7 @@ fn reads_defaults_and_overrides() { assert_eq!(defaults.format, "parquet"); assert!(defaults.fanout_enabled); assert!(defaults.column_fpp.is_empty()); - assert_eq!(defaults.dimensions, (640, 480)); + assert_eq!(defaults.dimensions, (640, 480, 320)); let properties = HashMap::from([ (RETRIES.to_string(), "8".to_string()), @@ -107,6 +124,7 @@ fn reads_defaults_and_overrides() { (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()), ]); let parsed = TestProperties::from_properties(&properties).unwrap(); @@ -115,7 +133,7 @@ fn reads_defaults_and_overrides() { assert_eq!(parsed.format, "orc"); assert!(!parsed.fanout_enabled); assert_eq!(parsed.column_fpp["id"], 0.01); - assert_eq!(parsed.dimensions, (1920, 1080)); + assert_eq!(parsed.dimensions, (1920, 1080, 720)); } #[test] @@ -128,11 +146,12 @@ fn writes_overrides_and_preserves_unrelated_properties() { (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()), ])) .unwrap(); let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); - parsed.write_properties(&mut properties); + parsed.write_properties(&mut properties).unwrap(); assert_eq!(properties[RETRIES], "8"); assert_eq!(properties[OWNER], "iceberg"); @@ -141,6 +160,7 @@ fn writes_overrides_and_preserves_unrelated_properties() { assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); assert_eq!(properties[WIDTH], "1920"); assert_eq!(properties[HEIGHT], "1080"); + assert_eq!(properties[DEPTH], "720"); assert_eq!(properties["unrelated"], "value"); } @@ -155,10 +175,11 @@ fn writing_defaults_removes_modeled_properties() { (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()), ("unrelated".to_string(), "value".to_string()), ]); - defaults.write_properties(&mut properties); + defaults.write_properties(&mut properties).unwrap(); assert_eq!( properties, @@ -211,7 +232,7 @@ fn nested_properties_use_a_flat_property_map() { properties.commit.num_retries = 9; let mut written = HashMap::new(); - properties.write_properties(&mut written); + properties.write_properties(&mut written).unwrap(); assert_eq!( written, HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) @@ -230,8 +251,13 @@ fn parse_non_empty(value: &str) -> Result { } } -fn serialize_trimmed(value: &str) -> String { - value.trim().to_string() +fn serialize_trimmed(value: &str) -> Result { + let value = value.trim(); + if value.is_empty() { + Err("value must not be empty") + } else { + Ok(value.to_string()) + } } #[derive(Debug, Properties)] @@ -263,10 +289,34 @@ fn custom_single_value_hooks_can_validate_and_normalize() { location: " normalized ".to_string(), }; let mut written = HashMap::new(); - properties.write_properties(&mut written); + properties.write_properties(&mut written).unwrap(); assert_eq!(written["location"], "normalized"); } +#[test] +fn reports_custom_serialization_errors() { + let invalid_location = ValidatedProperties { + location: " ".to_string(), + }; + let error = invalid_location + .write_properties(&mut HashMap::new()) + .unwrap_err(); + assert_eq!( + error, + "Failed to serialize location: value must not be empty" + ); + + let mut invalid_dimensions = TestProperties::from_properties(&HashMap::new()).unwrap(); + invalid_dimensions.dimensions = (0, 480, 320); + let error = invalid_dimensions + .write_properties(&mut HashMap::new()) + .unwrap_err(); + assert_eq!( + error, + "Failed to serialize dimensions.width: dimensions must be positive" + ); +} + mod accessor_fixture { use iceberg_property_macro::Properties; From 0068cce9872d1d269158f078b6b821d9dab68456 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Thu, 6 Aug 2026 16:03:34 +0800 Subject: [PATCH 03/11] Fix property macro CI checks --- .typos.toml | 1 + crates/property-macro/public-api.txt | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 crates/property-macro/public-api.txt diff --git a/.typos.toml b/.typos.toml index e9fa0028f5..cde21f7091 100644 --- a/.typos.toml +++ b/.typos.toml @@ -21,6 +21,7 @@ extend-ignore-identifiers-re = ["^bimap$"] [default.extend-words] AGS = "AGS" ags = "ags" +ser = "ser" [files] extend-exclude = ["**/testdata", "CHANGELOG.md", "**/public-api.txt"] diff --git a/crates/property-macro/public-api.txt b/crates/property-macro/public-api.txt new file mode 100644 index 0000000000..8edb5d0952 --- /dev/null +++ b/crates/property-macro/public-api.txt @@ -0,0 +1,2 @@ +pub mod iceberg_property_macro +pub proc macro iceberg_property_macro::#[derive(Properties)] From 4e35b2c825e1b5b903c407454f46895b707ca13b Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Fri, 7 Aug 2026 16:57:19 +0800 Subject: [PATCH 04/11] Remove write support --- crates/property-macro/README.md | 245 ++++++------------- crates/property-macro/src/lib.rs | 4 +- crates/property-macro/src/properties.rs | 260 +++++++------------- crates/property-macro/tests/properties.rs | 275 ++++++---------------- 4 files changed, 230 insertions(+), 554 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 4ab30c126f..48445bdc1e 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -19,43 +19,40 @@ # Iceberg property derive macro -`Properties` generates inherent methods for reading and writing a typed struct -from a flat `HashMap`. It deliberately does not implement -`Default`, `Serialize`, `Deserialize`, or any other trait. +`Properties` parses a typed struct from a flat `HashMap` 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 methods +## Generated API -For every annotated struct, `#[derive(Properties)]` generates these inherent -methods: +For every annotated struct, `#[derive(Properties)]` generates this inherent +constructor: ```text impl MyProperties { pub fn from_properties( properties: &HashMap, ) -> Result; - - pub fn write_properties( - &self, - properties: &mut HashMap, - ) -> Result<(), String>; } ``` -`from_properties` parses every modeled property, uses its annotated default -when absent, and returns an error containing the primary property key when a -value is invalid. Unknown keys are ignored. +`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 error containing its primary property key. -`write_properties` updates an existing map. It removes modeled keys whose -values equal their annotated defaults, writes non-default values as strings, -and preserves unknown keys. It returns an error when a custom -`serialize_with` or `serialize_properties_with` hook fails. +Adding `pub(getter)` to a field generates an immutable accessor with the field +name. Structurally known `Copy` types return `T`; other types return `&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 exercises the complete generated API: exact keys and defaults, -optional values, case-insensitive booleans, prefixed maps, nested property -groups, custom single-value conversion, custom multi-key conversion, public -accessors, contextual errors, and writing into an existing property map. +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; @@ -80,15 +77,6 @@ fn parse_location(value: &str) -> Result { } } -fn serialize_location(value: &str) -> Result { - let location = value.trim().trim_end_matches('/'); - if location.is_empty() { - Err("location must not be empty") - } else { - Ok(location.to_string()) - } -} - fn parse_dimensions( properties: &HashMap, width_key: &str, @@ -113,93 +101,61 @@ fn parse_dimensions( )) } -fn serialize_dimensions( - dimensions: &(u64, u64, u64), - properties: &mut HashMap, - width_key: &str, - additional_keys: &[&str], - default: &(u64, u64, u64), -) -> Result<(), String> { - if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); - } - if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { - return Err("dimensions must be positive".to_string()); - } - properties.remove(width_key); - properties.remove(additional_keys[0]); - properties.remove(additional_keys[1]); - if dimensions != default { - properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); - properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); - } - Ok(()) -} - #[derive(Debug, Properties)] struct CommitProperties { - #[property( - key = RETRIES, - default = 4, - pub(getter), - pub(setter) - )] + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, pub(getter))] retries: usize, } #[derive(Debug, Properties)] struct TableLikeProperties { - // Nested groups still read and write the same flat property map. - #[property(nested)] + /// Nested groups parse from the same flat property map. + #[property(nested, pub(getter))] commit: CommitProperties, - // Option distinguishes an absent property from a present value. - #[property(key = OWNER, default = None, pub(getter), pub(setter))] + /// Option distinguishes an absent property from a present value. + #[property(key = OWNER, default = None, pub(getter))] owner: Option, - // Boolean values are parsed case-insensitively. + /// Boolean values are parsed case-insensitively. #[property(key = FANOUT, default = true, pub(getter))] fanout_enabled: bool, - // A prefix captures suffix/value pairs into a typed map. + /// A prefix captures suffix/value pairs into a typed map. #[property(prefix = COLUMN_FPP_PREFIX, default = HashMap::new(), pub(getter))] column_fpp: HashMap, - // Single-key hooks provide validation and custom string conversion. + /// A single-key parser can validate and normalize a property value. #[property( key = LOCATION, default = "warehouse", parse_with = parse_location, - serialize_with = serialize_location, - pub(getter), - pub(setter) + pub(getter) )] location: String, - // Full-map hooks can model one field with multiple property keys. + /// A full-map parser can model one field with multiple property keys. #[property( key = WIDTH, additional_keys = [HEIGHT, DEPTH], default = (640, 480, 320), parse_properties_with = parse_dimensions, - serialize_properties_with = serialize_dimensions, pub(getter) )] dimensions: (u64, u64, u64), } fn main() -> Result<(), String> { - // An empty map uses every annotated property default. let defaults = TableLikeProperties::from_properties(&HashMap::new())?; - assert_eq!(*defaults.commit.retries(), 4); + assert_eq!(defaults.commit().retries(), 4); assert_eq!(defaults.owner(), &None); - assert!(*defaults.fanout_enabled()); + assert!(defaults.fanout_enabled()); assert!(defaults.column_fpp().is_empty()); assert_eq!(defaults.location(), "warehouse"); - assert_eq!(defaults.dimensions(), &(640, 480, 320)); + assert_eq!(defaults.dimensions(), (640, 480, 320)); - let mut raw = HashMap::from([ + let raw = HashMap::from([ (RETRIES.to_string(), "8".to_string()), (OWNER.to_string(), "iceberg".to_string()), (FANOUT.to_string(), "FALSE".to_string()), @@ -208,35 +164,17 @@ fn main() -> Result<(), String> { (WIDTH.to_string(), "1920".to_string()), (HEIGHT.to_string(), "1080".to_string()), (DEPTH.to_string(), "720".to_string()), - ("unmodeled".to_string(), "preserved".to_string()), + ("unmodeled".to_string(), "ignored".to_string()), ]); - let mut properties = TableLikeProperties::from_properties(&raw)?; - assert_eq!(*properties.commit.retries(), 8); + let properties = TableLikeProperties::from_properties(&raw)?; + assert_eq!(properties.commit().retries(), 8); 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.dimensions(), &(1920, 1080, 720)); - - // Generated setters modify private fields. Writing removes modeled values - // reset to their defaults and preserves properties the struct does not own. - properties.commit.set_retries(10); - properties.set_owner(None); - properties.set_location("s3://bucket/new-table/".to_string()); - properties.write_properties(&mut raw)?; - - assert_eq!(raw[RETRIES], "10"); - assert!(!raw.contains_key(OWNER)); - assert_eq!(raw[FANOUT], "false"); - assert_eq!(raw[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); - assert_eq!(raw[LOCATION], "s3://bucket/new-table"); - assert_eq!(raw[WIDTH], "1920"); - assert_eq!(raw[HEIGHT], "1080"); - assert_eq!(raw[DEPTH], "720"); - assert_eq!(raw["unmodeled"], "preserved"); - - // Parsing errors identify the primary property key. + assert_eq!(properties.dimensions(), (1920, 1080, 720)); + let error = TableLikeProperties::from_properties(&HashMap::from([( LOCATION.to_string(), "/".to_string(), @@ -248,95 +186,56 @@ fn main() -> Result<(), String> { } ``` -The annotated property default is independent of the value produced by a -derived `Default` implementation. When both are used, keep them aligned. - -## Using a property map with Serde +## Using ordinary derives together -Serde's standard derives serialize a struct's fields and cannot infer the -property-map representation from `Properties` attributes. A transparent adapter -keeps that conversion explicit while allowing `Default`, `Serialize`, and -`Deserialize` to remain ordinary derives: +`Properties` does not implicitly derive other traits, so `Default`, +`Serialize`, and `Deserialize` can be selected independently and behave like +ordinary Rust derives: ```rust use std::collections::HashMap; use iceberg_property_macro::Properties; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Serialize}; -#[derive(Debug, Default, PartialEq, Properties)] -struct WriteProperties { +#[derive(Debug, Default, Serialize, Deserialize, Properties)] +struct ReadProperties { #[property( key = "commit.retry.num-retries", - default = 0, - pub(getter), - pub(setter) + default = 4, + pub(getter) )] retries: u64, - - #[property(key = "owner", default = None)] - owner: Option, } -mod property_map { - use super::*; - - pub fn serialize(value: &WriteProperties, serializer: S) -> Result - where - S: Serializer, - { - let mut properties = HashMap::new(); - value - .write_properties(&mut properties) - .map_err(serde::ser::Error::custom)?; - properties.serialize(serializer) - } - - pub fn deserialize<'de, D>(deserializer: D) -> Result - where - D: Deserializer<'de>, - { - let properties = HashMap::::deserialize(deserializer)?; - WriteProperties::from_properties(&properties).map_err(serde::de::Error::custom) - } -} - -#[derive(Debug, Default, Serialize, Deserialize)] -#[serde(transparent)] -struct PropertyDocument(#[serde(with = "property_map")] WriteProperties); - fn main() -> Result<(), Box> { - let mut document = PropertyDocument::default(); - document.0.set_retries(4); - - let json = serde_json::to_string(&document)?; - assert_eq!(json, r#"{"commit.retry.num-retries":"4"}"#); - - let decoded: PropertyDocument = serde_json::from_str(&json)?; - assert_eq!(*decoded.0.retries(), 4); + // The property annotation supplies the default used by from_properties. + let properties = ReadProperties::from_properties(&HashMap::new())?; + assert_eq!(properties.retries(), 4); + + // The ordinary Default derive uses the field's Rust default instead. + let defaults = ReadProperties::default(); + assert_eq!(defaults.retries(), 0); + + // Ordinary Serde derives use Rust field names, not property keys. + let json = serde_json::to_string(&properties)?; + assert_eq!(json, r#"{"retries":4}"#); + let decoded: ReadProperties = serde_json::from_str(r#"{"retries":7}"#)?; + assert_eq!(decoded.retries(), 7); Ok(()) } ``` Property options may be grouped under `#[property(...)]`, which avoids a collision between the standalone `#[default(...)]` helper and Rust's `Default` -derive. The standalone annotations from the original framework remain -supported. - -`#[prefix(...)]` captures a family of properties in a `HashMap`, -keyed by the suffix after the prefix. `#[nested]` embeds another `Properties` -struct while keeping the property map flat. `#[parse_with(...)]` and -`#[serialize_with(...)]` customize conversion for one exact-key field. The -latter name refers to conversion into a property string and does not require -Serde. - -`#[parse_properties_with(...)]` and `#[serialize_properties_with(...)]` receive the -complete property map for fields represented by more than one key. -`#[additional_keys(...)]` supplies a list of secondary keys to those hooks. -Custom serialization hooks return `Result`, receive the field default, and are -responsible for removing or omitting default-valued properties. - -Boolean property values are parsed case-insensitively. Other values require -`FromStr` and `ToString` unless custom conversion hooks are supplied. Leaf -fields require `PartialEq` so default values can be omitted. String-literal and -path defaults are converted into their field type with `Into`. +derive. The original standalone annotations remain supported. + +`#[prefix(...)]` requires `HashMap`. `#[nested]` embeds another +`Properties` struct while reading the same flat map. `#[parse_with(...)]` +customizes parsing for one exact-key field. `#[parse_properties_with(...)]` +receives the complete property map, and `#[additional_keys(...)]` supplies its +list of secondary keys. + +Boolean values are parsed case-insensitively. Other values require `FromStr` +unless a custom parser is supplied. String-literal and path defaults are +converted into their field type with `Into`. diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 11b1746d76..1e7e9da4f9 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -22,7 +22,7 @@ use syn::{DeriveInput, parse_macro_input}; mod properties; -/// Derives property-map parsing, writing, and opt-in accessors for a struct. +/// Derives property-map parsing and opt-in read-only accessors for a struct. #[proc_macro_derive( Properties, attributes( @@ -32,9 +32,7 @@ mod properties; nested, default, parse_with, - serialize_with, parse_properties_with, - serialize_properties_with, property ) )] diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index f808195c39..67446a7aeb 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -16,7 +16,7 @@ // under the License. use proc_macro2::TokenStream as TokenStream2; -use quote::{format_ident, quote}; +use quote::quote; use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; use syn::{ @@ -33,20 +33,14 @@ struct PropertyField { nested: bool, default: Option, parse_with: Option, - serialize_with: Option, parse_properties_with: Option, - serialize_properties_with: Option, option_inner_type: Option, map_value_type: Option, public_getter: bool, - public_setter: bool, doc_attributes: Vec, } -enum PublicAccessor { - Getter, - Setter, -} +struct PublicGetter; enum PropertyOption { Key(Expr), @@ -55,10 +49,8 @@ enum PropertyOption { Nested, Default(Expr), ParseWith(Path), - SerializeWith(Path), ParsePropertiesWith(Path), - SerializePropertiesWith(Path), - Accessor(PublicAccessor), + Getter(PublicGetter), } #[derive(Default)] @@ -69,27 +61,24 @@ struct PropertyOptions { nested: bool, default: Option, parse_with: Option, - serialize_with: Option, parse_properties_with: Option, - serialize_properties_with: Option, public_getter: bool, - public_setter: bool, } -impl Parse for PublicAccessor { +impl Parse for PublicGetter { fn parse(input: ParseStream<'_>) -> syn::Result { input.parse::()?; let content; parenthesized!(content in input); let accessor = content.parse::()?; if !content.is_empty() { - return Err(content.error("expected getter or setter")); + return Err(content.error("expected getter")); } - match accessor.to_string().as_str() { - "getter" => Ok(Self::Getter), - "setter" => Ok(Self::Setter), - _ => Err(Error::new_spanned(accessor, "expected getter or setter")), + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) } } } @@ -97,7 +86,7 @@ impl Parse for PublicAccessor { impl Parse for PropertyOption { fn parse(input: ParseStream<'_>) -> syn::Result { if input.peek(Token![pub]) { - return input.parse().map(Self::Accessor); + return input.parse().map(Self::Getter); } let name = input.parse::()?; @@ -116,14 +105,9 @@ impl Parse for PropertyOption { "prefix" => Ok(Self::Prefix(expression)), "default" => Ok(Self::Default(expression)), "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), - "serialize_with" => { - expression_path(expression, "serialize_with").map(Self::SerializeWith) - } "parse_properties_with" => { expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) } - "serialize_properties_with" => expression_path(expression, "serialize_properties_with") - .map(Self::SerializePropertiesWith), _ => Err(Error::new_spanned(name, "unknown property option")), } } @@ -152,11 +136,10 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result let fields = fields .iter() - .map(parse_property_field) + .map(|field| parse_property_field(field, property_options(&field.attrs)?)) .collect::>>()?; let parses = fields.iter().map(parse_field); - let property_writes = fields.iter().map(write_field); - let accessors = fields.iter().map(field_accessors); + let accessors = fields.iter().map(field_getter); let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); Ok(quote! { @@ -173,27 +156,18 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result #(#parses,)* }) } - - pub fn write_properties( - &self, - properties: &mut ::std::collections::HashMap< - ::std::string::String, - ::std::string::String, - >, - ) -> ::std::result::Result<(), ::std::string::String> { - #(#property_writes)* - Ok(()) - } } }) } -fn parse_property_field(field: &Field) -> syn::Result { +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result { let ident = field .ident .clone() .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; - let property_options = property_options(&field.attrs)?; let key = merge_attribute_option( attribute_expression_value(&field.attrs, "key")?, property_options.key, @@ -261,44 +235,25 @@ fn parse_property_field(field: &Field) -> syn::Result { field, "parse_with", )?; - let serialize_with = merge_attribute_option( - attribute_path_value(&field.attrs, "serialize_with")?, - property_options.serialize_with, - field, - "serialize_with", - )?; let parse_properties_with = merge_attribute_option( attribute_path_value(&field.attrs, "parse_properties_with")?, property_options.parse_properties_with, field, "parse_properties_with", )?; - let serialize_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "serialize_properties_with")?, - property_options.serialize_properties_with, - field, - "serialize_properties_with", - )?; - if additional_keys.is_some() - && parse_properties_with.is_none() - && serialize_properties_with.is_none() - { + if additional_keys.is_some() && parse_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_keys(...)] requires parse_properties_with or serialize_properties_with", + "#[additional_keys(...)] requires parse_properties_with", )); } if (prefix.is_some() || nested) - && (additional_keys.is_some() - || parse_with.is_some() - || serialize_with.is_some() - || parse_properties_with.is_some() - || serialize_properties_with.is_some()) + && (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 or write functions", + "#[prefix(...)] and #[nested] fields do not support custom parse functions", )); } if parse_with.is_some() && parse_properties_with.is_some() { @@ -307,13 +262,6 @@ fn parse_property_field(field: &Field) -> syn::Result { "fields cannot declare both parse_with and parse_properties_with", )); } - if serialize_with.is_some() && serialize_properties_with.is_some() { - return Err(Error::new_spanned( - field, - "fields cannot declare both serialize_with and serialize_properties_with", - )); - } - Ok(PropertyField { ident, ty: field.ty.clone(), @@ -323,13 +271,10 @@ fn parse_property_field(field: &Field) -> syn::Result { nested, default, parse_with, - serialize_with, parse_properties_with, - serialize_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, public_getter: property_options.public_getter, - public_setter: property_options.public_setter, doc_attributes: field .attrs .iter() @@ -383,33 +328,17 @@ fn property_options(attributes: &[Attribute]) -> syn::Result { PropertyOption::ParseWith(value) => { set_property_option(&mut options.parse_with, value, attribute, "parse_with")? } - PropertyOption::SerializeWith(value) => set_property_option( - &mut options.serialize_with, - value, - attribute, - "serialize_with", - )?, PropertyOption::ParsePropertiesWith(value) => set_property_option( &mut options.parse_properties_with, value, attribute, "parse_properties_with", )?, - PropertyOption::SerializePropertiesWith(value) => set_property_option( - &mut options.serialize_properties_with, - value, - attribute, - "serialize_properties_with", - )?, - PropertyOption::Accessor(accessor) => { - let selected = match accessor { - PublicAccessor::Getter => &mut options.public_getter, - PublicAccessor::Setter => &mut options.public_setter, - }; - if *selected { + PropertyOption::Getter(_) => { + if options.public_getter { return Err(Error::new_spanned(attribute, "duplicate property accessor")); } - *selected = true; + options.public_getter = true; } } } @@ -449,32 +378,27 @@ fn merge_attribute_option( } } -fn field_accessors(field: &PropertyField) -> TokenStream2 { +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; - let getter = field.public_getter.then(|| { + if is_copy_type(ty) { quote! { #(#docs)* - pub fn #ident(&self) -> &#ty { - &self.#ident + pub fn #ident(&self) -> #ty { + self.#ident } } - }); - let setter = field.public_setter.then(|| { - let setter_ident = format_ident!("set_{}", ident); - let setter_doc = format!("Sets `{ident}`."); + } else { quote! { - #[doc = #setter_doc] - pub fn #setter_ident(&mut self, value: #ty) { - self.#ident = value; + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident } } - }); - - quote! { - #getter - #setter } } @@ -762,6 +686,55 @@ fn is_bool(ty: &Type) -> bool { is_named_type(ty, "bool") } +fn is_copy_type(ty: &Type) -> bool { + match ty { + Type::Array(array) => is_copy_type(&array.elem), + Type::BareFn(_) | Type::Never(_) | Type::Ptr(_) => true, + Type::Group(group) => is_copy_type(&group.elem), + Type::Paren(paren) => is_copy_type(&paren.elem), + Type::Reference(reference) => reference.mutability.is_none(), + Type::Tuple(tuple) => tuple.elems.iter().all(is_copy_type), + Type::Path(type_path) if type_path.qself.is_none() => { + let Some(segment) = type_path.path.segments.last() else { + return false; + }; + if matches!( + segment.ident.to_string().as_str(), + "bool" + | "char" + | "f32" + | "f64" + | "i8" + | "i16" + | "i32" + | "i64" + | "i128" + | "isize" + | "u8" + | "u16" + | "u32" + | "u64" + | "u128" + | "usize" + ) { + return true; + } + if segment.ident != "Option" && segment.ident != "Result" { + return false; + } + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return false; + }; + arguments.args.iter().all(|argument| match argument { + GenericArgument::Lifetime(_) => true, + GenericArgument::Type(ty) => is_copy_type(ty), + _ => false, + }) + } + _ => false, + } +} + fn is_named_type(ty: &Type, name: &str) -> bool { let Type::Path(type_path) = ty else { return false; @@ -773,70 +746,3 @@ fn is_named_type(ty: &Type, name: &str) -> bool { .last() .is_some_and(|segment| segment.ident == name) } - -fn write_field(field: &PropertyField) -> TokenStream2 { - let ident = &field.ident; - if field.nested { - return quote! { - self.#ident.write_properties(properties)?; - }; - } - - let default = typed_default(field); - - if let Some(serialize_properties_with) = &field.serialize_properties_with { - let key = field.key.as_ref().expect("exact-key fields have a key"); - let serialize = match &field.additional_keys { - Some(additional_keys) => { - quote!(#serialize_properties_with(&self.#ident, properties, #key, &[#(#additional_keys),*], &#default)) - } - None => quote!(#serialize_properties_with(&self.#ident, properties, #key, &#default)), - }; - return quote! { - #serialize.map_err(|error| { - format!("Failed to serialize {}: {error}", #key) - })?; - }; - } - - if let Some(prefix) = &field.prefix { - return quote! { - properties.retain(|key, _| !key.starts_with(#prefix)); - if self.#ident != #default { - for (suffix, value) in &self.#ident { - let key = format!("{}{}", #prefix, suffix); - properties.insert(key, ::std::string::ToString::to_string(value)); - } - } - }; - } - - let key = field.key.as_ref().expect("exact-key fields have a key"); - let value = match (&field.serialize_with, &field.option_inner_type) { - (Some(serialize_with), _) => quote!(#serialize_with(&self.#ident).map_err(|error| { - format!("Failed to serialize {}: {error}", #key) - })?), - (None, Some(_)) => quote!(::std::string::ToString::to_string( - self.#ident.as_ref().expect("checked is_some above") - )), - (None, None) => quote!(::std::string::ToString::to_string(&self.#ident)), - }; - let insert = if field.option_inner_type.is_some() { - quote! { - if self.#ident != #default && self.#ident.is_some() { - properties.insert((#key).to_string(), #value); - } - } - } else { - quote! { - if self.#ident != #default { - properties.insert((#key).to_string(), #value); - } - } - }; - - quote! { - properties.remove(#key); - #insert - } -} diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 8c75f2d635..2eecde722e 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; use iceberg_property_macro::Properties; +use serde::{Deserialize, Serialize}; const RETRIES: &str = "commit.retry.num-retries"; const OWNER: &str = "owner"; @@ -52,139 +53,72 @@ fn parse_dimensions( )) } -fn serialize_dimensions( - dimensions: &(u64, u64, u64), - properties: &mut HashMap, - width_key: &str, - additional_keys: &[&str], - default: &(u64, u64, u64), -) -> Result<(), String> { - if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); - } - if dimensions.0 == 0 || dimensions.1 == 0 || dimensions.2 == 0 { - return Err("dimensions must be positive".to_string()); - } - properties.remove(width_key); - properties.remove(additional_keys[0]); - properties.remove(additional_keys[1]); - if dimensions != default { - properties.insert(width_key.to_string(), dimensions.0.to_string()); - properties.insert(additional_keys[0].to_string(), dimensions.1.to_string()); - properties.insert(additional_keys[1].to_string(), dimensions.2.to_string()); - } - Ok(()) -} - #[derive(Debug, Properties)] struct TestProperties { #[key(RETRIES)] #[default(4)] - pub retries: u64, + #[property(pub(getter))] + retries: u64, - #[key(OWNER)] - #[default(None)] - pub owner: Option, + #[property(key = OWNER, default = None, pub(getter))] + owner: Option, - #[key(FORMAT)] - #[default("parquet")] - pub format: String, + #[property(key = FORMAT, default = "parquet", pub(getter))] + format: String, - #[key(FANOUT_ENABLED)] - #[default(true)] - pub fanout_enabled: bool, + #[property(key = FANOUT_ENABLED, default = true, pub(getter))] + fanout_enabled: bool, - #[prefix(COLUMN_FPP_PREFIX)] - #[default(HashMap::new())] - pub column_fpp: HashMap, + #[property( + prefix = COLUMN_FPP_PREFIX, + default = HashMap::new(), + pub(getter) + )] + column_fpp: HashMap, - #[key(WIDTH)] - #[additional_keys(HEIGHT, DEPTH)] - #[default((640, 480, 320))] - #[parse_properties_with(parse_dimensions)] - #[serialize_properties_with(serialize_dimensions)] - pub dimensions: (u64, u64, u64), + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), } #[test] -fn reads_defaults_and_overrides() { - let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); - assert_eq!(defaults.retries, 4); - assert_eq!(defaults.owner, None); - assert_eq!(defaults.format, "parquet"); - assert!(defaults.fanout_enabled); - assert!(defaults.column_fpp.is_empty()); - assert_eq!(defaults.dimensions, (640, 480, 320)); +fn reads_defaults_through_generated_getters() { + let properties = TestProperties::from_properties(&HashMap::new()).unwrap(); - let properties = 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()), - ]); - let parsed = TestProperties::from_properties(&properties).unwrap(); - - assert_eq!(parsed.retries, 8); - assert_eq!(parsed.owner.as_deref(), Some("iceberg")); - assert_eq!(parsed.format, "orc"); - assert!(!parsed.fanout_enabled); - assert_eq!(parsed.column_fpp["id"], 0.01); - assert_eq!(parsed.dimensions, (1920, 1080, 720)); -} - -#[test] -fn writes_overrides_and_preserves_unrelated_properties() { - let parsed = TestProperties::from_properties(&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()), - ])) - .unwrap(); - let mut properties = HashMap::from([("unrelated".to_string(), "value".to_string())]); - - parsed.write_properties(&mut properties).unwrap(); - - assert_eq!(properties[RETRIES], "8"); - assert_eq!(properties[OWNER], "iceberg"); - assert_eq!(properties[FORMAT], "orc"); - assert_eq!(properties[FANOUT_ENABLED], "false"); - assert_eq!(properties[&format!("{COLUMN_FPP_PREFIX}id")], "0.01"); - assert_eq!(properties[WIDTH], "1920"); - assert_eq!(properties[HEIGHT], "1080"); - assert_eq!(properties[DEPTH], "720"); - assert_eq!(properties["unrelated"], "value"); + 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 writing_defaults_removes_modeled_properties() { - let defaults = TestProperties::from_properties(&HashMap::new()).unwrap(); - let mut properties = HashMap::from([ +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()), + (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()), - ("unrelated".to_string(), "value".to_string()), + ("unknown".to_string(), "ignored".to_string()), ]); + let properties = TestProperties::from_properties(&raw).unwrap(); - defaults.write_properties(&mut properties).unwrap(); - - assert_eq!( - properties, - HashMap::from([("unrelated".to_string(), "value".to_string())]) - ); + 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] @@ -212,34 +146,25 @@ fn reports_the_property_with_an_invalid_value() { assert!(prefix_error.contains(&prefixed_key)); } -#[derive(Clone, Debug, Properties)] +#[derive(Debug, Properties)] struct CommitProperties { - #[key = "commit.retry.num-retries"] - #[default = 4] - pub num_retries: u64, + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, } #[derive(Debug, Properties)] struct NestedProperties { - #[nested] - pub commit: CommitProperties, + #[property(nested, pub(getter))] + commit: CommitProperties, } #[test] -fn nested_properties_use_a_flat_property_map() { - let mut properties = NestedProperties::from_properties(&HashMap::new()).unwrap(); - assert_eq!(properties.commit.num_retries, 4); +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(); - properties.commit.num_retries = 9; - let mut written = HashMap::new(); - properties.write_properties(&mut written).unwrap(); - assert_eq!( - written, - HashMap::from([("commit.retry.num-retries".to_string(), "9".to_string())]) - ); - - let decoded = NestedProperties::from_properties(&written).unwrap(); - assert_eq!(decoded.commit.num_retries, 9); + assert_eq!(properties.commit().retries(), 9); } fn parse_non_empty(value: &str) -> Result { @@ -251,32 +176,25 @@ fn parse_non_empty(value: &str) -> Result { } } -fn serialize_trimmed(value: &str) -> Result { - let value = value.trim(); - if value.is_empty() { - Err("value must not be empty") - } else { - Ok(value.to_string()) - } -} - #[derive(Debug, Properties)] struct ValidatedProperties { - #[key = "location"] - #[default = "default"] - #[parse_with(parse_non_empty)] - #[serialize_with(serialize_trimmed)] + #[property( + key = "location", + default = "default", + parse_with = parse_non_empty, + pub(getter) + )] location: String, } #[test] -fn custom_single_value_hooks_can_validate_and_normalize() { +fn custom_single_value_parser_can_validate_and_normalize() { let parsed = ValidatedProperties::from_properties(&HashMap::from([( "location".to_string(), " path ".to_string(), )])) .unwrap(); - assert_eq!(parsed.location, "path"); + assert_eq!(parsed.location(), "path"); let error = ValidatedProperties::from_properties(&HashMap::from([( "location".to_string(), @@ -284,71 +202,26 @@ fn custom_single_value_hooks_can_validate_and_normalize() { )])) .unwrap_err(); assert_eq!(error, "Invalid value for location: value must not be empty"); +} - let properties = ValidatedProperties { - location: " normalized ".to_string(), - }; - let mut written = HashMap::new(); - properties.write_properties(&mut written).unwrap(); - assert_eq!(written["location"], "normalized"); +#[derive(Debug, Default, Serialize, Deserialize, Properties)] +struct DerivedTraitProperties { + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, } #[test] -fn reports_custom_serialization_errors() { - let invalid_location = ValidatedProperties { - location: " ".to_string(), - }; - let error = invalid_location - .write_properties(&mut HashMap::new()) - .unwrap_err(); - assert_eq!( - error, - "Failed to serialize location: value must not be empty" - ); +fn coexists_with_default_serialize_and_deserialize_derives() { + let defaults = DerivedTraitProperties::default(); + assert_eq!(defaults.retries(), 0); - let mut invalid_dimensions = TestProperties::from_properties(&HashMap::new()).unwrap(); - invalid_dimensions.dimensions = (0, 480, 320); - let error = invalid_dimensions - .write_properties(&mut HashMap::new()) - .unwrap_err(); + let properties = DerivedTraitProperties::from_properties(&HashMap::new()).unwrap(); + assert_eq!(properties.retries(), 4); assert_eq!( - error, - "Failed to serialize dimensions.width: dimensions must be positive" + serde_json::to_string(&properties).unwrap(), + r#"{"retries":4}"# ); -} - -mod accessor_fixture { - use iceberg_property_macro::Properties; - - #[derive(Debug, Default, Properties)] - pub struct AccessorProperties { - #[doc = "A property with public read and write access."] - #[property(key = "public.both", default = 0, pub(getter), pub(setter))] - both: u64, - - #[property(key = "public.getter", default = "", pub(getter))] - getter_only: String, - - #[property(key = "public.setter", default = false, pub(setter))] - setter_only: bool, - } - - impl AccessorProperties { - pub fn setter_only_for_test(&self) -> bool { - self.setter_only - } - } -} - -#[test] -fn coexists_with_derived_default_and_generates_opt_in_accessors() { - let mut properties = accessor_fixture::AccessorProperties::default(); - - assert_eq!(*properties.both(), 0); - properties.set_both(2); - assert_eq!(*properties.both(), 2); - assert_eq!(properties.getter_only(), ""); - properties.set_setter_only(true); - assert!(properties.setter_only_for_test()); + let decoded: DerivedTraitProperties = serde_json::from_str(r#"{"retries":7}"#).unwrap(); + assert_eq!(decoded.retries(), 7); } From 5896eea9b90daf1ea1b5b9e0ab4e5fc5cf6ef288 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Fri, 7 Aug 2026 17:11:37 +0800 Subject: [PATCH 05/11] Force attributes --- crates/property-macro/README.md | 17 ++- crates/property-macro/src/lib.rs | 14 +- crates/property-macro/src/properties.rs | 166 ++++------------------ crates/property-macro/tests/properties.rs | 4 +- 4 files changed, 35 insertions(+), 166 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 48445bdc1e..7187e5ce26 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -226,15 +226,14 @@ fn main() -> Result<(), Box> { } ``` -Property options may be grouped under `#[property(...)]`, which avoids a -collision between the standalone `#[default(...)]` helper and Rust's `Default` -derive. The original standalone annotations remain supported. - -`#[prefix(...)]` requires `HashMap`. `#[nested]` embeds another -`Properties` struct while reading the same flat map. `#[parse_with(...)]` -customizes parsing for one exact-key field. `#[parse_properties_with(...)]` -receives the complete property map, and `#[additional_keys(...)]` supplies its -list of secondary keys. +All field settings must be grouped under `#[property(...)]`. This keeps `key`, +`default`, `prefix`, `nested`, parser hooks, additional keys, and getter +generation in one attribute and avoids collisions with ordinary Rust derives. + +The `prefix` setting requires `HashMap`. `nested` embeds another +`Properties` struct while reading the same flat map. `parse_with` customizes +parsing for one exact-key field. `parse_properties_with` receives the complete +property map, and `additional_keys` supplies its list of secondary keys. Boolean values are parsed case-insensitively. Other values require `FromStr` unless a custom parser is supplied. String-literal and path defaults are diff --git a/crates/property-macro/src/lib.rs b/crates/property-macro/src/lib.rs index 1e7e9da4f9..e73b48c0f7 100644 --- a/crates/property-macro/src/lib.rs +++ b/crates/property-macro/src/lib.rs @@ -23,19 +23,7 @@ use syn::{DeriveInput, parse_macro_input}; mod properties; /// Derives property-map parsing and opt-in read-only accessors for a struct. -#[proc_macro_derive( - Properties, - attributes( - key, - additional_keys, - prefix, - nested, - default, - parse_with, - parse_properties_with, - property - ) -)] +#[proc_macro_derive(Properties, attributes(property))] pub fn derive_properties(input: TokenStream) -> TokenStream { let input = parse_macro_input!(input as DeriveInput); diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 67446a7aeb..5416aba19f 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -21,7 +21,7 @@ use syn::parse::{Parse, ParseStream}; use syn::punctuated::Punctuated; use syn::{ Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, - Ident, Lit, Meta, Path, PathArguments, Token, Type, parenthesized, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, }; struct PropertyField { @@ -136,7 +136,7 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result let fields = fields .iter() - .map(|field| parse_property_field(field, property_options(&field.attrs)?)) + .map(|field| parse_property_field(field, property_options(field)?)) .collect::>>()?; let parses = fields.iter().map(parse_field); let accessors = fields.iter().map(field_getter); @@ -168,56 +168,34 @@ fn parse_property_field( .ident .clone() .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; - let key = merge_attribute_option( - attribute_expression_value(&field.attrs, "key")?, - property_options.key, - field, - "key", - )?; - let additional_keys = merge_attribute_option( - attribute_expression_list(&field.attrs, "additional_keys")?, - property_options.additional_keys, - field, - "additional_keys", - )?; - let prefix = merge_attribute_option( - attribute_expression_value(&field.attrs, "prefix")?, - property_options.prefix, - field, - "prefix", - )?; - let standalone_nested = marker_attribute(&field.attrs, "nested")?; - if standalone_nested && property_options.nested { - return Err(Error::new_spanned( - field, - "duplicate nested property option", - )); - } - let nested = standalone_nested || property_options.nested; + 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]", + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", )); } - let default = merge_attribute_option( - attribute_expression_value(&field.attrs, "default")?, - property_options.default, - field, - "default", - )?; if nested && default.is_some() { return Err(Error::new_spanned( field, - "#[nested] fields obtain defaults from their own property annotations and cannot declare #[default(...)]", + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", )); } if !nested && default.is_none() { return Err(Error::new_spanned( field, - "Properties leaf fields must declare #[default(...)]", + "Properties leaf fields must declare default in #[property(...)]", )); } @@ -225,27 +203,14 @@ fn parse_property_field( if prefix.is_some() && map_value_type.is_none() { return Err(Error::new_spanned( &field.ty, - "#[prefix(...)] fields must have type HashMap", + "property prefix fields must have type HashMap", )); } - let parse_with = merge_attribute_option( - attribute_path_value(&field.attrs, "parse_with")?, - property_options.parse_with, - field, - "parse_with", - )?; - let parse_properties_with = merge_attribute_option( - attribute_path_value(&field.attrs, "parse_properties_with")?, - property_options.parse_properties_with, - field, - "parse_properties_with", - )?; - if additional_keys.is_some() && parse_properties_with.is_none() { return Err(Error::new_spanned( field, - "#[additional_keys(...)] requires parse_properties_with", + "additional_keys requires parse_properties_with in #[property(...)]", )); } if (prefix.is_some() || nested) @@ -253,7 +218,7 @@ fn parse_property_field( { return Err(Error::new_spanned( field, - "#[prefix(...)] and #[nested] fields do not support custom parse functions", + "prefix and nested fields do not support custom parse functions", )); } if parse_with.is_some() && parse_properties_with.is_some() { @@ -274,7 +239,7 @@ fn parse_property_field( parse_properties_with, option_inner_type: option_inner_type(&field.ty), map_value_type, - public_getter: property_options.public_getter, + public_getter, doc_attributes: field .attrs .iter() @@ -284,9 +249,12 @@ fn parse_property_field( }) } -fn property_options(attributes: &[Attribute]) -> syn::Result { - let Some(attribute) = find_attribute(attributes, "property")? else { - return Ok(PropertyOptions::default()); +fn property_options(field: &Field) -> syn::Result { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); }; let parsed = @@ -362,22 +330,6 @@ fn set_property_option( Ok(()) } -fn merge_attribute_option( - standalone: Option, - grouped: Option, - field: &Field, - name: &str, -) -> syn::Result> { - match (standalone, grouped) { - (Some(_), Some(_)) => Err(Error::new_spanned( - field, - format!("duplicate {name} property option"), - )), - (Some(value), None) | (None, Some(value)) => Ok(Some(value)), - (None, None) => Ok(None), - } -} - fn field_getter(field: &PropertyField) -> TokenStream2 { if !field.public_getter { return TokenStream2::new(); @@ -402,74 +354,6 @@ fn field_getter(field: &PropertyField) -> TokenStream2 { } } -fn marker_attribute(attributes: &[Attribute], name: &str) -> syn::Result { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(false); - }; - - match &attribute.meta { - Meta::Path(_) => Ok(true), - _ => Err(Error::new_spanned( - attribute, - format!("{name} must use the form #[{name}]"), - )), - } -} - -fn attribute_expression_value(attributes: &[Attribute], name: &str) -> syn::Result> { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(None); - }; - - match &attribute.meta { - Meta::NameValue(name_value) => Ok(Some(name_value.value.clone())), - Meta::List(_) => attribute.parse_args::().map(Some), - _ => Err(Error::new_spanned( - attribute, - format!("{name} must use the form #[{name}(...)]"), - )), - } -} - -fn attribute_expression_list( - attributes: &[Attribute], - name: &str, -) -> syn::Result>> { - let Some(attribute) = find_attribute(attributes, name)? else { - return Ok(None); - }; - - let expressions = match &attribute.meta { - Meta::NameValue(name_value) => expression_list(name_value.value.clone(), name)?, - Meta::List(_) => attribute - .parse_args_with(Punctuated::::parse_terminated)? - .into_iter() - .collect(), - _ => { - return Err(Error::new_spanned( - attribute, - format!("{name} must contain a non-empty list of keys"), - )); - } - }; - - if expressions.is_empty() { - return Err(Error::new_spanned( - attribute, - format!("{name} must contain at least one key"), - )); - } - Ok(Some(expressions)) -} - -fn attribute_path_value(attributes: &[Attribute], name: &str) -> syn::Result> { - let Some(expression) = attribute_expression_value(attributes, name)? else { - return Ok(None); - }; - - expression_path(expression, name).map(Some) -} - fn expression_path(expression: Expr, name: &str) -> syn::Result { match expression { Expr::Path(ExprPath { path, .. }) => Ok(path), diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 2eecde722e..12bb4f3322 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -55,9 +55,7 @@ fn parse_dimensions( #[derive(Debug, Properties)] struct TestProperties { - #[key(RETRIES)] - #[default(4)] - #[property(pub(getter))] + #[property(key = RETRIES, default = 4, pub(getter))] retries: u64, #[property(key = OWNER, default = None, pub(getter))] From 4f9f7da5997ef5e9902c43ab9bac76423f2a57e9 Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Mon, 10 Aug 2026 14:58:55 +0800 Subject: [PATCH 06/11] Address property macro review feedback --- Cargo.lock | 69 +++++++- crates/property-macro/Cargo.toml | 3 +- crates/property-macro/README.md | 44 +++-- crates/property-macro/public-api.txt | 2 - crates/property-macro/src/properties.rs | 166 +++++++++--------- ...dditional_keys_without_parse_properties.rs | 27 +++ ...ional_keys_without_parse_properties.stderr | 6 + .../tests/compile-fail/missing_property.rs | 26 +++ .../compile-fail/missing_property.stderr | 5 + .../tests/compile-fail/multiple_sources.rs | 29 +++ .../compile-fail/multiple_sources.stderr | 6 + .../tests/compile-fail/nested_default.rs | 33 ++++ .../tests/compile-fail/nested_default.stderr | 6 + ...arse_properties_without_additional_keys.rs | 38 ++++ ..._properties_without_additional_keys.stderr | 6 + .../tests/compile-fail/prefix_default.rs | 29 +++ .../tests/compile-fail/prefix_default.stderr | 6 + .../tests/compile-fail/prefix_non_map.rs | 27 +++ .../tests/compile-fail/prefix_non_map.stderr | 5 + crates/property-macro/tests/compile_fail.rs | 21 +++ crates/property-macro/tests/properties.rs | 81 +++++++-- 21 files changed, 521 insertions(+), 114 deletions(-) delete mode 100644 crates/property-macro/public-api.txt create mode 100644 crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.rs create mode 100644 crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.stderr create mode 100644 crates/property-macro/tests/compile-fail/missing_property.rs create mode 100644 crates/property-macro/tests/compile-fail/missing_property.stderr create mode 100644 crates/property-macro/tests/compile-fail/multiple_sources.rs create mode 100644 crates/property-macro/tests/compile-fail/multiple_sources.stderr create mode 100644 crates/property-macro/tests/compile-fail/nested_default.rs create mode 100644 crates/property-macro/tests/compile-fail/nested_default.stderr create mode 100644 crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs create mode 100644 crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.stderr create mode 100644 crates/property-macro/tests/compile-fail/prefix_default.rs create mode 100644 crates/property-macro/tests/compile-fail/prefix_default.stderr create mode 100644 crates/property-macro/tests/compile-fail/prefix_non_map.rs create mode 100644 crates/property-macro/tests/compile-fail/prefix_non_map.stderr create mode 100644 crates/property-macro/tests/compile_fail.rs diff --git a/Cargo.lock b/Cargo.lock index d48f9beb79..401df1a3c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4002,7 +4002,7 @@ dependencies = [ "mimalloc", "stacker", "tokio", - "toml", + "toml 0.8.23", "tracing", "tracing-subscriber", ] @@ -4016,6 +4016,7 @@ dependencies = [ "serde", "serde_json", "syn", + "trybuild", ] [[package]] @@ -4036,7 +4037,7 @@ dependencies = [ "serde", "sqllogictest", "tokio", - "toml", + "toml 0.8.23", "tracing", ] @@ -6933,6 +6934,15 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -7674,6 +7684,12 @@ version = "0.13.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "adb6935a6f5c20170eeceb1a3835a49e12e19d792f6dd344ccc76a985ca5a6ca" +[[package]] +name = "target-triple" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3a6bfce3d99adfa72d24750a61f782f3036a81e7f86d8841ee1326deaebd171" + [[package]] name = "tempfile" version = "3.27.0" @@ -7687,6 +7703,15 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "termcolor" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06794f8f6c5c898b3275aebefa6b8a1cb24cd2c6c79397ab15774837a0bc5755" +dependencies = [ + "winapi-util", +] + [[package]] name = "termtree" version = "0.5.1" @@ -7898,11 +7923,26 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dc1beb996b9d83529a9e75c17a1686767d148d70663143c7854d8b4a09ced362" dependencies = [ "serde", - "serde_spanned", + "serde_spanned 0.6.9", "toml_datetime 0.6.11", "toml_edit 0.22.27", ] +[[package]] +name = "toml" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53c96ecdfa941c8fc4fcaed14f99ada8ebed502eef533015095a07e3301d4c3c" +dependencies = [ + "indexmap 2.14.0", + "serde_core", + "serde_spanned 1.1.1", + "toml_datetime 1.1.1+spec-1.1.0", + "toml_parser", + "toml_writer", + "winnow 1.0.3", +] + [[package]] name = "toml_datetime" version = "0.6.11" @@ -7929,7 +7969,7 @@ checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a" dependencies = [ "indexmap 2.14.0", "serde", - "serde_spanned", + "serde_spanned 0.6.9", "toml_datetime 0.6.11", "toml_write", "winnow 0.7.15", @@ -7962,6 +8002,12 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801" +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -8101,6 +8147,21 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "trybuild" +version = "1.0.120" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1e605bf6b39357663d8ba4e984f8be8da8df6bb32e81031d6889024ea8fd68e4" +dependencies = [ + "glob", + "serde", + "serde_derive", + "serde_json", + "target-triple", + "termcolor", + "toml 1.1.3+spec-1.1.0", +] + [[package]] name = "twox-hash" version = "2.1.2" diff --git a/crates/property-macro/Cargo.toml b/crates/property-macro/Cargo.toml index 075cf73f63..4e9cbe54a2 100644 --- a/crates/property-macro/Cargo.toml +++ b/crates/property-macro/Cargo.toml @@ -19,7 +19,7 @@ edition = { workspace = true } homepage = { workspace = true } name = "iceberg-property-macro" -publish = true +publish = false readme = "README.md" rust-version = { workspace = true } version = { workspace = true } @@ -42,6 +42,7 @@ syn = { version = "2", features = ["full"] } [dev-dependencies] serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +trybuild = "1" [lints] workspace = true diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 7187e5ce26..a997abb08a 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -41,8 +41,10 @@ impl MyProperties { uses its annotated default when a property is absent. Unknown keys are ignored. An invalid value returns an error containing its primary property key. -Adding `pub(getter)` to a field generates an immutable accessor with the field -name. Structurally known `Copy` types return `T`; other types return `&T`. +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. @@ -61,7 +63,7 @@ use iceberg_property_macro::Properties; const RETRIES: &str = "commit.retry.num-retries"; const OWNER: &str = "owner"; -const FANOUT: &str = "write.fanout.enabled"; +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"; @@ -104,26 +106,27 @@ fn parse_dimensions( #[derive(Debug, Properties)] struct CommitProperties { /// Maximum number of times to retry a commit. - #[property(key = RETRIES, default = 4, pub(getter))] + #[property(key = RETRIES, default = 4, getter)] retries: usize, } #[derive(Debug, Properties)] struct TableLikeProperties { /// Nested groups parse from the same flat property map. - #[property(nested, pub(getter))] + #[property(nested, getter)] commit: CommitProperties, /// Option distinguishes an absent property from a present value. - #[property(key = OWNER, default = None, pub(getter))] + #[property(key = OWNER, default = None, getter)] owner: Option, - /// Boolean values are parsed case-insensitively. - #[property(key = FANOUT, default = true, pub(getter))] + /// 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, default = HashMap::new(), pub(getter))] + #[property(prefix = COLUMN_FPP_PREFIX, getter)] column_fpp: HashMap, /// A single-key parser can validate and normalize a property value. @@ -131,7 +134,7 @@ struct TableLikeProperties { key = LOCATION, default = "warehouse", parse_with = parse_location, - pub(getter) + getter )] location: String, @@ -141,7 +144,7 @@ struct TableLikeProperties { additional_keys = [HEIGHT, DEPTH], default = (640, 480, 320), parse_properties_with = parse_dimensions, - pub(getter) + getter )] dimensions: (u64, u64, u64), } @@ -203,7 +206,7 @@ struct ReadProperties { #[property( key = "commit.retry.num-retries", default = 4, - pub(getter) + getter )] retries: u64, } @@ -230,11 +233,20 @@ All field settings must be grouped under `#[property(...)]`. This keeps `key`, `default`, `prefix`, `nested`, parser hooks, additional keys, and getter generation in one attribute and avoids collisions with ordinary Rust derives. -The `prefix` setting requires `HashMap`. `nested` embeds another -`Properties` struct while reading the same flat map. `parse_with` customizes -parsing for one exact-key field. `parse_properties_with` receives the complete -property map, and `additional_keys` supplies its list of secondary keys. +Exact-key fields require a `default`. The `prefix` setting requires +`HashMap` and returns the entries matched by the prefix, or an empty +map when none match. `nested` embeds another `Properties` struct while reading +the same flat map. Neither prefix nor nested fields accept a `default`. +`parse_with` customizes parsing for one exact-key field; on an `Option` field, +the parser produces `T` and the macro wraps a present value in `Some`. +`parse_properties_with` receives the complete property map, and its required +`additional_keys` setting supplies the list of secondary keys. Boolean values are parsed case-insensitively. Other values require `FromStr` unless a custom parser is supplied. String-literal and path defaults are converted into their field type with `Into`. + +`from_properties` currently returns errors as `String` so this standalone macro +does not impose Iceberg's error type on catalog configuration consumers. The +crate remains unpublished until a production consumer establishes the final +error integration and public API. diff --git a/crates/property-macro/public-api.txt b/crates/property-macro/public-api.txt deleted file mode 100644 index 8edb5d0952..0000000000 --- a/crates/property-macro/public-api.txt +++ /dev/null @@ -1,2 +0,0 @@ -pub mod iceberg_property_macro -pub proc macro iceberg_property_macro::#[derive(Properties)] diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 5416aba19f..ae2e88a7c5 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -21,7 +21,7 @@ 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, parenthesized, + Ident, Lit, Path, PathArguments, Token, Type, }; struct PropertyField { @@ -40,8 +40,6 @@ struct PropertyField { doc_attributes: Vec, } -struct PublicGetter; - enum PropertyOption { Key(Expr), AdditionalKeys(Vec), @@ -50,7 +48,7 @@ enum PropertyOption { Default(Expr), ParseWith(Path), ParsePropertiesWith(Path), - Getter(PublicGetter), + Getter, } #[derive(Default)] @@ -65,35 +63,16 @@ struct PropertyOptions { public_getter: bool, } -impl Parse for PublicGetter { - fn parse(input: ParseStream<'_>) -> syn::Result { - input.parse::()?; - let content; - parenthesized!(content in input); - let accessor = content.parse::()?; - if !content.is_empty() { - return Err(content.error("expected getter")); - } - - if accessor == "getter" { - Ok(Self) - } else { - Err(Error::new_spanned(accessor, "expected getter")) - } - } -} - impl Parse for PropertyOption { fn parse(input: ParseStream<'_>) -> syn::Result { - if input.peek(Token![pub]) { - return input.parse().map(Self::Getter); - } - let name = input.parse::()?; let option_name = name.to_string(); if option_name == "nested" { return Ok(Self::Nested); } + if option_name == "getter" { + return Ok(Self::Getter); + } input.parse::()?; let expression = input.parse::()?; @@ -138,7 +117,10 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result .iter() .map(|field| parse_property_field(field, property_options(field)?)) .collect::>>()?; - let parses = fields.iter().map(parse_field); + let parses = fields + .iter() + .map(parse_field) + .collect::>>()?; let accessors = fields.iter().map(field_getter); let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); @@ -192,10 +174,16 @@ fn parse_property_field( "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", )); } - if !nested && default.is_none() { + if prefix.is_some() && default.is_some() { return Err(Error::new_spanned( field, - "Properties leaf fields must declare default in #[property(...)]", + "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(...)]", )); } @@ -213,6 +201,12 @@ fn parse_property_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()) { @@ -302,7 +296,7 @@ fn property_options(field: &Field) -> syn::Result { attribute, "parse_properties_with", )?, - PropertyOption::Getter(_) => { + PropertyOption::Getter => { if options.public_getter { return Err(Error::new_spanned(attribute, "duplicate property accessor")); } @@ -397,68 +391,82 @@ fn find_attribute<'a>( Ok(first) } -fn parse_field(field: &PropertyField) -> TokenStream2 { +fn parse_field(field: &PropertyField) -> syn::Result { let ident = &field.ident; if field.nested { let ty = &field.ty; - return quote!(#ident: <#ty>::from_properties(properties)?); + return Ok(quote!(#ident: <#ty>::from_properties(properties)?)); } let ty = &field.ty; - let default = typed_default(field); if let Some(parse_properties_with) = &field.parse_properties_with { - let key = field.key.as_ref().expect("exact-key fields have a key"); - let parse = match &field.additional_keys { - Some(additional_keys) => { - quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) - } - None => quote!(#parse_properties_with(properties, #key, #default)), - }; - return quote! { - #ident: #parse.map_err(|error| { + 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: #parse_properties_with( + properties, + #key, + &[#(#additional_keys),*], + #default, + ).map_err(|error| { format!("Invalid value for {}: {error}", #key) })? - }; + }); } if let Some(prefix) = &field.prefix { - let value_type = field - .map_value_type - .as_ref() - .expect("prefix fields are validated as maps"); + let value_type = field.map_value_type.as_ref().ok_or_else(|| { + Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap", + ) + })?; let parse = if is_bool(value_type) { quote!(value.to_ascii_lowercase().parse::<#value_type>()) } else { quote!(value.parse::<#value_type>()) }; - return quote! { - #ident: { - let parsed = properties - .iter() - .filter_map(|(key, value)| { - key.strip_prefix(#prefix).map(|suffix| { - #parse - .map(|parsed| (suffix.to_string(), parsed)) - .map_err(|error| format!("Invalid value for {key}: {error}")) - }) + 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| format!("Invalid value for {key}: {error}")) }) - .collect::<::std::result::Result< - ::std::collections::HashMap<_, _>, - ::std::string::String, - >>()?; - if parsed.is_empty() { - #default - } else { - parsed - } - } - }; + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()? + }); } - let key = field.key.as_ref().expect("exact-key fields have a key"); + 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), _) => quote! { + (Some(parse_with), Some(_)) => quote! { + Some(#parse_with(value).map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (Some(parse_with), None) => quote! { #parse_with(value).map_err(|error| { format!("Invalid value for {}: {error}", #key) })? @@ -485,24 +493,24 @@ fn parse_field(field: &PropertyField) -> TokenStream2 { }, }; - quote! { + Ok(quote! { #ident: match properties.get(#key) { Some(value) => #parse, None => #default, } - } + }) } -fn typed_default(field: &PropertyField) -> TokenStream2 { +fn typed_default(field: &PropertyField) -> syn::Result { let ty = &field.ty; - let default = default_value( - field.default.as_ref().expect("leaf fields have defaults"), - ty, - ); - quote!({ + 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 { diff --git a/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.rs b/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.rs new file mode 100644 index 0000000000..3036fd3b96 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.rs @@ -0,0 +1,27 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct AdditionalKeysWithoutParser { + #[property(key = "value", additional_keys = ["other"], default = 1)] + value: u64, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.stderr b/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.stderr new file mode 100644 index 0000000000..905ffb0043 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/additional_keys_without_parse_properties.stderr @@ -0,0 +1,6 @@ +error: additional_keys requires parse_properties_with in #[property(...)] + --> tests/compile-fail/additional_keys_without_parse_properties.rs:23:5 + | +23 | / #[property(key = "value", additional_keys = ["other"], default = 1)] +24 | | value: u64, + | |______________^ diff --git a/crates/property-macro/tests/compile-fail/missing_property.rs b/crates/property-macro/tests/compile-fail/missing_property.rs new file mode 100644 index 0000000000..07ed0f770b --- /dev/null +++ b/crates/property-macro/tests/compile-fail/missing_property.rs @@ -0,0 +1,26 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct MissingProperty { + value: u64, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/missing_property.stderr b/crates/property-macro/tests/compile-fail/missing_property.stderr new file mode 100644 index 0000000000..3555f484d6 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/missing_property.stderr @@ -0,0 +1,5 @@ +error: Properties fields must declare #[property(...)] + --> tests/compile-fail/missing_property.rs:23:5 + | +23 | value: u64, + | ^^^^^^^^^^ diff --git a/crates/property-macro/tests/compile-fail/multiple_sources.rs b/crates/property-macro/tests/compile-fail/multiple_sources.rs new file mode 100644 index 0000000000..97aefaa567 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/multiple_sources.rs @@ -0,0 +1,29 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct MultipleSources { + #[property(key = "value", prefix = "values.", default = HashMap::new())] + value: HashMap, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/multiple_sources.stderr b/crates/property-macro/tests/compile-fail/multiple_sources.stderr new file mode 100644 index 0000000000..2e4c67f868 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/multiple_sources.stderr @@ -0,0 +1,6 @@ +error: Properties fields must declare exactly one of key, prefix, or nested in #[property(...)] + --> tests/compile-fail/multiple_sources.rs:25:5 + | +25 | / #[property(key = "value", prefix = "values.", default = HashMap::new())] +26 | | value: HashMap, + | |_______________________________^ diff --git a/crates/property-macro/tests/compile-fail/nested_default.rs b/crates/property-macro/tests/compile-fail/nested_default.rs new file mode 100644 index 0000000000..601b1df1e4 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/nested_default.rs @@ -0,0 +1,33 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct Inner { + #[property(key = "value", default = 1)] + value: u64, +} + +#[derive(Properties)] +struct NestedDefault { + #[property(nested, default = unreachable!())] + inner: Inner, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/nested_default.stderr b/crates/property-macro/tests/compile-fail/nested_default.stderr new file mode 100644 index 0000000000..6db683674a --- /dev/null +++ b/crates/property-macro/tests/compile-fail/nested_default.stderr @@ -0,0 +1,6 @@ +error: nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)] + --> tests/compile-fail/nested_default.rs:29:5 + | +29 | / #[property(nested, default = unreachable!())] +30 | | inner: Inner, + | |________________^ diff --git a/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs new file mode 100644 index 0000000000..b87a3be11b --- /dev/null +++ b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs @@ -0,0 +1,38 @@ +// 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 iceberg_property_macro::Properties; + +fn parse_all( + _properties: &HashMap, + _key: &str, + _additional_keys: &[&str], + default: u64, +) -> Result { + Ok(default) +} + +#[derive(Properties)] +struct ParsePropertiesWithoutAdditionalKeys { + #[property(key = "value", default = 1, parse_properties_with = parse_all)] + value: u64, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.stderr b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.stderr new file mode 100644 index 0000000000..ff2168f92f --- /dev/null +++ b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.stderr @@ -0,0 +1,6 @@ +error: parse_properties_with requires additional_keys in #[property(...)] + --> tests/compile-fail/parse_properties_without_additional_keys.rs:34:5 + | +34 | / #[property(key = "value", default = 1, parse_properties_with = parse_all)] +35 | | value: u64, + | |______________^ diff --git a/crates/property-macro/tests/compile-fail/prefix_default.rs b/crates/property-macro/tests/compile-fail/prefix_default.rs new file mode 100644 index 0000000000..d7c4ec3397 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/prefix_default.rs @@ -0,0 +1,29 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct PrefixDefault { + #[property(prefix = "values.", default = HashMap::new())] + values: HashMap, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/prefix_default.stderr b/crates/property-macro/tests/compile-fail/prefix_default.stderr new file mode 100644 index 0000000000..9dcddded2d --- /dev/null +++ b/crates/property-macro/tests/compile-fail/prefix_default.stderr @@ -0,0 +1,6 @@ +error: prefix fields collect matching properties and cannot declare default in #[property(...)] + --> tests/compile-fail/prefix_default.rs:25:5 + | +25 | / #[property(prefix = "values.", default = HashMap::new())] +26 | | values: HashMap, + | |________________________________^ diff --git a/crates/property-macro/tests/compile-fail/prefix_non_map.rs b/crates/property-macro/tests/compile-fail/prefix_non_map.rs new file mode 100644 index 0000000000..1d904daece --- /dev/null +++ b/crates/property-macro/tests/compile-fail/prefix_non_map.rs @@ -0,0 +1,27 @@ +// 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 iceberg_property_macro::Properties; + +#[derive(Properties)] +struct PrefixNonMap { + #[property(prefix = "values.")] + value: u64, +} + +fn main() {} diff --git a/crates/property-macro/tests/compile-fail/prefix_non_map.stderr b/crates/property-macro/tests/compile-fail/prefix_non_map.stderr new file mode 100644 index 0000000000..72627bc592 --- /dev/null +++ b/crates/property-macro/tests/compile-fail/prefix_non_map.stderr @@ -0,0 +1,5 @@ +error: property prefix fields must have type HashMap + --> tests/compile-fail/prefix_non_map.rs:24:12 + | +24 | value: u64, + | ^^^ diff --git a/crates/property-macro/tests/compile_fail.rs b/crates/property-macro/tests/compile_fail.rs new file mode 100644 index 0000000000..3d692ce893 --- /dev/null +++ b/crates/property-macro/tests/compile_fail.rs @@ -0,0 +1,21 @@ +// 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. + +#[test] +fn reports_invalid_property_annotations() { + trybuild::TestCases::new().compile_fail("tests/compile-fail/*.rs"); +} diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index 12bb4f3322..c5e76f0a3f 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -16,6 +16,7 @@ // under the License. use std::collections::HashMap; +use std::str::FromStr; use iceberg_property_macro::Properties; use serde::{Deserialize, Serialize}; @@ -23,7 +24,7 @@ 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.fanout.enabled"; +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"; @@ -55,22 +56,21 @@ fn parse_dimensions( #[derive(Debug, Properties)] struct TestProperties { - #[property(key = RETRIES, default = 4, pub(getter))] + #[property(key = RETRIES, default = 4, getter)] retries: u64, - #[property(key = OWNER, default = None, pub(getter))] + #[property(key = OWNER, default = None, getter)] owner: Option, - #[property(key = FORMAT, default = "parquet", pub(getter))] + #[property(key = FORMAT, default = "parquet", getter)] format: String, - #[property(key = FANOUT_ENABLED, default = true, pub(getter))] + #[property(key = FANOUT_ENABLED, default = true, getter)] fanout_enabled: bool, #[property( prefix = COLUMN_FPP_PREFIX, - default = HashMap::new(), - pub(getter) + getter )] column_fpp: HashMap, @@ -79,7 +79,7 @@ struct TestProperties { additional_keys = [HEIGHT, DEPTH], default = (640, 480, 320), parse_properties_with = parse_dimensions, - pub(getter) + getter )] dimensions: (u64, u64, u64), } @@ -147,13 +147,13 @@ fn reports_the_property_with_an_invalid_value() { #[derive(Debug, Properties)] struct CommitProperties { /// Maximum number of times to retry a commit. - #[property(key = RETRIES, default = 4, pub(getter))] + #[property(key = RETRIES, default = 4, getter)] retries: u64, } #[derive(Debug, Properties)] struct NestedProperties { - #[property(nested, pub(getter))] + #[property(nested, getter)] commit: CommitProperties, } @@ -180,13 +180,16 @@ struct ValidatedProperties { key = "location", default = "default", parse_with = parse_non_empty, - pub(getter) + 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(), @@ -202,9 +205,63 @@ fn custom_single_value_parser_can_validate_and_normalize() { assert_eq!(error, "Invalid value for location: value must not be empty"); } +#[derive(Debug, Properties)] +struct OptionalValidatedProperties { + #[property( + key = "optional-location", + default = None, + parse_with = parse_non_empty, + getter + )] + location: Option, +} + +#[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")); +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct Meters(u64); + +impl FromStr for Meters { + type Err = ::Err; + + fn from_str(value: &str) -> Result { + value.parse().map(Self) + } +} + +#[derive(Debug, Properties)] +struct CopyGetterProperties { + #[property(key = "distance", default = Meters(1), getter)] + distance: Meters, + + #[property(key = "count", default = Some(2), getter)] + count: Option, +} + +#[test] +fn returns_only_structurally_known_copy_types_by_value() { + let properties = CopyGetterProperties::from_properties(&HashMap::new()).unwrap(); + + let _: &Meters = properties.distance(); + let _: Option = properties.count(); + assert_eq!(properties.distance(), &Meters(1)); + assert_eq!(properties.count(), Some(2)); +} + #[derive(Debug, Default, Serialize, Deserialize, Properties)] struct DerivedTraitProperties { - #[property(key = RETRIES, default = 4, pub(getter))] + #[property(key = RETRIES, default = 4, getter)] retries: u64, } From ccee9f8616a9f460c4bd52899ed5e424a4787a8a Mon Sep 17 00:00:00 2001 From: Renjie Liu Date: Mon, 10 Aug 2026 15:40:13 +0800 Subject: [PATCH 07/11] Use Iceberg errors in generated property API --- Cargo.lock | 1 + crates/iceberg/src/lib.rs | 1 + crates/property-macro/Cargo.toml | 1 + crates/property-macro/README.md | 19 +++++---- crates/property-macro/src/properties.rs | 49 +++++++++++++++++------ crates/property-macro/tests/properties.rs | 16 ++++++-- 6 files changed, 62 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 789d007b0d..8d939f2894 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4011,6 +4011,7 @@ dependencies = [ name = "iceberg-property-macro" version = "0.10.0" dependencies = [ + "iceberg", "proc-macro2", "quote", "serde", diff --git a/crates/iceberg/src/lib.rs b/crates/iceberg/src/lib.rs index 301992d15e..43473cc35e 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; +extern crate self as iceberg; mod error; pub use error::{Error, ErrorKind, Result}; diff --git a/crates/property-macro/Cargo.toml b/crates/property-macro/Cargo.toml index 4e9cbe54a2..bf82d3a911 100644 --- a/crates/property-macro/Cargo.toml +++ b/crates/property-macro/Cargo.toml @@ -40,6 +40,7 @@ quote = "1" syn = { version = "2", features = ["full"] } [dev-dependencies] +iceberg = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } trybuild = "1" diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index a997abb08a..9cea8f6b82 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -33,13 +33,14 @@ constructor: impl MyProperties { pub fn from_properties( properties: &HashMap, - ) -> Result; + ) -> iceberg::Result; } ``` `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 error containing its primary property key. +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 @@ -59,6 +60,7 @@ keys, and contextual errors. ```rust use std::collections::HashMap; +use iceberg::ErrorKind; use iceberg_property_macro::Properties; const RETRIES: &str = "commit.retry.num-retries"; @@ -149,7 +151,7 @@ struct TableLikeProperties { dimensions: (u64, u64, u64), } -fn main() -> Result<(), String> { +fn main() -> iceberg::Result<()> { let defaults = TableLikeProperties::from_properties(&HashMap::new())?; assert_eq!(defaults.commit().retries(), 4); assert_eq!(defaults.owner(), &None); @@ -183,7 +185,8 @@ fn main() -> Result<(), String> { "/".to_string(), )])) .unwrap_err(); - assert!(error.contains(LOCATION)); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert!(error.message().contains(LOCATION)); Ok(()) } @@ -246,7 +249,7 @@ Boolean values are parsed case-insensitively. Other values require `FromStr` unless a custom parser is supplied. String-literal and path defaults are converted into their field type with `Into`. -`from_properties` currently returns errors as `String` so this standalone macro -does not impose Iceberg's error type on catalog configuration consumers. The -crate remains unpublished until a production consumer establishes the final -error integration and public API. +`from_properties` uses `iceberg::Result`, and generated parse failures use +`ErrorKind::DataInvalid`. Custom parsers may return any error that implements +`Display`; the macro converts it to the same Iceberg error kind and adds the +primary property key to the message. diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index ae2e88a7c5..8fc299f9f5 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -133,7 +133,7 @@ pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result ::std::string::String, ::std::string::String, >, - ) -> ::std::result::Result { + ) -> ::iceberg::Result { Ok(Self { #(#parses,)* }) @@ -421,7 +421,10 @@ fn parse_field(field: &PropertyField) -> syn::Result { &[#(#additional_keys),*], #default, ).map_err(|error| { - format!("Invalid value for {}: {error}", #key) + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) })? }); } @@ -445,13 +448,15 @@ fn parse_field(field: &PropertyField) -> syn::Result { key.strip_prefix(#prefix).map(|suffix| { #parse .map(|parsed| (suffix.to_string(), parsed)) - .map_err(|error| format!("Invalid value for {key}: {error}")) + .map_err(|error| { + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {key}: {error}"), + ) + }) }) }) - .collect::<::std::result::Result< - ::std::collections::HashMap<_, _>, - ::std::string::String, - >>()? + .collect::<::iceberg::Result<::std::collections::HashMap<_, _>>>()? }); } @@ -463,32 +468,50 @@ fn parse_field(field: &PropertyField) -> syn::Result { let parse = match (&field.parse_with, &field.option_inner_type) { (Some(parse_with), Some(_)) => quote! { Some(#parse_with(value).map_err(|error| { - format!("Invalid value for {}: {error}", #key) + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) })?) }, (Some(parse_with), None) => quote! { #parse_with(value).map_err(|error| { - format!("Invalid value for {}: {error}", #key) + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) })? }, (None, Some(inner_type)) if is_bool(inner_type) => quote! { Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| { - format!("Invalid value for {}: {error}", #key) + ::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| { - format!("Invalid value for {}: {error}", #key) + ::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| { - format!("Invalid value for {}: {error}", #key) + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) })? }, (None, None) => quote! { value.parse::<#ty>().map_err(|error| { - format!("Invalid value for {}: {error}", #key) + ::iceberg::Error::new( + ::iceberg::ErrorKind::DataInvalid, + format!("Invalid value for {}: {error}", #key), + ) })? }, }; diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index c5e76f0a3f..cfa05c786d 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -18,6 +18,7 @@ use std::collections::HashMap; use std::str::FromStr; +use iceberg::ErrorKind; use iceberg_property_macro::Properties; use serde::{Deserialize, Serialize}; @@ -126,14 +127,16 @@ fn reports_the_property_with_an_invalid_value() { "many".to_string(), )])) .unwrap_err(); - assert!(numeric_error.contains(RETRIES)); + 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!(boolean_error.contains(FANOUT_ENABLED)); + 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([( @@ -141,7 +144,8 @@ fn reports_the_property_with_an_invalid_value() { "low".to_string(), )])) .unwrap_err(); - assert!(prefix_error.contains(&prefixed_key)); + assert_eq!(prefix_error.kind(), ErrorKind::DataInvalid); + assert!(prefix_error.message().contains(&prefixed_key)); } #[derive(Debug, Properties)] @@ -202,7 +206,11 @@ fn custom_single_value_parser_can_validate_and_normalize() { " ".to_string(), )])) .unwrap_err(); - assert_eq!(error, "Invalid value for location: value must not be empty"); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert_eq!( + error.message(), + "Invalid value for location: value must not be empty" + ); } #[derive(Debug, Properties)] From 6ea3bb54904a56b26f76729a59bc9eaa30891890 Mon Sep 17 00:00:00 2001 From: Renjie Liu <257669749+blackmwk@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:15:51 +0800 Subject: [PATCH 08/11] Require Iceberg results from property parsers --- crates/property-macro/README.md | 31 +++++++++----- crates/property-macro/src/properties.rs | 42 ++++++++----------- ...arse_properties_without_additional_keys.rs | 2 +- crates/property-macro/tests/properties.rs | 34 ++++++++++----- 4 files changed, 63 insertions(+), 46 deletions(-) diff --git a/crates/property-macro/README.md b/crates/property-macro/README.md index 9cea8f6b82..e85e9194b0 100644 --- a/crates/property-macro/README.md +++ b/crates/property-macro/README.md @@ -60,7 +60,7 @@ keys, and contextual errors. ```rust use std::collections::HashMap; -use iceberg::ErrorKind; +use iceberg::{Error, ErrorKind}; use iceberg_property_macro::Properties; const RETRIES: &str = "commit.retry.num-retries"; @@ -72,10 +72,13 @@ const WIDTH: &str = "dimensions.width"; const HEIGHT: &str = "dimensions.height"; const DEPTH: &str = "dimensions.depth"; -fn parse_location(value: &str) -> Result { +fn parse_location(value: &str) -> iceberg::Result { let location = value.trim().trim_end_matches('/'); if location.is_empty() { - Err("location must not be empty") + Err(Error::new( + ErrorKind::DataInvalid, + "location must not be empty", + )) } else { Ok(location.to_string()) } @@ -86,14 +89,21 @@ fn parse_dimensions( width_key: &str, additional_keys: &[&str], default: (u64, u64, u64), -) -> Result<(u64, u64, u64), String> { +) -> iceberg::Result<(u64, u64, u64)> { if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); + return Err(Error::new( + ErrorKind::DataInvalid, + "dimensions require height and depth keys", + )); } let parse = |key: &str, default| { properties .get(key) - .map(|value| value.parse::().map_err(|error| error.to_string())) + .map(|value| { + value.parse::().map_err(|error| { + Error::new(ErrorKind::DataInvalid, error.to_string()) + }) + }) .transpose() .map(|value| value.unwrap_or(default)) }; @@ -186,7 +196,7 @@ fn main() -> iceberg::Result<()> { )])) .unwrap_err(); assert_eq!(error.kind(), ErrorKind::DataInvalid); - assert!(error.message().contains(LOCATION)); + assert!(format!("{error}").contains(LOCATION)); Ok(()) } @@ -249,7 +259,6 @@ Boolean values are parsed case-insensitively. Other values require `FromStr` unless a custom parser is supplied. String-literal and path defaults are converted into their field type with `Into`. -`from_properties` uses `iceberg::Result`, and generated parse failures use -`ErrorKind::DataInvalid`. Custom parsers may return any error that implements -`Display`; the macro converts it to the same Iceberg error kind and adds the -primary property key to the message. +`from_properties` and both custom parser hooks use `iceberg::Result`. Generated +`FromStr` failures use `ErrorKind::DataInvalid`. The macro preserves errors from +custom parsers and adds the primary property key as error context. diff --git a/crates/property-macro/src/properties.rs b/crates/property-macro/src/properties.rs index 8fc299f9f5..03820311fb 100644 --- a/crates/property-macro/src/properties.rs +++ b/crates/property-macro/src/properties.rs @@ -415,17 +415,15 @@ fn parse_field(field: &PropertyField) -> syn::Result { })?; let default = typed_default(field)?; return Ok(quote! { - #ident: #parse_properties_with( - properties, - #key, - &[#(#additional_keys),*], - #default, - ).map_err(|error| { - ::iceberg::Error::new( - ::iceberg::ErrorKind::DataInvalid, - format!("Invalid value for {}: {error}", #key), - ) - })? + #ident: { + let parsed: ::iceberg::Result<#ty> = #parse_properties_with( + properties, + #key, + &[#(#additional_keys),*], + #default, + ); + parsed.map_err(|error| error.with_context("property", #key))? + } }); } @@ -466,21 +464,17 @@ fn parse_field(field: &PropertyField) -> syn::Result { .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(_)) => quote! { - Some(#parse_with(value).map_err(|error| { - ::iceberg::Error::new( - ::iceberg::ErrorKind::DataInvalid, - format!("Invalid value for {}: {error}", #key), - ) - })?) + (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! { - #parse_with(value).map_err(|error| { - ::iceberg::Error::new( - ::iceberg::ErrorKind::DataInvalid, - format!("Invalid value for {}: {error}", #key), - ) - })? + { + 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| { diff --git a/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs index b87a3be11b..96dd5b18d2 100644 --- a/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs +++ b/crates/property-macro/tests/compile-fail/parse_properties_without_additional_keys.rs @@ -25,7 +25,7 @@ fn parse_all( _key: &str, _additional_keys: &[&str], default: u64, -) -> Result { +) -> iceberg::Result { Ok(default) } diff --git a/crates/property-macro/tests/properties.rs b/crates/property-macro/tests/properties.rs index cfa05c786d..2ced3b7f73 100644 --- a/crates/property-macro/tests/properties.rs +++ b/crates/property-macro/tests/properties.rs @@ -18,7 +18,7 @@ use std::collections::HashMap; use std::str::FromStr; -use iceberg::ErrorKind; +use iceberg::{Error, ErrorKind}; use iceberg_property_macro::Properties; use serde::{Deserialize, Serialize}; @@ -36,14 +36,21 @@ fn parse_dimensions( width_key: &str, additional_keys: &[&str], default: (u64, u64, u64), -) -> Result<(u64, u64, u64), String> { +) -> iceberg::Result<(u64, u64, u64)> { if additional_keys.len() != 2 { - return Err("dimensions require height and depth keys".to_string()); + 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::().map_err(|error| error.to_string())) + .map(|value| { + value + .parse::() + .map_err(|error| Error::new(ErrorKind::DataInvalid, error.to_string())) + }) .transpose() .map(|value| value.unwrap_or(default)) }; @@ -146,6 +153,12 @@ fn reports_the_property_with_an_invalid_value() { .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)] @@ -169,10 +182,13 @@ fn nested_properties_read_the_same_flat_map() { assert_eq!(properties.commit().retries(), 9); } -fn parse_non_empty(value: &str) -> Result { +fn parse_non_empty(value: &str) -> iceberg::Result { let value = value.trim(); if value.is_empty() { - Err("value must not be empty") + Err(Error::new( + ErrorKind::DataInvalid, + "value must not be empty", + )) } else { Ok(value.to_string()) } @@ -207,10 +223,8 @@ fn custom_single_value_parser_can_validate_and_normalize() { )])) .unwrap_err(); assert_eq!(error.kind(), ErrorKind::DataInvalid); - assert_eq!( - error.message(), - "Invalid value for location: value must not be empty" - ); + assert_eq!(error.message(), "value must not be empty"); + assert!(format!("{error}").contains("property: location")); } #[derive(Debug, Properties)] From 993188f6f32b0862f8d670d2fe4832d97b144424 Mon Sep 17 00:00:00 2001 From: Renjie Liu <257669749+blackmwk@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:35:44 +0800 Subject: [PATCH 09/11] Manage property macro dependencies in workspace --- Cargo.toml | 4 ++++ crates/property-macro/Cargo.toml | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index bcdd080347..87fe8373d1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -126,7 +126,9 @@ ordered-float = "4" parquet = "58.4" pilota = "0.11.10" pretty_assertions = "1.4" +proc-macro2 = "1" pyo3 = "0.28" +quote = "1" rand = "0.9.3" regex = "1.11.3" reqwest = { version = "0.12.12", default-features = false, features = ["json"] } @@ -142,6 +144,7 @@ sqllogictest = "0.29" sqlx = { version = "0.8.1", default-features = false } stacker = "0.1.20" strum = "0.27.2" +syn = "2" tempfile = "3.18" thrift = "0.17.0" tokio = { version = "1.47", default-features = false, features = [ @@ -151,6 +154,7 @@ tokio = { version = "1.47", default-features = false, features = [ toml = "0.8" tracing = "0.1.41" tracing-subscriber = "0.3.20" +trybuild = "1" typed-builder = "0.20" typetag = "0.2" url = "2.5.7" diff --git a/crates/property-macro/Cargo.toml b/crates/property-macro/Cargo.toml index bf82d3a911..700b2ecf44 100644 --- a/crates/property-macro/Cargo.toml +++ b/crates/property-macro/Cargo.toml @@ -35,15 +35,15 @@ keywords = ["iceberg"] proc-macro = true [dependencies] -proc-macro2 = "1" -quote = "1" -syn = { version = "2", features = ["full"] } +proc-macro2 = { workspace = true } +quote = { workspace = true } +syn = { workspace = true, features = ["full"] } [dev-dependencies] iceberg = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } -trybuild = "1" +trybuild = { workspace = true } [lints] workspace = true From 1faa7fc1d5f6fbcba5e40b4a5ec66c5075e5d727 Mon Sep 17 00:00:00 2001 From: Renjie Liu <257669749+blackmwk@users.noreply.github.com> Date: Mon, 10 Aug 2026 18:44:15 +0800 Subject: [PATCH 10/11] Sort workspace members alphabetically --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 87fe8373d1..8e69861a35 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,9 +21,9 @@ members = [ "crates/catalog/*", "crates/examples", "crates/iceberg", - "crates/property-macro", "crates/integration_tests", "crates/integrations/*", + "crates/property-macro", "crates/sqllogictest", "crates/storage/*", "crates/test_utils", From 7435f86198085c3845d67379b78bb0c99bcdb3c5 Mon Sep 17 00:00:00 2001 From: Renjie Liu <257669749+blackmwk@users.noreply.github.com> Date: Mon, 10 Aug 2026 19:08:24 +0800 Subject: [PATCH 11/11] Exclude trybuild snapshots from license checks --- .licenserc.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.licenserc.yaml b/.licenserc.yaml index ea80177032..b25fbaceb0 100644 --- a/.licenserc.yaml +++ b/.licenserc.yaml @@ -31,6 +31,8 @@ header: - "**/DEPENDENCIES.*.tsv" # Generated content by cargo-public-api - "**/public-api.txt" + # Generated content by trybuild + - "**/*.stderr" # Release distributions - "dist/*" - "target"