Repository navigation
Conversation
satisfies_order_of read source_type, which is never set on TruncateTransform, so comparing two truncate transforms with different widths raised AttributeError. The source type is not known on the unbound transform, and a wider truncation only preserves the order of a narrower one for strings, so only equal transforms are reported as satisfying the order. Closes apache#3680 Generated-by: Grok Bot (AI coding assistant)
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
The Java implementation specifically checks widths. I'm not sure if this is the approach we want to take.
A truncate transform satisfies the order of another truncate transform when its width is greater than or equal to the other's, as in the Java Truncate implementation. This no longer reads the missing source_type attribute. Generated-by: Grok Bot (AI coding assistant)
|
@rambleraptor I've updated this to match the Java implementation (truncate satisfies another truncate when its width is >= the other's), and CI is green. PTAL when you get a chance. |
| assert truncate_transform.satisfies_order_of(truncate_transform) | ||
|
|
||
|
|
||
| def test_truncate_satisfies_order_of_different_widths() -> None: |
There was a problem hiding this comment.
Can we port over the same list of tests that we see in the reference implementation? We need to give ourselves some certainty that this matches the reference implementation.
There was a problem hiding this comment.
sure! ported testSatisfiesTruncateFieldOrder from TestSortOrder.java in the Java repo, same 5 assertions (identity vs truncate, truncate(4) vs truncate(2) and the reverse). pyiceberg doesn't have SortOrder.satisfies yet so they're checked straight on satisfies_order_of, as a parametrized test. kept the equal-width and truncate-vs-bucket cases from before too. the test fails on main and passes with the fix (｡•ᴗ•｡)
…tion Mirror TestSortOrder#testSatisfiesTruncateFieldOrder at the transform level, since pyiceberg has no SortOrder.satisfies. Generated-by: AI-assisted
|
@rambleraptor gentle ping — Java |
Closes #3680
Rationale for this change
TruncateTransform.satisfies_order_ofreadsself.source_type, but_source_typeis never set onTruncateTransform, so comparing two truncate transforms with different widths raises:Following review feedback, this now matches the Java
Truncate.satisfiesOrderOf: a truncate transform satisfies the order of another truncate transform when its width is greater than or equal to the other's. It no longer reads the missingsource_type.I left the
source_typeproperty in place to avoid an API change.This picks up the same bug as #3681, #3682 and #3854, which the stale bot closed.
Are these changes tested?
Yes.
test_truncate_satisfies_order_of_different_widthsintests/test_transforms.pycovers equal widths, both directions of different widths, and other transform types.tests/test_transforms.pypasses locally, and ruff check and ruff format pass.Are there any user-facing changes?
TruncateTransform(a).satisfies_order_of(TruncateTransform(b))now returnsa >= binstead of raisingAttributeErrorwhena != b.This change was prepared with an AI coding assistant, and the tests were run locally. The commits have a
Generated-by:trailer, following the ASF generative tooling guidance. I've also reviewed the changes myself.