Skip to content

fix: raise TypeError for unsupported partition source type#3617

Open
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:patch-16
Open

fix: raise TypeError for unsupported partition source type#3617
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:patch-16

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

The @singledispatch base of _to_partition_representation in
pyiceberg/partitioning.py returned its error instead of raising it:

return TypeError(f"Unsupported partition field type: {type}")

For an unsupported (non-primitive) partition source type, the base handler is the
one that runs, and it handed the TypeError instance back as the partition value
rather than raising it. The exception object would then flow downstream as the
"iceberg-typed" partition value (through partition_record_value ->
PartitionKey.partition -> Record) and only blow up later during serialization,
far from the real cause, or write a garbage partition record.

This is the only return <Exception>(...) in the pyiceberg/ tree. Every sibling
registered handler for this function raises (the TimestampType / DateType /
TimeType / UUIDType / PrimitiveType registrations all raise ValueError), as
does every other singledispatch base handler in the codebase (for example the
to_bytes / from_bytes / to_json / from_json bases in
pyiceberg/conversions.py). So raise is the correct, convention-matching
behavior; the fix is a one-word correction (return -> raise).

In normal use check_compatible blocks non-primitive partition sources upstream, so
this base handler is reached only in edge cases: this is a latent robustness fix with
no behavioral downside on the happy path.

Are these changes tested?

Yes. A focused regression test was added in tests/table/test_partitioning.py
(test_to_partition_representation_unsupported_type) that asserts the base handler
raises TypeError for a non-primitive (StructType) source type. It fails on the
old return (DID NOT RAISE TypeError) and passes after the change to raise.

python -m pytest tests/table/test_partitioning.py \
  -k to_partition_representation_unsupported_type -v
# -> 1 passed

The linters (ruff, ruff-format, mypy and the rest of the pre-commit suite) pass on
both changed files. No dependencies changed, so uv.lock is untouched.

Are there any user-facing changes?

No public API changes. The only observable difference is that an unsupported
partition source type now raises TypeError at its source (as it was always meant
to) instead of silently yielding an exception instance as the partition value.

The singledispatch fallback of _to_partition_representation used
return TypeError(...) instead of raise TypeError(...). For an
unsupported (non-primitive) partition source type this returned the
exception instance as the partition value instead of raising, so the
error would surface far from its cause. Raise it, and add a regression
test covering the base handler.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>

@ebyhr ebyhr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

partitioning.py change looks good. I'm not convinced the new test adds much value, though.

The base handler is unreachable in normal use, so it exercises an implementation detail rather than observable behavior.

Comment thread pyiceberg/partitioning.py
since epoch).
"""
return TypeError(f"Unsupported partition field type: {type}")
raise TypeError(f"Unsupported partition field type: {type}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No other same mistake as far as I checked.

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.

2 participants