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] Catalog.list_tables() inconsistency between docstring and signature #1163

Closed
dataders opened this issue Sep 11, 2024 · 4 comments · Fixed by #1166 or #1168
Closed

[BUG] Catalog.list_tables() inconsistency between docstring and signature #1163

dataders opened this issue Sep 11, 2024 · 4 comments · Fixed by #1166 or #1168
Assignees

Comments

@dataders
Copy link
Contributor

Apache Iceberg version

0.7.1 (latest release)

Please describe the bug 🐞

the docstring for catalog.list_tables() says

If namespace not provided, will list all tables in the catalog.

But I'm finding that to not be the case! The function definition itself seems to not allow this

@abstractmethod
def list_tables(self, namespace: Union[str, Identifier]) -> List[Identifier]:
"""List tables under the given namespace in the catalog.
If namespace not provided, will list all tables in the catalog.
Args:
namespace (str | Identifier): Namespace identifier to search.
Returns:
List[Identifier]: list of table identifiers.
Raises:
NoSuchNamespaceError: If a namespace with the given name does not exist.
"""

error

TypeError: RestCatalog.list_tables() missing 1 required positional argument: 'namespace'
File ~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:478, in Retrying.__call__(self, fn, *args, **kwargs)
    [476](https://file+.vscode-resource.vscode-cdn.net/Users/dataders/repos/sandbox-spark-iceberg/~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:476) if isinstance(do, DoAttempt):
    [477](https://file+.vscode-resource.vscode-cdn.net/Users/dataders/repos/sandbox-spark-iceberg/~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:477)     try:
--> [478](https://file+.vscode-resource.vscode-cdn.net/Users/dataders/repos/sandbox-spark-iceberg/~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:478)         result = fn(*args, **kwargs)
    [479](https://file+.vscode-resource.vscode-cdn.net/Users/dataders/repos/sandbox-spark-iceberg/~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:479)     except BaseException:  # noqa: B902
    [480](https://file+.vscode-resource.vscode-cdn.net/Users/dataders/repos/sandbox-spark-iceberg/~/repos/sandbox-spark-iceberg/.conda/lib/python3.11/site-packages/tenacity/__init__.py:480)         retry_state.set_exception(sys.exc_info())  # type: ignore[arg-type]

TypeError: RestCatalog.list_tables() missing 1 required positional argument: 'namespace'
@kevinjqliu
Copy link
Contributor

Thanks for reporting this! I noticed it too when reviewing #1140.

I think the spec only allows listing tables in a namespace.
https://github.com/apache/iceberg/blob/34cd01ba2e057866cdb13db8f9919bc98e11e638/open-api/rest-catalog-open-api.yaml#L497-L505

And the reference Java HiveCatalog implementation too
https://github.com/apache/iceberg/blob/34cd01ba2e057866cdb13db8f9919bc98e11e638/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java#L121-L157

So I assume the docstring is outdated

@sungwy
Copy link
Collaborator

sungwy commented Sep 12, 2024

Thank you for raising this issue @dataders good to see you here on the GitHub Repository 🙂

I agree with @kevinjqliu 's finding that the docstring is outdated, and that we should update it to be compliant with the spec. Would you be interested in submitting a PR?

@dataders
Copy link
Contributor Author

dataders commented Sep 12, 2024

Would you be interested in submitting a PR?

of course! thanks for giving me the opportunity. I'll try and ship something by EOTW

One question I have is how the docstrings correspond to The Code Reference page of the docs site (https://py.iceberg.apache.org/reference/pyiceberg/). Ostensibly, the docs page is auto-generated from the source code, for all base classes (but not ABCs)?

@kevinjqliu
Copy link
Contributor

Hey there, found a couple more instances

If namespace is not provided, lists all views in the catalog.

If namespace not provided, will list all tables in the catalog.

I think this should cover all of them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants