Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[test] Run partition transform tests for all transforms #1592

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kevinjqliu
Copy link
Contributor

Closes #1591

Blocked on iceberg-rust/#920 and the next release of pyiceberg_core (0.5.0)

@kevinjqliu kevinjqliu marked this pull request as draft January 31, 2025 23:58
@kevinjqliu kevinjqliu force-pushed the kevinjqliu/check-all-transforms branch 2 times, most recently from 245ae20 to 2c3e97f Compare February 21, 2025 19:53
@kevinjqliu
Copy link
Contributor Author

CI failed https://github.com/apache/iceberg-python/actions/runs/13464196511/job/37626276454?pr=1592

=================================== FAILURES ===================================
_ test_transform_consistency_with_pyarrow_transform[source_type10-\x8e\xd1\x87\x01] _

source_type = BinaryType(), value = b'\x8e\xd1\x87\x01'

    @pytest.mark.parametrize(
        "source_type, value",
        [
            (IntegerType(), 22),
            (LongType(), 22),
            (DecimalType(5, 9), Decimal(19.25)),
            (DateType(), datetime.date(1925, 5, 22)),
            (TimeType(), datetime.time(19, 25, 00)),
            (TimestampType(), datetime.datetime(19, 5, 1, 22, 1, 1)),
            (TimestamptzType(), datetime.datetime(19, 5, 1, 22, 1, 1, tzinfo=datetime.timezone.utc)),
            (StringType(), "abc"),
            (UUIDType(), UUID("12345678-1234-5678-1234-567812345678").bytes),
            (FixedType(5), 'b"\x8e\xd1\x87\x01"'),
            (BinaryType(), b"\x8e\xd1\x87\x01"),
        ],
    )
    def test_transform_consistency_with_pyarrow_transform(source_type: PrimitiveType, value: Any) -> None:
        import pyarrow as pa
    
        all_transforms = [  # type: ignore
            IdentityTransform(),
            BucketTransform(10),
            TruncateTransform(10),
            YearTransform(),
            MonthTransform(),
            DayTransform(),
            HourTransform(),
        ]
        for t in all_transforms:
            if t.can_transform(source_type):
>               assert t.transform(source_type)(value) == t.pyarrow_transform(source_type)(pa.array([value])).to_pylist()[0]

tests/table/test_partitioning.py:211: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

array = <pyarrow.lib.BinaryArray object at 0x7ffad121be80>
[
  8ED18701
]

    def _transform(array: "ArrayLike") -> "ArrayLike":
        if isinstance(array, pa.Array):
>           return transform_func(array, *args)
E           pyo3_runtime.PanicException: range end index 10 out of range for slice of length 4

pyiceberg/transforms.py:209: PanicException
----------------------------- Captured stderr call -----------------------------
thread '<unnamed>' panicked at /home/runner/work/iceberg-rust/iceberg-rust/crates/iceberg/src/transform/truncate.rs:47:11:
range end index 10 out of range for slice of length 4
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
=========================== short test summary info ============================
FAILED tests/table/test_partitioning.py::test_transform_consistency_with_pyarrow_transform[source_type10-\x8e\xd1\x87\x01] - pyo3_runtime.PanicException: range end index 10 out of range for slice of length 4
========== 1 failed, 2771 passed, 1053 deselected in 67.84s (0:01:07) ==========
make: *** [Makefile:78: test-coverage-unit] Error 1

cc @Fokko do you think this is a bug on the rust side?

@kevinjqliu
Copy link
Contributor Author

@Fokko
Copy link
Contributor

Fokko commented Feb 21, 2025

Yes, looks like it. Let me submit a patch, thanks!

@kevinjqliu
Copy link
Contributor Author

nightly build doing its job :)

kevinjqliu pushed a commit to apache/iceberg-rust that referenced this pull request Feb 21, 2025
@kevinjqliu kevinjqliu force-pushed the kevinjqliu/check-all-transforms branch from 2c3e97f to be11aca Compare February 21, 2025 23:06
@kevinjqliu
Copy link
Contributor Author

all good here, thanks for the quick fix @Fokko

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.

[test] Improve partition transform test
2 participants