Introduce invalid data error macro by xanderbailey · Pull Request #2928 · apache/iceberg-rust

xanderbailey · 2026-07-30T14:15:02Z

Which issue does this PR close?

Today we construct DataInvalid like so: which ends up being very verbose in my opinion

Error::new(
    ErrorKind::DataInvalid,
    "Partition column is not a StructArray",
)

What changes are included in this PR?

Introduce invalid_data! macro as a shorthand for this error.

The error above becomes:

invalid_data!("Partition column is not a StructArray")

The migration is behaviour-preserving except at two sites, where the error message changes:

avro_schema_to_schema in crates/iceberg/src/avro/schema.rs — an intentional fix. It
previously passed "Can't convert non record avro schema to iceberg schema: {avro_schema}" as a
plain &str, so the braces printed verbatim and avro_schema was never substituted. The macro's
literal arm routes the message through format!, so the message now actually interpolates the
schema via Display.
get_arrow_datum in crates/iceberg/src/arrow/schema.rs — the FixedSizeBinary conversion.
The failure path was Error::new(ErrorKind::DataInvalid, e.to_string()), which made arrow's error
text the message() and left the source unset. It is now
invalid_data!("FixedSizeBinary conversion failed").with_source(e), so the message is a stable
static string and arrow's text is preserved as the error's source rather than being flattened into
the message. Nothing is lost, but message() no longer contains arrow's text — callers matching
on it should read the source instead.

Every .with_context(...) call on a migrated error is preserved as-is; only the two messages above
differ.

Are these changes tested?

AI Disclosure

Claude wrote the script to do this migration but I have reviewed manually.

xanderbailey · 2026-07-30T14:29:36Z

Not sure what folks think about this change but I personally think it improves readability.

anoopj

Nice cleanup!

anoopj · 2026-07-31T03:14:10Z

+/// // 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 {


Just one design thought: the expr arm accepts any expression, so if somone writes invalid_data!(format!(...)) (out of habit), we will double allocate. Not worth guarding against though.

github-actions · 2026-08-31T00:27:23Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

xanderbailey · 2026-09-03T07:41:07Z

@blackmwk just wanted to tag you for thoughts before this goes stale, if we're happy then I'll resolve conflicts

laskoviymishka

agreed with anoopj, this is a nice cleanup — the invalid_data! macro reads well and the call sites are genuinely quieter for it.

Since it's a script-driven rewrite across ~68 files, I did a second pass focused on one thing: whether the mechanical migration preserved the old error semantics 1:1 everywhere. Mostly yes, but a couple of things I'd want to sort before merge.

The one real behavior change is in avro/schema.rs. The old code passed "...iceberg schema: {avro_schema}" as a plain &str, so the braces printed verbatim — a latent bug. The macro's literal arm wraps it in format!, so now it actually interpolates avro_schema. That's a strictly better message, but it's a silent semantic change riding along on a "no behavior change" cleanup, and it only compiles because AvroSchema: Display happens to hold. I'd make it deliberate — call it out in the description (or switch to {avro_schema:?}) so it's on the record rather than an accident.

The other thing is scope: delete_vector.rs still has ~9 Error::new(ErrorKind::DataInvalid, ...) sites and arrow/reader/pipeline.rs has at least one, none of which are in this PR. A partial migration leaves the crate with two idioms for the same error and no way to tell "skipped on purpose" from "missed." I'd either run the script over those too, or add a line to the description saying why they're excluded.

Everything else is nits — a handful of leftover .to_string() calls the literal arm makes redundant, one invalid_data!(e.to_string()) that drops the source chain, and a couple of macro-hygiene things I left inline.

Once the avro change is intentional and the migration scope is settled, happy to take another pass and approve.

laskoviymishka · 2026-09-09T09:14:32Z

-            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}"


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.

laskoviymishka · 2026-09-09T09:14:32Z

-                    )
-                    .with_context("value", value)
-                    .with_source(e)
+                    invalid_data!("Failed to parse field id".to_string())


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.

laskoviymishka · 2026-09-09T09:14:32Z

+/// 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)*)?) => {


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:

($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?

laskoviymishka · 2026-09-09T09:14:32Z

        (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()))?;


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.

laskoviymishka · 2026-09-09T09:14:32Z

+}
+
+// Crate-internal macro: re-exported so other modules can `use crate::error::invalid_data;`.
+pub(crate) use invalid_data;


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.

laskoviymishka · 2026-09-09T09:14:32Z

    }

+    #[test]
+    fn test_invalid_data_macro() {


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.

laskoviymishka · 2026-09-09T09:14:32Z

 }

 use super::DataFileFormat;
+use crate::error::invalid_data;


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.

laskoviymishka

Nice — this is close now. The scope ask from last round landed: delete_vector.rs and arrow/reader/pipeline.rs are both migrated in this pass, so the crate isn't split across two idioms for the same error anymore. The leftover .to_string() calls and the with_source chains are all clean too, and the macro hygiene ($crate:: paths, no #[macro_export]) reads well.

One thing carried over: the avro/schema.rs interpolation is still riding along silently. I'm not asking to revert it — it's a strictly better message — just to make it deliberate, either a note in the description or {avro_schema:?}, so it's on the record. That was my one gate from last round.

The one new thing is the literal arm always going through format!, which is what clippy::useless_format keys on for the ~30 zero-arg call sites. It may or may not surface from inside the expansion, so really I just want to confirm clippy is green on this PR — if it is, the single-arm design is fine as-is. I left that inline along with a tiny .with_source duplication nit in arrow/schema.rs.

Confirm clippy's green and make the avro change intentional, and I'm happy to approve.

laskoviymishka · 2026-09-14T11:47:36Z

-            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}"


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?

I've update the PR description, hope that works for you!

laskoviymishka · 2026-09-14T11:47:36Z

+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)*)?))


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:

($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?

blackmwk · 2026-09-15T10:16:28Z

+///
+/// The `use` path below is crate-internal and only resolves inside this crate.
+///
+/// ```ignore


We should not ignore this, we should compile it.

The macro is pub(crate) use, so a doctest — which compiles as an external crate — can't resolve it. Should we make it pub?

blackmwk · 2026-09-15T10:19:26Z

+/// // 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 {


nit, should we have an arm which includes exception so that we don't need to write with_source?

invalid_data!("msg").with_source(e) → invalid_data!("msg", source = e)

Like this?

blackmwk · 2026-09-15T10:21:04Z

+        $crate::error::Error::new($crate::error::ErrorKind::DataInvalid, $msg)
+    };
+}
+


I see a lot of places has repeated return Err(invalid_data!()), should we have a macro for it as well?

Could we do this kind of change as a follow up? It would be a reasonable change on top of this PR I think

laskoviymishka

the avro {avro_schema} interpolation being called out as an intentional fix in the description is exactly what I was after — that closes the last thing I had open on it.

with scope complete (delete_vector and the reader pipeline are both in now) and clippy green on the latest run, everything I gated on across the earlier rounds is resolved, and the crate-internal single-arm design is fine as-is.

two small non-blocking things if you feel like it: the FixedSizeBinary path swaps arrow's error text for a static message (the source is preserved, so it's actually a nicer shape — I'd just mention it in the description alongside the avro note), and a tiny test pinning the avro interpolation would keep it from silently regressing. neither holds up the merge.

this is good to land. thanks for sticking with the iteration on this one.

laskoviymishka · 2026-09-18T12:13:40Z

        (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))?;


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?

laskoviymishka · 2026-09-18T12:13:40Z

-            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}"


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.

laskoviymishka · 2026-09-18T12:13:40Z

+///
+/// The `use` path below is crate-internal and only resolves inside this crate.
+///
+/// ```ignore


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?

xanderbailey · 2026-09-18T12:53:13Z

I think that's all sorted now, thanks for the reviews!

xanderbailey · 2026-09-18T12:53:32Z

CI blocked on #3249

…/invalid_data_macro

xanderbailey force-pushed the xb/invalid_data_macro branch from 7a4473f to b4c444d Compare July 30, 2026 14:28

xanderbailey marked this pull request as ready for review July 30, 2026 14:29

anoopj approved these changes Jul 31, 2026

View reviewed changes

github-actions Bot added the stale label Aug 31, 2026

github-actions Bot removed the stale label Sep 4, 2026

laskoviymishka requested changes Sep 9, 2026

View reviewed changes

xanderbailey force-pushed the xb/invalid_data_macro branch from b4c444d to 7b8eed7 Compare September 11, 2026 08:54

Introduce invalid data error macro

5286e7e

xanderbailey force-pushed the xb/invalid_data_macro branch from 7b8eed7 to 5286e7e Compare September 11, 2026 11:01

laskoviymishka requested changes Sep 14, 2026

View reviewed changes

fix

99e0844

blackmwk reviewed Sep 15, 2026

View reviewed changes

laskoviymishka approved these changes Sep 18, 2026

View reviewed changes

xanderbailey added 2 commits September 18, 2026 13:46

Merge remote-tracking branch 'origin/main' into xb/invalid_data_macro

66c0235

comments

553b9ee

Merge branch 'main' of https://github.com/apache/iceberg-rust into xb…

c038ea3

…/invalid_data_macro

Conversation

xanderbailey commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

AI Disclosure

Uh oh!

xanderbailey commented Jul 30, 2026

Uh oh!

anoopj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

xanderbailey commented Sep 3, 2026

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented Sep 18, 2026

Uh oh!

xanderbailey commented Sep 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xanderbailey commented Jul 30, 2026 •

edited

Loading