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

Bug Fix: Glue and Hive catalog return only Iceberg tables #1145

Merged
merged 3 commits into from
Oct 19, 2024

Conversation

mark-major
Copy link
Contributor

@mark-major mark-major commented Sep 7, 2024

Closes #314

Glue and Hive catalogs list_tables() method filters for Iceberg tables.

@mark-major mark-major marked this pull request as draft September 7, 2024 15:45
@mark-major mark-major marked this pull request as ready for review September 10, 2024 19:22
@mark-major mark-major changed the title WIP: Catalog return only Iceberg tables Glue and Hive catalog return only Iceberg tables Sep 10, 2024
Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

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

Hi @mark-major thank you for putting together this PR. The tests for the hive and glue catalogs seem to prove that your implementation is working 💯

I've left a question about the dynamodb implementation, and a nit suggestion, please let me know your thoughts!

pyiceberg/catalog/glue.py Outdated Show resolved Hide resolved
@@ -393,7 +393,7 @@ def drop_namespace(self, namespace: Union[str, Identifier]) -> None:
raise NoSuchNamespaceError(f"Database does not exist: {database_name}") from e

def list_tables(self, namespace: Union[str, Identifier]) -> List[Identifier]:
"""List tables under the given namespace in the catalog (including non-Iceberg tables).
"""List Iceberg tables under the given namespace in the catalog.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that we only updated the docstring here, without a change in the code.
Does Dynamodb already only return Iceberg tables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was quite a long time since I have worked on this, but if I recall correctly then yes, DynamoDB returned only Iceberg tables.

Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>
Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

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

Thank you again for working on this @mark-major ! I've confirmed that this behavior is consistent with Java

@sungwy sungwy merged commit a272bf7 into apache:main Oct 19, 2024
7 checks passed
@sungwy sungwy changed the title Glue and Hive catalog return only Iceberg tables Bug Fix: Glue and Hive catalog return only Iceberg tables Oct 19, 2024
@mark-major mark-major deleted the filter-out-non-iceberg-tables branch November 8, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants