laskoviymishka commented on code in PR #3044: URL: https://github.com/apache/iceberg-rust/pull/3044#discussion_r3873474401
########## 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: `new()` is hardcoded `pub` regardless of the struct's own visibility, so a `pub(crate)` or private view ends up with a constructor more visible than its type and can trip `unreachable_pub`. I'd emit the struct's `#visibility` here instead. Minor, and it mirrors an existing quirk in `from_properties`, but it's newly introduced on this path. ########## 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: Good to see the `collect::<Result<HashMap>>()?` fix in — this is exactly last round's bug. One gap: the test has only the single bad prefixed entry, so it doesn't actually prove the all-or-nothing behavior. If someone later refactored back to a filter-map-and-skip, the valid siblings would be silently dropped again and this test would still pass. I'd add a valid sibling alongside the bad one and assert `column_fpp()` still errors (and names the bad key) — that pins the short-circuit so the round-1 regression can't sneak back. ########## 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: One inconsistency with the derive macro here: in `#[derive(Properties)]`, `getter` always emits `pub fn`, but this branch only promotes to `pub` when the field visibility is `Inherited`. So `#[property(..., getter)] pub(crate) retries: usize` silently stays `pub(crate)` — the `getter` is swallowed with no diagnostic, and someone migrating from the derive expecting the promotion gets no compile-time signal. I'd either drop the `Inherited` guard so `getter` always wins (matching the derive), or reject `getter` on an explicit-visibility field with a `syn::Error`. Leaning toward the first — wdyt? ########## 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: The nested getter forwards the field type verbatim (`CommitPropertiesView<'_>`), so `'_` elides to the `&self` borrow rather than `'properties` — the returned view ends up tied to the parent view instead of the map. `let c = outer.commit()?; drop(outer); c.retries()` won't compile even though the map is still alive, and the README implies the opposite (one lifetime, just a borrow of the map). Since the view only holds `&'properties HashMap`, I'd thread the `'properties` ident through `view_field_getter` and emit `::iceberg::Result<NestedType<'properties>>` for the nested case. It compiles today via covariance so it's not a crash, but it's worth closing so the codegen matches what the docs promise. wdyt? -- 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]
