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.

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

TODOs in storage/iceberg/impl.py in create_table_version #497

Merged
merged 3 commits into from
Mar 11, 2025

Conversation

sahil-03
Copy link
Contributor

@sahil-03 sahil-03 commented Mar 7, 2025

Summary

Resolving the following TODOs in create_table_vesrion:
# TODO: ensure catalog.create_table() is idempotent
# TODO: get table and commit new metadata if table already exists?

Rationale

Simple changes to get onboarded onto the project.

Changes

Addressed catalog.create_table() idempotecy --> attempts to load existing iceberg table; if it doesn't exist it performs catalog.create_table()

Addressed new metadata commit to existing table --> if table exists, uses the iceberg table transaction and sets the properties to the table_properties.

Impact

n/a

Testing

Passes all current tests. Need guidance on how/where to add tests.

Regression Risk

n/a

Checklist

  • Unit tests covering the changes have been added

    • If this is a bugfix, regression tests have been added
  • E2E testing has been performed

Additional Notes

Any additional information or context relevant to this PR.

Copy link
Member

@pdames pdames left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to delete the TODO statements on line 396 and 397 (and remove the corresponding comments about resolve todo #n), and then I'll get this merged.

@pdames
Copy link
Member

pdames commented Mar 10, 2025

Hey @sahil-03 - looks like we've got a linter failure in the CI/CD workflow - can you run make lint in your workspace and check in another revision with all linter checks passing? Most issues will be auto-resolved, but the linter will stop to let you review its changes after every failure, so you may need to run it a few times to resolve all issues.

@pdames pdames merged commit 7947528 into ray-project:2.0 Mar 11, 2025
3 checks passed
anshumankomawar pushed a commit that referenced this pull request Mar 11, 2025
* resolving TODOs in create_table_version (storage/iceberg/impl.py)

* resolved TODOs + cleaned; ready to merge

* ran make lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants