Conversation
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
| and isinstance(other, TruncateTransform) | ||
| and isinstance(other.source_type, StringType) | ||
| ): | ||
| elif isinstance(other, TruncateTransform): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Good question. The For the widening concern: |
|
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. |
Generated-by: OpenAI Codex
Closes #3680
Rationale for this change
TruncateTransform.satisfies_order_ofreads an uninitialized_source_typewhen comparing unequal transforms, raisingAttributeErrorinstead 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 earliertransform(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.
--noconftestand plugin autoload disabled; they are not a full project-suite result.make testwith 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.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
Falseinstead of raising an exception or claiming unsupported order compatibility. Equal-width comparisons remainTrue. Constructor and method signatures, thesource_typeproperty, and truncation behavior are preserved.