laskoviymishka commented on code in PR #2928:
URL: https://github.com/apache/iceberg-rust/pull/2928#discussion_r3966656397
##########
crates/iceberg/src/avro/schema.rs:
##########
@@ -604,9 +587,8 @@ pub(crate) fn avro_schema_to_schema(avro_schema:
&AvroSchema) -> Result<Schema>
))
}
} else {
- Err(Error::new(
- ErrorKind::DataInvalid,
- "Can't convert non record avro schema to iceberg schema:
{avro_schema}",
+ Err(invalid_data!(
+ "Can't convert non record avro schema to iceberg schema:
{avro_schema}"
Review Comment:
This is the one spot where the rewrite changes behavior. Before,
`{avro_schema}` sat inside a plain `&str`, so the braces printed verbatim and
the substitution never happened — a latent bug. The macro's literal arm wraps
the string in `format!`, so now it actually interpolates `avro_schema` via
`Display`.
It's a better message, and it only compiles because `AvroSchema: Display`
holds, so nothing's broken. But it's a silent semantic change on a cleanup
that's meant to be behavior-preserving. I'd make it deliberate — note it in the
PR description, or switch to `{avro_schema:?}` — so it reads as an intentional
fix rather than an accident.
##########
crates/iceberg/src/arrow/record_batch_projector.rs:
##########
@@ -97,12 +97,9 @@ impl RecordBatchProjector {
let field_id_fetch_func = |field: &Field| -> Result<Option<i64>> {
if let Some(value) =
field.metadata().get(PARQUET_FIELD_ID_META_KEY) {
let field_id = value.parse::<i32>().map_err(|e| {
- Error::new(
- ErrorKind::DataInvalid,
- "Failed to parse field id".to_string(),
- )
- .with_context("value", value)
- .with_source(e)
+ invalid_data!("Failed to parse field id".to_string())
Review Comment:
The `.to_string()` is redundant here — the literal arm already runs the
message through `format!`, so `invalid_data!("Failed to parse field id")`
produces the exact same allocation. Dropping the suffix keeps it on the more
idiomatic literal arm.
Same shape in about six other spots: `arrow/schema.rs` (the field-id and
decimal-type branches), `spec/manifest/mod.rs` (both the serialize and
deserialize sites), `spec/values/datum.rs` (the AboveMax/BelowMin arm), and
`spec/schema/prune_columns.rs`.
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -851,7 +806,7 @@ pub(crate) fn get_arrow_datum(datum: &Datum) ->
Result<Arc<dyn ArrowDatum + Send
}
(PrimitiveType::Fixed(_), PrimitiveLiteral::Binary(value)) => {
let array =
FixedSizeBinaryArray::try_from_iter(std::iter::once(value.as_slice()))
- .map_err(|e| Error::new(ErrorKind::DataInvalid,
e.to_string()))?;
+ .map_err(|e| invalid_data!(e.to_string()))?;
Review Comment:
`invalid_data!(e.to_string())` stringifies the source and drops the
structured chain, so `err.source()` comes back `None`. Not a regression — the
old code did the same — but since we're on the line,
`invalid_data!("{e}").with_source(e)` keeps the chain and matches the many
sites that already do `.with_source`. Same story for the other `e.to_string()`
spots that don't chain the source.
##########
crates/iceberg/src/error.rs:
##########
@@ -469,6 +469,42 @@ macro_rules! ensure_data_valid {
};
}
+/// Helper macro to construct an [`ErrorKind::DataInvalid`] error.
+///
+/// This is a shorthand for `Error::new(ErrorKind::DataInvalid, ...)`, the most
+/// common error constructed in this crate. It returns the [`Error`] value (it
+/// does *not* return from the enclosing function), so it composes with `?`,
+/// `.map_err(...)`, `.ok_or_else(...)`, and explicit `return Err(...)`.
+///
+/// The message may be a plain expression or a format string with arguments.
+///
+/// # Examples
+///
+///
+/// ```ignore
+/// use crate::error::invalid_data;
+///
+/// // As an expression
+/// let err = invalid_data!("unexpected value: {value}");
+///
+/// // With `.ok_or_else`
+/// let field = fields.get(id).ok_or_else(|| invalid_data!("missing field
{id}"))?;
+///
+/// // Attaching a source error
+/// let n: i32 = s.parse().map_err(|e| invalid_data!("not an int:
{s}").with_source(e))?;
+/// ```
+macro_rules! invalid_data {
+ ($fmt: literal $(, $($arg:tt)*)?) => {
Review Comment:
One refinement on the literal arm: `invalid_data!("static message")` expands
to `format!("static message")`, which clippy's `useless_format` can flag — and
with `-D warnings` in the Makefile that could bite CI depending on toolchain.
Worth confirming CI is green here, since clippy's firing span through macros
varies by version.
If it does fire, a dedicated no-arg arm sidesteps it and doubles as the
canonical form for the bare-literal sites:
```rust
($fmt: literal) => {
$crate::error::Error::new($crate::error::ErrorKind::DataInvalid, $fmt)
};
($fmt: literal, $($arg:tt)*) => {
$crate::error::Error::new($crate::error::ErrorKind::DataInvalid,
format!($fmt, $($arg)*))
};
($msg: expr $(,)?) => { /* unchanged */ };
```
That way `invalid_data!("...")` skips `format!` entirely, and the leftover
`.to_string()` calls just become bare literals. wdyt?
##########
crates/iceberg/src/error.rs:
##########
@@ -532,6 +568,25 @@ Source: networking error
)
}
+ #[test]
+ fn test_invalid_data_macro() {
Review Comment:
The test covers the literal arm nicely but not the `expr` arm directly —
it's only exercised via the migrated call sites compiling. A quick `let s =
String::from("computed"); assert_eq!(invalid_data!(s).message(), "computed");`
would pin it down.
##########
crates/iceberg/src/error.rs:
##########
@@ -469,6 +469,42 @@ macro_rules! ensure_data_valid {
};
}
+/// Helper macro to construct an [`ErrorKind::DataInvalid`] error.
+///
+/// This is a shorthand for `Error::new(ErrorKind::DataInvalid, ...)`, the most
+/// common error constructed in this crate. It returns the [`Error`] value (it
+/// does *not* return from the enclosing function), so it composes with `?`,
+/// `.map_err(...)`, `.ok_or_else(...)`, and explicit `return Err(...)`.
+///
+/// The message may be a plain expression or a format string with arguments.
+///
+/// # Examples
+///
+///
+/// ```ignore
+/// use crate::error::invalid_data;
+///
+/// // As an expression
+/// let err = invalid_data!("unexpected value: {value}");
+///
+/// // With `.ok_or_else`
+/// let field = fields.get(id).ok_or_else(|| invalid_data!("missing field
{id}"))?;
+///
+/// // Attaching a source error
+/// let n: i32 = s.parse().map_err(|e| invalid_data!("not an int:
{s}").with_source(e))?;
+/// ```
+macro_rules! invalid_data {
+ ($fmt: literal $(, $($arg:tt)*)?) => {
+ $crate::error::Error::new($crate::error::ErrorKind::DataInvalid,
format!($fmt $(, $($arg)*)?))
+ };
+ ($msg: expr $(,)?) => {
+ $crate::error::Error::new($crate::error::ErrorKind::DataInvalid, $msg)
+ };
+}
+
+// Crate-internal macro: re-exported so other modules can `use
crate::error::invalid_data;`.
+pub(crate) use invalid_data;
Review Comment:
Small thing while we're here: `ensure_data_valid!` just above is
`#[macro_export]` but `invalid_data!` is `pub(crate)`. That's a defensible
split, but nothing says so, and a future contributor could `#[macro_export]`
this without realizing it'd leak into the public API. A one-line comment on the
intent — plus a note in the doc example that the `use
crate::error::invalid_data` path is crate-internal — would lock it in.
##########
crates/iceberg/src/spec/manifest/entry.rs:
##########
@@ -170,15 +170,13 @@ impl TryFrom<i32> for ManifestStatus {
0 => Ok(ManifestStatus::Existing),
1 => Ok(ManifestStatus::Added),
2 => Ok(ManifestStatus::Deleted),
- _ => Err(Error::new(
- ErrorKind::DataInvalid,
- format!("manifest status {v} is invalid"),
- )),
+ _ => Err(invalid_data!("manifest status {v} is invalid")),
}
}
}
use super::DataFileFormat;
+use crate::error::invalid_data;
Review Comment:
This `use crate::error::invalid_data;` landed mid-file next to the `static
STATUS` block rather than in the top import group. Worth moving up with the
other `use crate::` lines.
--
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]
