Pass data type as string representation to NestedField#1860
Merged
kevinjqliu merged 3 commits intoapache:mainfrom Mar 30, 2025
Merged
Pass data type as string representation to NestedField#1860kevinjqliu merged 3 commits intoapache:mainfrom
NestedField#1860kevinjqliu merged 3 commits intoapache:mainfrom
Conversation
sunxiaojian
force-pushed
the
pass-type-as-string
branch
from
861769e to
940643b
Compare
sunxiaojian
force-pushed
the
pass-type-as-string
branch
from
940643b to
87f2f2d
Compare
kevinjqliu
reviewed
Mar 28, 2025
Contributor
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Thank you for the PR! I've added a few comments
tests/test_types.py
Outdated
|
|
||
| def test_nested_field_type_as_str_unsupported() -> None: | ||
| with pytest.raises(ValueError) as exc_info: | ||
| _ = (NestedField(1, "field", "list", required=True),) |
Contributor
There was a problem hiding this comment.
nit: lets also test other complex type list/map/struct
pyiceberg/types.py
Outdated
Comment on lines
343
to
348
| if isinstance(data["type"], str): | ||
| try: | ||
| data["type"] = IcebergType.handle_primitive_type(data["type"], None) | ||
| except ValueError as e: | ||
| raise ValueError(f"Unsupported field type: {data['type']}.") from e | ||
|
|
Contributor
There was a problem hiding this comment.
nit: what do you think about using a pydantic field validator here?
https://docs.pydantic.dev/latest/concepts/validators/#field-before-validator
@validator("field_type", pre=True)
def convert_field_type(cls, v):
"""Convert string values into IcebergType instances."""
if isinstance(v, str):
try:
return IcebergType.handle_primitive_type(v, None)
except ValueError as e:
raise ValueError(f"Unsupported field type: {v}") from e
return v # If already an IcebergType, keep as is
tests/test_types.py
Outdated
| assert "Unsupported field type: list" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_nested_field_type_as_str_struct() -> None: |
Contributor
There was a problem hiding this comment.
nit: include other types that are part of handle_primitive_type here as well
Contributor
Author
|
@kevinjqliu Thanks for the review! I've fixed all the comments. |
kevinjqliu
approved these changes
Mar 29, 2025
Contributor
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! I left a few nit comments to improve test readability
tests/test_types.py
Outdated
| assert "validation errors for NestedField" in str(exc_info.value) | ||
|
|
||
|
|
||
| def test_nested_field_type_as_str_unsupported() -> None: |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| def test_nested_field_type_as_str_unsupported() -> None: | |
| def test_nested_field_complex_type_as_str_unsupported() -> None: |
tests/test_types.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("type_str, type_class", primitive_types.items()) | ||
| def test_nested_field_type_as_str(type_str: str, type_class: type) -> None: |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| def test_nested_field_type_as_str(type_str: str, type_class: type) -> None: | |
| def test_nested_field_primitive_type_as_str(type_str: str, type_class: type) -> None: |
Contributor
There was a problem hiding this comment.
nit: parametrizing the test actually is slower compared to just running a for loop into the function
for type_str, type_class in primitive_types.items():
field_var = NestedField(
1,
"field",
type_str,
required=True,
)
assert isinstance(
field_var.field_type, type_class
), f"Expected {type_class.__name__}, got {field_var.field_type.__class__.__name__}"
| assert isinstance( | ||
| field_var.field_type, type_class | ||
| ), f"Expected {type_class.__name__}, got {field_var.field_type.__class__.__name__}" | ||
|
|
Contributor
There was a problem hiding this comment.
nit can we add a test for this case too?
except ValueError as e:
raise ValueError(f"Unsupported field type: '{v}'") from e
6d0cdb1 to
a73dda5
Compare
Contributor
Author
|
@kevinjqliu Fixed all the comments. PTAL, thanks |
kevinjqliu
changed the title
NestedField
Contributor
|
Thanks for the contribution @sunxiaojian |
gabeiglio
pushed a commit
to Netflix/iceberg-python
that referenced
this pull request
Aug 13, 2025
<!--
Thanks for opening a pull request!
-->
<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
# Rationale for this change
Closes apache#1851
# Are these changes tested?
test_types.py
# Are there any user-facing changes?
N/A
<!-- In the case of user-facing changes, please add the changelog label.
-->
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
2 participants
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
Closes #1851
Are these changes tested?
test_types.py
Are there any user-facing changes?
N/A