laskoviymishka commented on code in PR #2928:
URL: https://github.com/apache/iceberg-rust/pull/2928#discussion_r4004814640
##########
crates/iceberg/src/arrow/schema.rs:
##########
@@ -851,7 +805,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}").with_source(e))?;
Review Comment:
Small one — `{e}` formats the Arrow error into the message and then
`.with_source(e)` attaches it again, so the same text shows up twice in the
Display output. Not wrong, and it's actually more than the pre-PR code carried
(which dropped the source entirely), but the decimal conversions just above use
the brief-message-plus-source form. I'd match them:
```suggestion
.map_err(|e| invalid_data!("FixedSizeBinary conversion
failed").with_source(e))?;
```
##########
crates/iceberg/src/error.rs:
##########
@@ -469,6 +469,47 @@ 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.
+///
+/// Unlike the public [`ensure_data_valid!`], this macro is deliberately
+/// crate-internal — adding `#[macro_export]` would commit it to the public
API.
+///
+/// # Examples
+///
+/// The `use` path below is crate-internal and only resolves inside this crate.
+///
+/// ```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 {
+ // Bare literals stay in `format!` so inline captures like `{id}` still
interpolate.
+ ($fmt: literal $(, $($arg:tt)*)?) => {
+ $crate::error::Error::new($crate::error::ErrorKind::DataInvalid,
format!($fmt $(, $($arg)*)?))
Review Comment:
This is the one new thing I'd want to nail down before merge. The literal
arm always routes through `format!`, so every zero-arg call —
`invalid_data!("File already closed")` and the ~30 others like it — expands to
`format!("File already closed")`, which is exactly what
`clippy::useless_format` fires on. CI runs clippy with `-D warnings`, so if it
surfaces from the expansion the build goes red.
Whether it actually surfaces through a `macro_rules!` expansion is
version-dependent, so the real question is just: is clippy green on this PR? If
it is, I'm happy to leave the single-arm design as-is. If not, I'd add a
zero-arg arm ahead of the variadic one:
```rust
($msg: literal) => {
$crate::error::Error::new($crate::error::ErrorKind::DataInvalid,
$msg.to_owned())
};
```
(the trade-off being that inline-capture calls like `invalid_data!("text
{var}")` would then need the explicit `invalid_data!("text {}", var)` form).
wdyt?
##########
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 interpolation from last round — still riding along silently.
`{avro_schema}` printed verbatim before; through the macro's `format!` arm it
now actually interpolates, and it only compiles because `AvroSchema: Display`
happens to hold.
I'm not asking to revert it, it's a strictly better message. Just make it
deliberate — a line in the description, or `{avro_schema:?}` — so it's on the
record rather than an accident. 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]
