Skip to content

fix(spec): format dropped-source integer partition values - #3432

Open
jopdorp wants to merge 2 commits into
apache:mainfrom
jopdorp:fix-dropped-partition-format
Open

jopdorp wants to merge 2 commits into
apache:mainfrom
jopdorp:fix-dropped-partition-format

Conversation

@jopdorp

@jopdorp jopdorp commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Addresses #3381.

What changes are included in this PR?

We format partition values whose source column has been dropped instead of panicking, for the identity and truncate cases in the issue and for uuid and decimal. Decimal prints the unscaled value, since the scale only lived on the dropped column.

Are these changes tested?

the regression panics without the fix. the library suite, fmt and clippy pass.

AI Disclosure

The implementation and regression test were generated with an AI coding assistant and reviewed by a second coding assistant. The regression reproduces the panic without the fix; the library suite, fmt and clippy pass.

@JeonDaehong

JeonDaehong commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Thanks for picking this up!

I ran the same setup on this branch with other source types. int/long/date/bool/binary now work, but identity(decimal), truncate(decimal) and identity(uuid) still hit the unreachable!() in Display for Datum (Int128/UInt128 with Unknown type).

I missed these in #3381, sorry. uuid could reuse the Uuid formatting; decimal is trickier since Unknown loses the scale. Would a non-panicking fallback arm make sense here?

@JeonDaehong

Copy link
Copy Markdown
Contributor

Following up on the decimal/uuid part, I checked what Java writes here (iceberg-api, JDK 21). Once the source is dropped, Java sees UnknownType and just uses value.toString():

source present source dropped
identity(decimal(9,2)) 1.23 p=1.23 p=1.23
truncate[10](decimal(9,2)) 1.20 p=1.20 p=1.20
identity(uuid) p=f79c3e09-… p=f79c3e09-…
identity(date) 19000 p=2022-01-08 p=19000

uuid should be easy to match by formatting the UInt128 as a UUID. decimal is the tricky one: BigDecimal keeps its scale, but on our side only the unscaled Int128 is left, so any fallback would write p=123 where Java writes p=1.23. date already gives 19000 on this branch, same as Java.

@jopdorp

jopdorp commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

@JeonDaehong thanks for checking these and for the Java table, that helped a lot.

We pushed b765bbf so uuid and decimal no longer panic. uuid prints like Java (p=f79c3e09-…). decimal prints the unscaled value, so p=123 where Java writes p=1.23, because the scale lives on the dropped source type and the partition value only carries the unscaled Int128. We added both cases to the existing dropped-source test. Happy to adjust if you'd rather handle decimal differently.

@JeonDaehong JeonDaehong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked b765bbf: it merges cleanly with main, and the spec::partition and spec::values tests pass (167). No more unreachable!() for dropped decimal/uuid sources, and uuid matches Java.

Printing the unscaled decimal is fine with me. The scale is not lost everywhere, though: a manifest is decoded with the schema stored in it, where the source still exists, so the value arrives as Int128 and only the path formatting (which uses the current schema) has no scale. Matching Java exactly would mean formatting with the manifest's schema, which is a separate change. LGTM.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants