laskoviymishka commented on code in PR #2928:
URL: https://github.com/apache/iceberg-rust/pull/2928#discussion_r4046607858
##########
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!("FixedSizeBinary conversion
failed").with_source(e))?;
Review Comment:
this one quietly changes the visible message — it used to surface arrow's
own error text as `.message()`, now it's the static `"FixedSizeBinary
conversion failed"` with the arrow error tucked into `.with_source(e)`.
I actually think the new shape is better since the source chain keeps
everything, it's just the one spot in an otherwise mechanical pass that changes
what a caller sees. I'd add a line to the PR description calling it out
alongside the avro fix so it's not a silent behaviour change. wdyt?
##########
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
Review Comment:
re: the open thread about compiling this instead of `ignore` — I think
`ignore` is right here. doctests compile as their own external crate, so they
can't resolve a `pub(crate)` item no matter the toolchain, and making the macro
`pub` just to green the doctest would contradict the "deliberately
crate-internal" line right above it. I'd leave it as-is and close that thread.
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 fix from the description landing — `{avro_schema}`
was riding along as literal text before and now actually renders through the
macro's `format!` arm.
since it's the one real behaviour change in the PR, could we add a small
test that runs `avro_schema_to_schema` on a non-record schema (a bare
`AvroSchema::Boolean` would do) and asserts the message contains the schema's
Display rather than the literal `{avro_schema}`? that locks the fix in so it
can't quietly regress. not blocking.
--
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]
