fix: Don't use _position_to_field_name
#3917
Merged
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
This was referenced Mar 5, 2025
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3917 +/- ##
==========================================
+ Coverage 75.35% 78.45% +3.09%
==========================================
Files 767 768 +1
Lines 103619 97902 -5717
==========================================
- Hits 78080 76807 -1273
+ Misses 25539 21095 -4444
🚀 New features to boost your workflow:
|
Member
|
Hey @Fokko, thanks for bringing this up! From what I can tell, the I'm okay with doing that, but we should get it in with these changes. Would you mind changing the version requirement in pyproject.toml? |
Contributor
Author
|
@kevinzwang Thanks, that's a great point! I wouldn't recommend anyone using 0.4.0 anyway, I've bumped it to |
Fokko
force-pushed
the
fd-remove_position_to_field_name
branch
from
11701c8 to
ca7e6e1
Compare
kevinzwang
approved these changes
Mar 7, 2025
Member
kevinzwang
left a comment
kevinzwang
left a comment
There was a problem hiding this comment.
Looks great! thanks again for the help
kevinzwang
pushed a commit
that referenced
this pull request
Mar 8, 2025
Don't use internal fields :) I want to remove this one in apache/iceberg-python#1768. It should be okay since the `Record` also has a `__len__`.
Fokko
added a commit
to apache/iceberg-python
that referenced
this pull request
Apr 22, 2025
This aligns the implementation with Java. We had the keywords there mostly for the tests, but they should not be used, and it seems like that's already the case :'( I was undecided if the costs of this PR (all the changes), are worth it, but I see more PRs using the Record in a bad way (example #1743) that might lead to very subtle bugs where the position might sometime change based on the ordering of the dict. Blocked by Eventual-Inc/Daft#3917
gabeiglio
pushed a commit
to Netflix/iceberg-python
that referenced
this pull request
Aug 13, 2025
This aligns the implementation with Java. We had the keywords there mostly for the tests, but they should not be used, and it seems like that's already the case :'( I was undecided if the costs of this PR (all the changes), are worth it, but I see more PRs using the Record in a bad way (example apache#1743) that might lead to very subtle bugs where the position might sometime change based on the ordering of the dict. Blocked by Eventual-Inc/Daft#3917
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.
Don't use internal fields :)
I want to remove this one in apache/iceberg-python#1768. It should be okay since the
Recordalso has a__len__.