Modify list_* methods in catalogs to return Iterators
#2172
Conversation
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
This looks awesome! Thanks for doing it. I've got one code question and then a versioning question:
Do we need to wait for 1.0 to change these APIs? I'm hoping the answer will be no.
| if tables := list(self.list_tables(namespace)): | ||
| raise NamespaceNotEmptyError(f"Namespace {namespace_str} is not empty. {len(list(tables))} tables exist.") |
There was a problem hiding this comment.
When we (eventually) handle pagination tokens, I believe this will cause the process to consume the entire paginated list. If it's a long list, that'll require a lot of server calls to fetch all of the tables.
If that's true, what if we did this instead?
| if tables := list(self.list_tables(namespace)): | |
| raise NamespaceNotEmptyError(f"Namespace {namespace_str} is not empty. {len(list(tables))} tables exist.") | |
| tables = self.list_tables(namespace) | |
| try: | |
| next(tables) | |
| raise NamespaceNotEmptyError(f"Namespace {namespace_str} is not empty. {len(list(tables))} tables exist.") | |
| except StopIteration: | |
| pass | |
There was a problem hiding this comment.
I see, maybe it makes the most sense to make a little helper function here for if a namespace is empty or not? can do that in a different MR but i dont know if thats an official method upstream https://github.com/search?q=repo%3Aapache%2Ficeberg%20namespacenotempty&type=code
3 participants
Rationale for this change
We want to support pagination and we need to be able to return iterators to do so.
See #2158 for more context
Are these changes tested?
Existing tests were modified
Are there any user-facing changes?
yes, this is user facing and breaking but overall an improvement.