Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat: Implement create_table_if_not_exists #415

Merged
merged 6 commits into from
Feb 20, 2024
Merged

Feat: Implement create_table_if_not_exists #415

merged 6 commits into from
Feb 20, 2024

Conversation

hussein-awala
Copy link
Member

@hussein-awala hussein-awala commented Feb 12, 2024 •
edited
Loading

closes: #406

This PR implements the create_table_if_not_exists method to create the table if it doesn't exist, and return the existing table if it exists instead of raising a TableAlreadyExistsError exception.

@Fokko
Copy link
Contributor

Fokko commented Feb 13, 2024

I think we should have some more discussion around this. I would prefer to have, analog to SQL, a CREATE OR REPLACE API.

@sungwy
Copy link
Collaborator

sungwy commented Feb 13, 2024

I think we should have some more discussion around this. I would prefer to have, analog to SQL, a CREATE OR REPLACE API.

This feels analogous to the SQL CREATE TABLE IF NOT EXISTS, which allows applications to run the statement idempotently in a Production deployed code without throwing an exception. Hence I'd argue it fills in a different niche than CREATE OR REPLACE, which effectively replaces an existing table (which is exactly what we want to avoid doing).

@hussein-awala
Copy link
Member Author

Exactly, this implements CREATE TABLE IF NOT EXISTS and not CREATE OR REPLACE.

If you prefer to have a new method create_table_if_not_exists, I can move the table creation block to a new private/protected method, and use it in create_table and the new create_table_if_not_exists.

@Fokko
Copy link
Contributor

Fokko commented Feb 16, 2024

@hussein-awala Thanks! You're right, and I think create_table_if_not_exists would be a cleaner solution, see #406.

@hussein-awala
Copy link
Member Author

I will update the PR then 👍

@hussein-awala hussein-awala changed the title Feat: Add fail_if_exists param to create_table Feat: Implement create_table_if_not_exists Feb 20, 2024
@hussein-awala
Copy link
Member Author

I think it's ready 🤞

Fokko
Fokko reviewed Feb 20, 2024
View reviewed changes
tests/catalog/test_rest.py
@@ -560,6 +560,64 @@ def test_create_table_409(rest_mock: Mocker, table_schema_simple: Schema) -> Non
assert "Table already exists" in str(e.value)


def test_create_table_if_not_exists_200(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! It would have been easier to write an integration test, but great job on the mocking 👍

Fokko
Fokko approved these changes Feb 20, 2024
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

@hussein-awala This looks great, thanks!

@Fokko Fokko merged commit 9b01248 into apache:main Feb 20, 2024
6 checks passed
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Feb 20, 2024
@hussein-awala @sungwy
Implement create_table_if_not_exists (apache#415)
f4de1d1 
* Feat: Add fail_if_exists param to create_table

* create create_table_if_not_exists method

* fix reset test

* fix mypy check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fokko Fokko Fokko 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.

check if table exist
3 participants
@hussein-awala @Fokko @sungwy