blackmwk commented on code in PR #3044: URL: https://github.com/apache/iceberg-rust/pull/3044#discussion_r3877112381
########## crates/property-macro/src/properties_view.rs: ########## @@ -0,0 +1,100 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::{Error, Fields, ItemStruct, Visibility}; + +use crate::properties::{ + ParseTarget, PropertyField, parse_field_value, parse_property_field, property_options, +}; + +pub(crate) fn expand_properties_view(input: ItemStruct) -> syn::Result<TokenStream2> { + if !input.generics.params.is_empty() || input.generics.where_clause.is_some() { + return Err(Error::new_spanned( + input.generics, + "properties_view! does not support generic declarations", + )); + } + + let attributes = input.attrs; + let visibility = input.vis; + let struct_name = input.ident; + let fields = match input.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "properties_view! requires a struct-shaped declaration with named fields", + )); + } + }; + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let getters = fields + .iter() + .map(view_field_getter) + .collect::<syn::Result<Vec<_>>>()?; + + Ok(quote! { + #(#attributes)* + #visibility struct #struct_name<'properties> { + properties: &'properties ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + } + + impl<'properties> #struct_name<'properties> { + /// Creates a property view without parsing any values. + pub fn new( + properties: &'properties ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> Self { + Self { properties } + } + + #(#getters)* + } + }) +} + +fn view_field_getter(field: &PropertyField) -> syn::Result<TokenStream2> { + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + let visibility = if field.public_getter && matches!(field.visibility, Visibility::Inherited) { + quote!(pub) + } else { + let visibility = &field.visibility; + quote!(#visibility) + }; + let parse = parse_field_value(field, ParseTarget::View)?; + + Ok(quote! { + #(#docs)* + #visibility fn #ident(&self) -> ::iceberg::Result<#ty> { Review Comment: Done. The generated properties lifetime is now threaded into nested getter generation, and the nested return type uses that source-map lifetime. The integration test now lets the parent view go out of scope before calling the nested getter, and the README documents this behavior. ########## crates/property-macro/src/properties_view.rs: ########## @@ -0,0 +1,100 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::{Error, Fields, ItemStruct, Visibility}; + +use crate::properties::{ + ParseTarget, PropertyField, parse_field_value, parse_property_field, property_options, +}; + +pub(crate) fn expand_properties_view(input: ItemStruct) -> syn::Result<TokenStream2> { + if !input.generics.params.is_empty() || input.generics.where_clause.is_some() { + return Err(Error::new_spanned( + input.generics, + "properties_view! does not support generic declarations", + )); + } + + let attributes = input.attrs; + let visibility = input.vis; + let struct_name = input.ident; + let fields = match input.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "properties_view! requires a struct-shaped declaration with named fields", + )); + } + }; + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let getters = fields + .iter() + .map(view_field_getter) + .collect::<syn::Result<Vec<_>>>()?; + + Ok(quote! { + #(#attributes)* + #visibility struct #struct_name<'properties> { + properties: &'properties ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + } + + impl<'properties> #struct_name<'properties> { + /// Creates a property view without parsing any values. + pub fn new( Review Comment: Done. The generated new() method now uses the struct declaration's visibility. I also added a code-generation test covering a pub(crate) view. ########## crates/property-macro/tests/properties_view.rs: ########## @@ -0,0 +1,194 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; +use std::mem::size_of; + +use iceberg::{Error, ErrorKind}; +use iceberg_property_macro::properties_view; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.datafusion.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_dimensions( + properties: &HashMap<String, String>, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> iceberg::Result<(u64, u64, u64)> { + if additional_keys.len() != 2 { + return Err(Error::new( + ErrorKind::DataInvalid, + "dimensions require height and depth keys", + )); + } + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| { + value + .parse::<u64>() + .map_err(|error| Error::new(ErrorKind::DataInvalid, error.to_string())) + }) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +fn parse_non_empty(value: &str) -> iceberg::Result<String> { + let value = value.trim(); + if value.is_empty() { + Err(Error::new( + ErrorKind::DataInvalid, + "value must not be empty", + )) + } else { + Ok(value.to_string()) + } +} + +properties_view! { + #[derive(Debug)] + struct TestPropertiesView { + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4)] + pub retries: u64, + + #[property(key = OWNER, default = None)] + pub owner: Option<String>, + + #[property(key = FORMAT, default = "parquet")] + pub format: String, + + #[property(key = FANOUT_ENABLED, default = true)] + pub fanout_enabled: bool, + + #[property(prefix = COLUMN_FPP_PREFIX)] + pub column_fpp: HashMap<String, f64>, + + #[property( + key = "location", + default = "default", + parse_with = parse_non_empty + )] + pub location: String, + + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions + )] + pub dimensions: (u64, u64, u64), + } +} + +properties_view! { + #[derive(Debug)] + struct CommitPropertiesView { + #[property(key = RETRIES, default = 4)] + pub retries: u64, + } +} + +properties_view! { + #[derive(Debug)] + struct NestedPropertiesView { + #[property(nested)] + pub commit: CommitPropertiesView<'_>, + } +} + +#[test] +fn property_view_is_only_a_reference_to_the_source_map() { + assert_eq!( + size_of::<TestPropertiesView<'_>>(), + size_of::<&HashMap<String, String>>() + ); +} + +#[test] +fn property_view_parses_only_the_requested_field() { + let raw = HashMap::from([ + (RETRIES.to_string(), "many".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ]); + let properties = TestPropertiesView::new(&raw); + + let error = properties.retries().unwrap_err(); + assert_eq!(error.kind(), ErrorKind::DataInvalid); + assert!(error.message().contains(RETRIES)); + assert_eq!(properties.owner().unwrap().as_deref(), Some("iceberg")); + assert_eq!(properties.format().unwrap(), "orc"); + assert!(!properties.fanout_enabled().unwrap()); + assert_eq!(properties.dimensions().unwrap(), (1920, 1080, 720)); +} + +#[test] +fn property_view_uses_defaults_and_supports_custom_parsers() { + let raw = HashMap::from([("location".to_string(), " path ".to_string())]); + let properties = TestPropertiesView::new(&raw); + + assert_eq!(properties.retries().unwrap(), 4); + assert_eq!(properties.owner().unwrap(), None); + assert_eq!(properties.format().unwrap(), "parquet"); + assert!(properties.fanout_enabled().unwrap()); + assert!(properties.column_fpp().unwrap().is_empty()); + assert_eq!(properties.location().unwrap(), "path"); +} + +#[test] +fn nested_property_views_borrow_the_same_source_map() { + let raw = HashMap::from([(RETRIES.to_string(), "9".to_string())]); + let properties = NestedPropertiesView::new(&raw); + + assert_eq!(properties.commit().unwrap().retries().unwrap(), 9); +} + +#[test] +fn property_view_reports_errors_when_the_corresponding_getter_is_called() { + let prefixed_key = format!("{COLUMN_FPP_PREFIX}id"); + let raw = HashMap::from([ Review Comment: Done. The prefix regression now includes a valid sibling alongside the invalid entry and still asserts that column_fpp() errors and identifies the invalid key. ########## crates/property-macro/src/properties_view.rs: ########## @@ -0,0 +1,100 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::{Error, Fields, ItemStruct, Visibility}; + +use crate::properties::{ + ParseTarget, PropertyField, parse_field_value, parse_property_field, property_options, +}; + +pub(crate) fn expand_properties_view(input: ItemStruct) -> syn::Result<TokenStream2> { + if !input.generics.params.is_empty() || input.generics.where_clause.is_some() { + return Err(Error::new_spanned( + input.generics, + "properties_view! does not support generic declarations", + )); + } + + let attributes = input.attrs; + let visibility = input.vis; + let struct_name = input.ident; + let fields = match input.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "properties_view! requires a struct-shaped declaration with named fields", + )); + } + }; + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let getters = fields + .iter() + .map(view_field_getter) + .collect::<syn::Result<Vec<_>>>()?; + + Ok(quote! { + #(#attributes)* + #visibility struct #struct_name<'properties> { + properties: &'properties ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + } + + impl<'properties> #struct_name<'properties> { + /// Creates a property view without parsing any values. + pub fn new( + properties: &'properties ::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> Self { + Self { properties } + } + + #(#getters)* + } + }) +} + +fn view_field_getter(field: &PropertyField) -> syn::Result<TokenStream2> { + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + let visibility = if field.public_getter && matches!(field.visibility, Visibility::Inherited) { Review Comment: Agreed. I dropped the inherited-visibility guard, so the getter option now always emits a public method, matching the derive macro. A code-generation test covers getter on an explicitly pub(crate) declaration. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
