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

add support for glue.id #490

Merged
merged 2 commits into from
Mar 5, 2024
Merged

add support for glue.id #490

merged 2 commits into from
Mar 5, 2024

Conversation

jrouly
Copy link
Contributor

@jrouly jrouly commented Mar 1, 2024

The upstream Iceberg documentation details glue.id as a parameter which can be used to set the AWS Glue Catalog ID (AWS Account ID).

This is critical for scenarios where the Glue catalog exists in a separate environment from the workload running pyiceberg (e.g. an account separation between workloads and data lake).

This PR reads glue.id and, if set, injects CatalogId= as a parameter on all boto3 Glue service calls.

@Fokko
Copy link
Contributor

Fokko commented Mar 2, 2024

@HonahX since you're more familiar with Glue, would you be able to do this review? :)

Copy link
Contributor

@HonahX HonahX left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Thanks for working on this! @jrouly I like the way of using Event System to support the catalog id. Just have some small comments.

pyiceberg/catalog/glue.py Show resolved Hide resolved
pyiceberg/catalog/glue.py Outdated Show resolved Hide resolved
pyiceberg/catalog/glue.py Outdated Show resolved Hide resolved
pyiceberg/catalog/glue.py Show resolved Hide resolved
@jrouly
Copy link
Contributor Author

jrouly commented Mar 4, 2024

@HonahX appreciate the review! Pushed a new commit with your suggestions.

Copy link
Contributor

@HonahX HonahX left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @jrouly

@HonahX HonahX merged commit be81529 into apache:main Mar 5, 2024
7 checks passed
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.

3 participants