Skip to content

Fix TruncateTransform.satisfies_order_of crashing on different widths - #3682

Open
SAY-5 wants to merge 2 commits into
apache:mainfrom
SAY-5:fix-truncate-satisfies-order-of
Open

SAY-5 wants to merge 2 commits into
apache:mainfrom
SAY-5:fix-truncate-satisfies-order-of

Conversation

@SAY-5

@SAY-5 SAY-5 commented Jul 18, 2026 •

Copy link
Copy Markdown

Closes #3680

Rationale for this change

TruncateTransform.satisfies_order_of reads an uninitialized _source_type when comparing unequal transforms, raising AttributeError instead of returning a boolean. The width-only replacement originally proposed here is also incorrect for numeric truncation, even on the same source field.

For integer values [3, 2], truncation at width 5 produces [0, 0], while width 3 produces [3, 0]. The sequence is ordered under the first transform but not the second. Width comparison alone therefore cannot establish sort-order compatibility.

Use equality-only comparison for unbound truncate transforms. Equal transforms satisfy each other's order; unequal widths conservatively return False. This avoids reading missing type state and makes the result independent of earlier transform(source) calls. Type-specific unequal-width refinements are not inferred without a source type.

AI disclosure: this follow-up code and regression-test revision, and this revised description, were generated with OpenAI Codex. This disclosure describes the current revision, not the provenance of earlier contributions.

Are these changes tested?

The correction was checked locally with Python 3.13 and the project unit-test selector. Integration, cloud and notebook suites were not run locally.

  • New ordering regressions: 10 failed and 6 passed on the previous PR implementation; all 16 passed with the correction. They cover positive and negative integer/long/decimal ties, string/binary prefix ties, equal and unequal widths, other transform classes, and independence from source-type calls.
  • Related focused tests: 63 passed. These focused runs used Python 3.11 with --noconftest and plugin autoload disabled; they are not a full project-suite result.
  • make test with an isolated external runner: 3,779 passed, 2 skipped, 123 deselected. Two multiprocessing cases (test_use_executor_in_different_process, fork/spawn) were explicitly deselected because spawned processes would not inherit the local isolation; the remaining deselections come from the project selector. Integration/notebook directories were excluded from collection after verifying their tests were already excluded by that selector. Existing unit fixtures ran with isolated configuration, loopback services and scoped Kerberos-init/cleanup mocks. This is not a full-suite or exact-CI-environment result.
  • Actual make lint: all 12 configured hooks passed, with no existing source or lockfile changes. The Python hook environments, including mypy, used Python 3.14.4.

Are there any user-facing changes?

Yes. Unequal-width comparisons return False instead of raising an exception or claiming unsupported order compatibility. Equal-width comparisons remain True. Constructor and method signatures, the source_type property, and truncation behavior are preserved.

Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Comment thread pyiceberg/transforms.py Outdated
and isinstance(other, TruncateTransform)
and isinstance(other.source_type, StringType)
):
elif isinstance(other, TruncateTransform):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

dropping the source_type check means an int/long/decimal transform now satisfies_order_of another purely on width. is that the intended widening, or should it stay type-gated? the java implementation only returns true within the same type family, so one type family claiming to satisfy another (string vs int) could let an invalid sort-order replacement through.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Intentional. _source_type is a PrivateAttr that is never assigned on TruncateTransform, so self.source_type raised AttributeError and the whole branch was dead, string vs string included.

Java can gate on type because it has separate TruncateString/TruncateInteger/TruncateDecimal classes; pyiceberg has one width-carrying transform with no bound type, same as iceberg-rust, which also compares widths only. And satisfies_order_of is only ever asked between transforms on the same source field, so a string truncate never gets handed an int one in practice.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

AI-generated follow-up using OpenAI Codex; the current code and test revision was also generated with Codex.

The width-only widening is incorrect even for the same integer field: values [3, 2] produce keys [0, 0] with width 5 and [3, 0] with width 3. Ordering by width 5 therefore does not guarantee ordering by width 3. The earlier statement that iceberg-rust compares truncate widths only was incorrect and should not have justified this change.

The correction uses self == other: equal transforms return True, unequal widths return False, without reading _source_type or changing the public API signatures/property. This deliberately leaves type-specific unequal-width compatibility unrecognized for unbound transforms. The regression tests exercise the numeric counterexample, prefix ties, and independence from prior source-type calls. The new regressions fail on the previous implementation and pass with the correction. All 12 make lint hooks passed. The isolated make test run passed 3,779 tests, with 2 skipped and 123 deselected; two multiprocessing cases were explicitly excluded because spawned processes would not inherit the local isolation. Integration, cloud and notebook suites were not run locally; full validation remains with CI.

@SAY-5

SAY-5 commented Jul 20, 2026

Copy link
Copy Markdown
Author

Good question. The _source_type PrivateAttr on TruncateTransform is never assigned anywhere in the codebase, so self.source_type raises AttributeError on any non-equal comparison, which is the crash this PR fixes (#3680). That means the previous StringType branch never actually ran for a bare TruncateTransform(n) either.

For the widening concern: satisfies_order_of is only meaningful when both transforms are bound to the same source field (same sort key), so they already share a type family by construction; a truncate transform carries no independent source type to cross-compare. This also matches the sibling transforms in this file, e.g. the time transforms compare purely on granularity and BucketTransform on num_buckets, without re-checking the source type. The added test asserts a non-truncate (BucketTransform) returns False, so cross-transform mixing is still rejected. Happy to add an explicit same-family guard if you'd prefer it spelled out.

@github-actions

Copy link
Copy Markdown

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.

@github-actions github-actions Bot added the stale label Sep 25, 2026

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TruncateTransform.satisfies_order_of raises AttributeError for different widths

2 participants