-
Notifications
You must be signed in to change notification settings - Fork 229
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
Small getting started guide on writes #311
Conversation
6c7d6e3
to
683b700
Compare
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 tutorial! @Fokko. LGTM! Just have one comment about the usage of internal method
mkdocs/docs/index.md
Outdated
|
||
with table.update_schema() as update_schema: | ||
# Blocked by https://github.com/apache/iceberg-python/pull/305 | ||
update_schema.union_by_name(Catalog._convert_schema_if_needed(df.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.
Shall we use add_column
instead?
update_schema.add_column("tip_per_mile", DoubleType())
I think it might be better not to use internal methods in the tutorial.
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 fully agree. I should have been more explicit in the comment above. Once #305 is in, we can update the union_by_name
to also accept pa.Schema
. WDYT?
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 context! Yes, I think it's a good idea. Let's do it after #305 😄
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 should have marked it as a draft. It is ready now @HonahX :)
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.
LGTM! Thanks @Fokko. Sorry for being late here.
Just to get people started!