fix: raise TypeError for unsupported partition source type#3617
Open
anxkhn wants to merge 1 commit into
Open
Conversation
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
approved these changes
Jul 7, 2026
ebyhr
left a comment
Member
There was a problem hiding this comment.
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.
| since epoch). | ||
| """ | ||
| return TypeError(f"Unsupported partition field type: {type}") | ||
| raise TypeError(f"Unsupported partition field type: {type}") |
Member
There was a problem hiding this comment.
No other same mistake as far as I checked.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The
@singledispatchbase of_to_partition_representationinpyiceberg/partitioning.pyreturned its error instead of raising it:For an unsupported (non-primitive) partition source type, the base handler is the
one that runs, and it handed the
TypeErrorinstance back as the partition valuerather 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 thepyiceberg/tree. Every siblingregistered handler for this function raises (the
TimestampType/DateType/TimeType/UUIDType/PrimitiveTyperegistrations allraise ValueError), asdoes every other
singledispatchbase handler in the codebase (for example theto_bytes/from_bytes/to_json/from_jsonbases inpyiceberg/conversions.py). Soraiseis the correct, convention-matchingbehavior; the fix is a one-word correction (
return->raise).In normal use
check_compatibleblocks non-primitive partition sources upstream, sothis 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 handlerraises
TypeErrorfor a non-primitive (StructType) source type. It fails on theold
return(DID NOT RAISE TypeError) and passes after the change toraise.The linters (ruff, ruff-format, mypy and the rest of the pre-commit suite) pass on
both changed files. No dependencies changed, so
uv.lockis untouched.Are there any user-facing changes?
No public API changes. The only observable difference is that an unsupported
partition source type now raises
TypeErrorat its source (as it was always meantto) instead of silently yielding an exception instance as the partition value.