fix checking physical type for Decimal type in StatsAggregator #2515
Conversation
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
4 tasks
desmondcheongzx
pushed a commit
to Eventual-Inc/Daft
that referenced
this pull request
Sep 25, 2025
## Changes Made Added changes to conditionally handle breaking changes in pyiceberg 0.10.0 for how to initialize `DataFile` and `Record` and constraints on `name` for `Schema` fields and `PartitionField`. I did not update dev dependencies at this point, because there are actually still issues with Decimal. I have a [PR](apache/iceberg-python#2515) open that hopefully addresses this. To test, I had to temporarily change dev dependencies to 0.10.0, and run tests without decimal type. I need to update to 0.10.0 because support for anonymous was added (needed on my side). Note sure what the best path is here considering that pyiceberg releases on a much slower cadence. ## Related Issues Closes #5223 ## Checklist - [ ] Documented in API Docs (if applicable) - [ ] Documented in User Guide (if applicable) - [ ] If adding a new documentation page, doc is added to `docs/mkdocs.yml` navigation - [ ] Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)
Contributor
|
Hey @gmweaver thanks for adding this. Could you also add a test for this? This ensures that we don't break it in the future. |
Contributor
Author
|
@Fokko test added and ensured I ran |
kevinjqliu
reviewed
Sep 26, 2025
Contributor
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
iceberg-python/pyiceberg/io/pyarrow.py
Lines 2507 to 2518 in e5e7453
can we simplify this logic too? can be a follow up PR
kevinjqliu
reviewed
Sep 26, 2025
kevinjqliu
reviewed
Sep 26, 2025
kevinjqliu
approved these changes
Sep 26, 2025
Contributor
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM Thanks for fixing this issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Fix for #2057. It looks like the initial fix #1839 might have missed updating here to handle. I could use feedback on if this is the best fix, it is at least simple.
Are these changes tested?
Are there any user-facing changes?
No