laskoviymishka commented on code in PR #3044:
URL: https://github.com/apache/iceberg-rust/pull/3044#discussion_r3862487349
##########
crates/property-macro/src/properties.rs:
##########
@@ -415,17 +437,28 @@ fn parse_field(field: &PropertyField) ->
syn::Result<TokenStream2> {
)
})?;
let default = typed_default(field)?;
- return Ok(quote! {
- #ident: {
+ let parse = if loosely {
+ quote! {
+ #parse_properties_with(
+ properties,
+ #key,
+ &[#(#additional_keys),*],
+ #default,
+ )
+ .unwrap_or_else(|_| #default)
Review Comment:
`#default` lands here twice — once passed into `parse_properties_with` as
the 4th arg, once in the `unwrap_or_else` closure — so on the error path it's
evaluated twice. Harmless for a literal, but a default expressed as a function
call would run twice with no signal.
I'd hoist it into a `let` binding above the call and reuse it.
##########
crates/property-macro/src/properties.rs:
##########
@@ -139,6 +143,19 @@ pub(crate) fn expand_properties(input: DeriveInput) ->
syn::Result<TokenStream2>
#(#parses,)*
})
}
+
+ /// Parses this typed property set from a flat string-to-string
map,
+ /// using each field's default when that field cannot be parsed.
+ pub fn try_from_loosely(
Review Comment:
every arm in the loose path is infallible (`unwrap_or_else` /
`unwrap_or_default`, and the nested `try_from_loosely` is itself always-`Ok`),
so this returns `Result<Self>` but can never return `Err`. The `try_` prefix
and the doc's "when that field cannot be parsed" both imply a fallibility that
isn't there — callers will `.unwrap()` on something that can't panic.
I'd either return `Self` and rename it (`from_properties_lossy`?), or keep
`Result<Self>` for future-proofing but document that it always succeeds today.
##########
crates/iceberg/public-api.txt:
##########
@@ -2884,6 +2884,7 @@ pub fn
iceberg::spec::TableProperties::parquet_dict_size_bytes(&self) -> usize
pub fn iceberg::spec::TableProperties::parquet_page_row_limit(&self) -> usize
pub fn iceberg::spec::TableProperties::parquet_page_size_bytes(&self) -> usize
pub fn iceberg::spec::TableProperties::parquet_row_group_size_bytes(&self) ->
usize
+pub fn iceberg::spec::TableProperties::try_from_loosely(properties:
&std::collections::hash::map::HashMap<alloc::string::String,
alloc::string::String>) -> iceberg::Result<Self>
Review Comment:
exposing this at `iceberg::spec::TableProperties` makes it the "easy" path
for anyone who just wants to dodge a parse error — and `TableProperties` is
exactly the struct that drives Parquet params, commit retry, and write
locations. A caller reaching for it to avoid a crash silently inherits wrong
write settings.
If we keep a loose variant, I'd lean toward exposing it only on structs that
opt in, or splitting advisory props (safe to default) from write-path ones,
rather than blanket-applying it to the primary write-config struct. This ties
into CTTY's thread — wdyt?
##########
crates/property-macro/src/properties.rs:
##########
@@ -455,60 +488,101 @@ fn parse_field(field: &PropertyField) ->
syn::Result<TokenStream2> {
})
})
})
- .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()?
- });
+ .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()
+ };
+ let parse = if loosely {
+ quote!(#parse.unwrap_or_default())
Review Comment:
this one looks like a straight bug independent of the design question.
`collect()` into a single `Result` short-circuits on the first `Err`, and
`unwrap_or_default()` then throws away the *whole* map — not just the offending
entry. One malformed key under a shared prefix (per-column bloom-filter FPP,
say) silently zeroes every valid sibling under that prefix.
I'd skip only the failing entry in loose mode rather than poisoning the
collection — move the `.ok()` inside the `filter_map` so a bad value drops just
its own key. Nothing currently tests this with more than one entry, so the
regression is invisible (see my note on the test).
##########
crates/property-macro/tests/properties.rs:
##########
@@ -161,6 +161,27 @@ fn reports_the_property_with_an_invalid_value() {
assert!(format!("{dimensions_error}").contains(WIDTH));
}
+#[test]
+fn loose_parsing_defaults_only_invalid_fields() {
Review Comment:
every new loose test asserts invalid→default, but none asserts that valid
input actually parses *through* the loose path. If a bug made
`try_from_loosely` always return defaults, all of these would still pass.
I'd add a case with all-valid values (`RETRIES=8` → `retries()==8`, a
location that trims to a real path) so we're pinning both directions.
##########
crates/property-macro/src/properties.rs:
##########
@@ -455,60 +488,101 @@ fn parse_field(field: &PropertyField) ->
syn::Result<TokenStream2> {
})
})
})
- .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()?
- });
+ .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()
+ };
+ let parse = if loosely {
+ quote!(#parse.unwrap_or_default())
+ } else {
+ quote!(#parse?)
+ };
+ return Ok(quote!(#ident: #parse));
}
let key = field
.key
.as_ref()
.ok_or_else(|| Error::new_spanned(&field.ident, "property fields must
declare key"))?;
let default = typed_default(field)?;
- let parse = match (&field.parse_with, &field.option_inner_type) {
- (Some(parse_with), Some(inner_type)) => quote! {
- {
- let parsed: ::iceberg::Result<#inner_type> =
#parse_with(value);
- Some(parsed.map_err(|error| error.with_context("property",
#key))?)
- }
- },
- (Some(parse_with), None) => quote! {
- {
- let parsed: ::iceberg::Result<#ty> = #parse_with(value);
- parsed.map_err(|error| error.with_context("property", #key))?
- }
- },
- (None, Some(inner_type)) if is_bool(inner_type) => quote! {
-
Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, Some(inner_type)) => quote! {
- Some(value.parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, None) if is_bool(ty) => quote! {
- value.to_ascii_lowercase().parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
- (None, None) => quote! {
- value.parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
+ let parse = if loosely {
+ match (&field.parse_with, &field.option_inner_type) {
+ (Some(parse_with), Some(inner_type)) => quote! {
+ #parse_with(value)
+ .map(|parsed: #inner_type| Some(parsed))
+ .unwrap_or_else(|_| #default)
+ },
+ (Some(parse_with), None) => quote! {
+ #parse_with(value).unwrap_or_else(|_| #default)
+ },
+ (None, Some(inner_type)) if is_bool(inner_type) => quote! {
+ value
+ .to_ascii_lowercase()
+ .parse::<#inner_type>()
+ .map(Some)
+ .unwrap_or_else(|_| #default)
+ },
+ (None, Some(inner_type)) => quote! {
+ value
+ .parse::<#inner_type>()
+ .map(Some)
+ .unwrap_or_else(|_| #default)
+ },
+ (None, None) if is_bool(ty) => quote! {
+ value
+ .to_ascii_lowercase()
+ .parse::<#ty>()
+ .unwrap_or_else(|_| #default)
+ },
+ (None, None) => quote! {
+ value.parse::<#ty>().unwrap_or_else(|_| #default)
Review Comment:
this is CTTY's concern made concrete: a present-but-invalid value silently
becomes the default here. `commit.retry.num-retries=abc` fails to parse and the
table quietly runs with 4 retries — the user set the key, believes it's active,
and gets no error or log.
Worth noting `from_properties` already returns the default when the key is
*absent*; the only new behavior this adds is swallowing parse errors on values
that *are* set — which is exactly the case I'd argue should stay an error.
Happy to be convinced there's a real use case, but I'd want it spelled out on
CTTY's thread first rather than replied here.
##########
crates/property-macro/tests/properties.rs:
##########
@@ -161,6 +161,27 @@ fn reports_the_property_with_an_invalid_value() {
assert!(format!("{dimensions_error}").contains(WIDTH));
}
+#[test]
+fn loose_parsing_defaults_only_invalid_fields() {
+ 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(), "sometimes".to_string()),
+ (format!("{COLUMN_FPP_PREFIX}id"), "low".to_string()),
+ (WIDTH.to_string(), "wide".to_string()),
+ ]);
+
+ let properties = TestProperties::try_from_loosely(&raw).unwrap();
+
+ assert_eq!(properties.retries(), 4);
+ assert_eq!(properties.owner().as_deref(), Some("iceberg"));
+ assert_eq!(properties.format(), "orc");
+ assert!(properties.fanout_enabled());
+ assert!(properties.column_fpp().is_empty());
Review Comment:
with a single prefix entry this assertion can't tell "discard everything on
first error" apart from "skip only the bad entry" — both leave `column_fpp`
empty, so the all-or-nothing bug I flagged on `properties.rs:494` slips right
through here. I'd add a second, valid `COLUMN_FPP` entry (e.g. `0.01`) and
assert it survives alongside the bad one.
##########
crates/property-macro/src/properties.rs:
##########
@@ -455,60 +488,101 @@ fn parse_field(field: &PropertyField) ->
syn::Result<TokenStream2> {
})
})
})
- .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()?
- });
+ .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()
+ };
+ let parse = if loosely {
+ quote!(#parse.unwrap_or_default())
+ } else {
+ quote!(#parse?)
+ };
+ return Ok(quote!(#ident: #parse));
}
let key = field
.key
.as_ref()
.ok_or_else(|| Error::new_spanned(&field.ident, "property fields must
declare key"))?;
let default = typed_default(field)?;
- let parse = match (&field.parse_with, &field.option_inner_type) {
- (Some(parse_with), Some(inner_type)) => quote! {
- {
- let parsed: ::iceberg::Result<#inner_type> =
#parse_with(value);
- Some(parsed.map_err(|error| error.with_context("property",
#key))?)
- }
- },
- (Some(parse_with), None) => quote! {
- {
- let parsed: ::iceberg::Result<#ty> = #parse_with(value);
- parsed.map_err(|error| error.with_context("property", #key))?
- }
- },
- (None, Some(inner_type)) if is_bool(inner_type) => quote! {
-
Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, Some(inner_type)) => quote! {
- Some(value.parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, None) if is_bool(ty) => quote! {
- value.to_ascii_lowercase().parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
- (None, None) => quote! {
- value.parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
+ let parse = if loosely {
Review Comment:
the `loosely` flag forks this whole six-arm match into two near-identical
copies that differ only in the error-handling tail — and the same split shows
up in the nested / `parse_properties_with` / prefix branches above. Any new
field category has to be updated in both halves, and forgetting one diverges
strict vs loose with no compile error.
Could we extract the raw parse expression per arm once, then apply a
strict-wrapper vs loose-wrapper over it? Would roughly halve this function.
wdyt?
##########
crates/property-macro/src/properties.rs:
##########
@@ -455,60 +488,101 @@ fn parse_field(field: &PropertyField) ->
syn::Result<TokenStream2> {
})
})
})
- .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()?
- });
+ .collect::<::iceberg::Result<::std::collections::HashMap<_,
_>>>()
+ };
+ let parse = if loosely {
+ quote!(#parse.unwrap_or_default())
+ } else {
+ quote!(#parse?)
+ };
+ return Ok(quote!(#ident: #parse));
}
let key = field
.key
.as_ref()
.ok_or_else(|| Error::new_spanned(&field.ident, "property fields must
declare key"))?;
let default = typed_default(field)?;
- let parse = match (&field.parse_with, &field.option_inner_type) {
- (Some(parse_with), Some(inner_type)) => quote! {
- {
- let parsed: ::iceberg::Result<#inner_type> =
#parse_with(value);
- Some(parsed.map_err(|error| error.with_context("property",
#key))?)
- }
- },
- (Some(parse_with), None) => quote! {
- {
- let parsed: ::iceberg::Result<#ty> = #parse_with(value);
- parsed.map_err(|error| error.with_context("property", #key))?
- }
- },
- (None, Some(inner_type)) if is_bool(inner_type) => quote! {
-
Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, Some(inner_type)) => quote! {
- Some(value.parse::<#inner_type>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?)
- },
- (None, None) if is_bool(ty) => quote! {
- value.to_ascii_lowercase().parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
- (None, None) => quote! {
- value.parse::<#ty>().map_err(|error| {
- ::iceberg::Error::new(
- ::iceberg::ErrorKind::DataInvalid,
- format!("Invalid value for {}: {error}", #key),
- )
- })?
- },
+ let parse = if loosely {
+ match (&field.parse_with, &field.option_inner_type) {
+ (Some(parse_with), Some(inner_type)) => quote! {
+ #parse_with(value)
+ .map(|parsed: #inner_type| Some(parsed))
Review Comment:
`.map(|parsed: #inner_type| Some(parsed))` trips
`clippy::redundant_closure_for_method_calls`, and since this expands into
downstream crates, any consumer running `cargo clippy -- -D warnings` on an
`Option<T>` field with `parse_with` would fail to build. The sibling arms
already use `.map(Some)` — I'd match them here:
```rust
#parse_with(value).map(Some).unwrap_or_else(|_| #default)
```
--
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]
