Skip to content

test(iceberg): cover partition field transform conversion #7024

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.

Sign up for GitHub

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

Merged
colin-ho merged 1 commit into from
Jun 1, 2026
Merged

test(iceberg): cover partition field transform conversion #7024

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions tests/io/iceberg/test_iceberg_metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,28 @@ def test_convert_partition_field_bucket():
assert pf.transform == PartitionTransform.iceberg_bucket(4)


def test_convert_partition_field_day():
@pytest.mark.parametrize(
"source_id, transform, field_name, expected_dtype, expected_transform, expected_source_name",
[
(2, DayTransform(), "ts_day", DataType.date(), PartitionTransform.day(), "ts"),
(2, YearTransform(), "ts_year", DataType.int32(), PartitionTransform.year(), "ts"),
(2, MonthTransform(), "ts_month", DataType.int32(), PartitionTransform.month(), "ts"),
(2, HourTransform(), "ts_hour", DataType.int32(), PartitionTransform.hour(), "ts"),
(3, TruncateTransform(8), "name_trunc", DataType.string(), PartitionTransform.iceberg_truncate(8), "name"),
],
ids=["day", "year", "month", "hour", "truncate"],
)
def test_convert_partition_field_transforms(
source_id, transform, field_name, expected_dtype, expected_transform, expected_source_name
):
pf = convert_iceberg_partition_field(
SIMPLE_SCHEMA,
IcebergPartitionField(source_id=2, field_id=1002, transform=DayTransform(), name="ts_day"),
IcebergPartitionField(source_id=source_id, field_id=1003, transform=transform, name=field_name),
)
assert pf.field.name == "ts_day"
assert pf.field.dtype == DataType.date()
assert pf.source_field.name == "ts"
assert pf.field.name == field_name
assert pf.field.dtype == expected_dtype
assert pf.source_field.name == expected_source_name
assert pf.transform == expected_transform
Comment on lines +144 to +165

@greptile-apps greptile-apps Bot May 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 DayTransform left outside the parametrized test

DayTransform is conceptually in the same family as YearTransform, MonthTransform, and HourTransform (all are time-based transforms on ts), yet it lives in the standalone test_convert_partition_field_day function. That older test also never asserts pf.transform, so the transform equality for day is currently untested. Folding the day case into test_convert_partition_field_transforms (with PartitionTransform.day() and DataType.date() as expected values) would give it the same assertion depth as the other time-based transforms and keep all similar cases in one place.

Rule Used: Prefer single parametrized functions over multiple... (source)

Learned From
Eventual-Inc/Daft#5207

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@jackylee-ch jackylee-ch May 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in 9493dce: folded the DayTransform case into the parametrized transform test and added the PartitionTransform.day() assertion.



def test_convert_partition_spec_multiple_fields():
Expand Down
Loading