Repository navigation
Core: Add setter for firstRowId in V4 metadata - #16849
gaborkaszab wants to merge 1 commit into
Conversation
|
Currently, there is no mechanism to set |
| } | ||
|
|
||
| void setFirstRowId(Long newFirstRowId) { | ||
| Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null"); |
There was a problem hiding this comment.
Why not just make the parameter long since it's required anyway?
There was a problem hiding this comment.
+1 to make the parameter to long.
There was a problem hiding this comment.
Good point! done
|
|
||
| void setFirstRowId(Long newFirstRowId) { | ||
| Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null"); | ||
| Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative"); |
There was a problem hiding this comment.
| Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative"); | |
| Preconditions.checkArgument( | |
| newFirstRowId >= 0, "Invalid first row ID: %s (must be >= 0)", newFirstRowId); |
| } | ||
|
|
||
| void setFirstRowId(Long newFirstRowId) { | ||
| Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null"); |
There was a problem hiding this comment.
+1 to make the parameter to long.
fe964f0 to
a0b0ddd
Compare
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for taking a look! I addressed all the comments. Additionally, there are 2 extra modifications I figured would make sense:
- Not allow to set first row id for ADDED entries. I think for these null is the desired value
- I also added a setter method to
TrackedFileStructfor convenience. With this the user doesn't have to be aware of the actual representation being inTrackingand can simply invokeTrackedFileStruct.setFirstRowId()without knowing how it is represented internally.
Let me know what you think!
| } | ||
|
|
||
| void setFirstRowId(Long newFirstRowId) { | ||
| Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null"); |
There was a problem hiding this comment.
Good point! done
|
|
||
| void setFirstRowId(Long newFirstRowId) { | ||
| Preconditions.checkArgument(newFirstRowId != null, "Invalid first row ID: null"); | ||
| Preconditions.checkArgument(newFirstRowId >= 0, "Invalid first row ID: negative"); |
a0b0ddd to
ab3b98a
Compare
gaborkaszab
changed the title
| public void setFirstRowId(int firstRowId) { | ||
| Preconditions.checkState( | ||
| contentType == FileContent.DATA || contentType == FileContent.DATA_MANIFEST, | ||
| "Cannot set first row id on an equality delete or delete manifest entry"); |
There was a problem hiding this comment.
nit: instead of saying what's not allowed. Would stating only allow the supported content type be more explicit?
| } | ||
|
|
||
| /** Sets the first row id on this {@link TrackedFileStruct} */ | ||
| public void setFirstRowId(int firstRowId) { |
There was a problem hiding this comment.
curious, should firstRowId be long type instead, like in TrackingStruct?
|
Thanks for looking, @dramaticlly ! Your comments make sense. In the meantime there might be a different approach to set firstRowId, so I'll change this to draft until we decide. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
5 participants
No description provided.