fix(transform): correct decimal and binary truncate literals by advancedxy · Pull Request #3243 · apache/iceberg-rust

advancedxy · 2026-09-17T16:16:35Z

This correctness issue was identified while implementing and reviewing #2456.

Which issue does this PR close?

No issue is currently linked.

What changes are included in this PR?

Decimal truncate literals lost their source scale, and binary literals were rejected. For example, truncate[50](10.65) returned 1050 instead of 10.50, even though the Arrow decimal path already preserves precision and scale.

Preserve the source decimal precision and scale during literal truncation and the projection boundary adjustments of one unscaled unit. A shared Datum::with_decimal_mantissa helper keeps this logic in one place. Projection boundaries may temporarily exceed the source precision, so the helper validates the Decimal type without imposing a precision limit on the adjusted value.
Truncate binary literals to their first W bytes, retaining shorter values. Keep Fixed unsupported because Fixed and Binary share the same literal variant, and document why the type guard is required.
Collect binary bytes directly from an iterator, avoiding an intermediate Vec allocation.
Update decimal strict-projection expectations and cover binary inclusive/strict projection, deduplication of truncated set literals, and decimal projection boundaries outside the source precision.

Related PR: #3120 also adds binary literal truncation and additionally supports LargeBinary arrays. The binary literal arm and some tests overlap; whichever PR is rebased after the other should retain the Fixed guard and its rejection test. LargeBinary array support remains in #3120.

Are these changes tested?

cargo test -p iceberg --lib --all-features --locked: 1850 library tests passed, including 21 truncate tests.
make check-fmt.
cargo clippy -p iceberg --all-targets --all-features --locked -- -D warnings.
make check-license-notice.
git diff --check.

AI Disclosure

OpenAI Codex was used for implementation. I reviewed the changes against repository conventions.

advancedxy · 2026-09-17T16:23:58Z

@CTTY @mbutrovich would you mind to take a look at this. I think this is a correctness fix.

advancedxy · 2026-09-24T03:28:04Z

Gently ping @CTTY @mbutrovich

The Arrow array path already preserves decimal precision and scale and supports binary truncation. The literal path instead reset decimal scale to zero and rejected binary values. For example, truncate[50](10.65) returned 1050 instead of 10.50. Align literal truncation with the Arrow array path: preserve the source decimal precision and scale, and return the first W bytes of binary values, or the entire value when shorter. Decimal projection boundary adjustments must also preserve precision and scale to keep their values correct before truncation.

advancedxy · 2026-10-08T03:18:39Z

        assert_eq!(
            result.to_string(),
-            "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 10000)".to_string()
+            "((pcol1 < 100) AND (pcol2 < 100)) AND (pcol3 < 100.00)".to_string()


the related pyiceberg test: https://github.com/apache/iceberg-python/blob/068aae50402285657b41f5357acb67672b0a053d/tests/test_transforms.py#L1574-L1592

other places are similar.

advancedxy · 2026-10-08T03:18:55Z

Gently ping @CTTY.
and @ZENOTME @Xuanwo @Fokko also since you touched/reviewed the original related code, you might have more related context.

JeonDaehong

I compared this with iceberg-java. The new expected values match TestTruncatesProjection exactly (e.g., testDecimalStrictLowerBound: < 100.00, >= -> > 99.90, NOT IN [99.00, 100.00, 101.00]; upper bound [98.90, 99.90, 100.90]).

Keeping the scale on the ±1 boundary is also what ProjectionUtil does (new BigDecimal(unscaled ± 1, scale)), and the first-W-bytes approach for binary matches TruncateByteBuffer. LGTM!

One note : #3120 (open since 8/31, closes #3119) also adds the Binary arm to Truncate::transform_literal, so it may be worth linking the two so they don't conflict.

laskoviymishka

The core fix is right — the literal path was hard-coding scale 0 when it rebuilt a truncated decimal, dropping the source scale, and routing through the input's own type fixes that and lines up with the Arrow path. The new binary arm and the Fixed rejection both look sound, so nothing here blocks.

Two small things I'd do in this pass: the binary arm leans entirely on the == PrimitiveType::Binary guard to keep Fixed out (they share the same literal variant), so a one-line comment there would stop someone deleting the guard later; and the .to_vec() can just be .iter().copied() to skip the second allocation. The repeated Datum::new(type.clone(), Int128(..)) idiom (three sites now) and a projection-level test for the new binary arm are both worth a look but fine as follow-ups.

Worth linking this with #3120 so one PR owns the binary arm — whoever rebases second needs to keep the Fixed guard and test_fixed_literal_is_unsupported.

laskoviymishka · 2026-10-11T18:39:30Z

-                    Self::truncate_decimal_i128(*v, width),
-                    0,
-                ))?
+                Datum::new(


We're now building Datum::new(type.clone(), Int128(..)) in three places — here plus try_increment_number / try_decrement_number — all through the unchecked pub(crate) constructor. Passing the unscaled value straight through is the right call for the fix, so my only suggestion is pulling it into a small Datum-level helper so the idiom lives in one spot instead of being copied. Fine as a follow-up if you'd rather keep this PR tight.

thanks for the suggestion, let me pull it together in this pr.

laskoviymishka · 2026-10-11T18:39:31Z

                let len = self.width as usize;
                Datum::string(Self::truncate_str(v, len).to_string())
            })),
+            PrimitiveLiteral::Binary(v) if input.data_type() == &PrimitiveType::Binary => Ok(Some(


This arm is correct only because the == &PrimitiveType::Binary guard keeps Fixed out — Datum::fixed and Datum::binary both store PrimitiveLiteral::Binary, so without the guard a Fixed value would truncate and come back typed as binary. I'd add a one-line comment saying exactly that, since it's an easy guard to drop in a later refactor. (matches!(input.data_type(), PrimitiveType::Binary) also reads a touch cleaner than == &.. if you're on the line anyway.)

thanks for the suggestion, will do.

laskoviymishka · 2026-10-11T18:39:31Z

                Datum::string(Self::truncate_str(v, len).to_string())
            })),
+            PrimitiveLiteral::Binary(v) if input.data_type() == &PrimitiveType::Binary => Ok(Some(
+                Datum::binary(Self::truncate_binary(v, self.width as usize).to_vec()),


Datum::binary already collects from an IntoIterator<u8>, so the .to_vec() allocates once and then the constructor collects again. Dropping to an iterator skips the intermediate Vec — small, non-hot path:

Suggested change

Datum::binary(Self::truncate_binary(v, self.width as usize).to_vec()),

Datum::binary(Self::truncate_binary(v, self.width as usize).iter().copied()),

laskoviymishka · 2026-10-11T18:39:31Z

+    }
+
+    #[test]
+    fn test_binary_literal() {


These exercise transform_literal directly, but the binary arm is now reachable through strict/inclusive projection too and nothing covers that path. A TestProjectionFixture binary case (=, <=, IN) would lock in the projection behavior the same way the decimal fixtures do. Follow-up is fine.

thanks for the suggestion, let me check if it's a good fit to put that in this pr too.

it's included in this pr as well.

advancedxy force-pushed the codex/fix-truncate-literal branch from 14a6628 to c13bfb3 Compare September 24, 2026 03:25

advancedxy force-pushed the codex/fix-truncate-literal branch from c13bfb3 to 7bc9d7f Compare October 8, 2026 02:55

advancedxy commented Oct 8, 2026

View reviewed changes

JeonDaehong approved these changes Oct 11, 2026

View reviewed changes

laskoviymishka approved these changes Oct 11, 2026

View reviewed changes

fix(transform): address truncate literal review feedback

41acbd2

laskoviymishka added this pull request to the merge queue Oct 12, 2026

Merged via the queue into apache:main with commit b154603 Oct 12, 2026
25 checks passed

andygrove mentioned this pull request Oct 13, 2026

Bump iceberg-rust past apache/iceberg-rust#3255 and narrow the nested schema evolution fallback apache/datafusion-comet#6934

Open

moomindani mentioned this pull request Oct 16, 2026

Parse unrecognized transform names as Transform::Unknown #2790

Open

	Datum::binary(Self::truncate_binary(v, self.width as usize).to_vec()),
	Datum::binary(Self::truncate_binary(v, self.width as usize).iter().copied()),

Conversation

advancedxy commented Sep 17, 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!

advancedxy commented Sep 17, 2026

Uh oh!

advancedxy commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advancedxy commented Oct 8, 2026

Uh oh!

JeonDaehong left a comment

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

advancedxy commented Sep 17, 2026 •

edited

Loading