Skip to content

Make commit_table public - #1112

Merged
sungwy merged 7 commits into
apache:mainfrom
Fokko:fd-commit-api
Sep 5, 2024
Merged

Make commit_table public#1112
sungwy merged 7 commits into
apache:mainfrom
Fokko:fd-commit-api

Conversation

@Fokko

@FokkoFokko commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

This was brought up earlier, and I think this is a good idea. I changed the signature to also pass in the table.

@FokkoFokko mentioned this pull request Aug 28, 2024
Merged
sungwy
sungwy reviewed Aug 29, 2024
View reviewed changes

@sungwysungwy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Fokko - thank you for driving this! In agreement with you on the updated input params for the method as well. 👍 I left a few nits, hope you find them helpful

Comment threadpyiceberg/catalog/glue.py Outdated
Comment threadpyiceberg/catalog/hive.py Outdated
Comment threadpyiceberg/catalog/hive.py Outdated
Comment threadpyiceberg/catalog/rest.py
Comment threadpyiceberg/catalog/rest.py Outdated
kevinjqliu
kevinjqliu reviewed Aug 30, 2024
View reviewed changes

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some minor comments.
Also want to point out that _commit_table doc used to be "Update one or more tables." and is now changed to support updating only 1 table.

Any idea how we can safely deprecate the _commit_table function?

Comment threadpyiceberg/catalog/rest.py Outdated
Comment threadpyiceberg/table/__init__.py Outdated
Comment threadpyiceberg/table/__init__.py Outdated
Comment threadtests/catalog/test_base.py Outdated
Comment threadpyiceberg/catalog/rest.py Outdated
Comment threadpyiceberg/catalog/glue.py Outdated
Comment threadpyiceberg/catalog/hive.py Outdated
Comment threadpyiceberg/catalog/sql.py Outdated
kevinjqliu
kevinjqliu approved these changes Sep 1, 2024
View reviewed changes

@kevinjqliukevinjqliu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@sungwy

sungwy commented Sep 3, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

Hi @Fokko

Just a heads up - my apologies for adding in a merge conflict here. I merged in #820 first because it had been open for quite some time. Could we rebase this branch from the updated main?

@Fokko

Fokko commented Sep 5, 2024

Copy link
Copy Markdown
ContributorAuthor

@sungwy No problem at all, I've pulled in latest master 👍

@sungwy
sungwy merged commit 052a9cd into apache:mainSep 5, 2024
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@Fokko@sungwy
Makecommit_tablepublic (apache#1112)
cdfeaf3
* Make `commit_table` public
* Comments
* Thanks Kevin!
* Update tests
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
@Fokko@sungwy
Makecommit_tablepublic (apache#1112)
7f6a329
* Make `commit_table` public
* Comments
* Thanks Kevin!
* Update tests
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kevinjqliukevinjqliukevinjqliu approved these changes
@sungwysungwysungwy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Fokko@sungwy@kevinjqliu