-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add UnionByName functionality #296
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great work! @Fokko. It looks good. I left some comments and questions
type=ListType( | ||
element_id=2, | ||
element_type=ListType( | ||
element_id=3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, did you manually format the schema in this style or the make lint
did the job? This looks nice, especially in case of multi-level nested schema 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the linter :D I guess it got the inspiration from Java: https://github.com/apache/iceberg/blob/17757276928e87e78314b6c19c8b3ebad9612619/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java#L429-L459 :D
pyiceberg/table/__init__.py
Outdated
else: | ||
return self.new_schema.find_field(field_id).field_type | ||
|
||
def field(self, field: NestedField, field_partner: Optional[int], field_result: bool) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def field(self, field: NestedField, field_partner: Optional[int], field_result: bool) -> bool: | |
def field(self, field: NestedField, partner_id: Optional[int], field_result: bool) -> bool: |
Shall we name the second argument as partner_id
to better reveal its content? We already did this in schema
and struct
. Same applied for *_partner
arguments below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, thanks for the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Fokko. LGTM!
Co-authored-by: Honah J. <[email protected]>
Thanks for the review @HonahX |
Resolves #284