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

SqlCatalog and other catalog works different on load_namespace_properties #769

Closed
giucris opened this issue May 27, 2024 · 3 comments
Closed

Comments

@giucris
Copy link

giucris commented May 27, 2024

Apache Iceberg version

0.6.0 (latest release)

Please describe the bug 🐞

During the development of a new feature I encountered a behaviour in the implementation of the SqlCatalog that currently we are using as test catalog and I think is a bug.

In the documentation of the abstract method list_namespace_properties there is a reference to an exception being thrown if the namespace does not exist, but the current implementation of SqlCatalog returns an empty dict when the namespace does not exist instead of raising the exception.

In this way the catalogs, although part of the same "interface" work and function in two completely different ways.

How to reproduce it

    def test_find_namespace(self):
        #initialize in memory sql catalog
        props = {
            "uri": "sqlite+pysqlite:///:memory:",
            "warehouse": f"file://tmp/test_catalog",
        }
        catalog = SqlCatalog("test_catalog", **props)

        #create a namespace
        catalog.create_namespace("existing_namespace")

        #retrieve properties for the created namespace
        existing_namespace = catalog.load_namespace_properties("existing_namespace")

        #retrieve properties for not existing namespace
        not_existing_namespace = catalog.load_namespace_properties("not_existing_namespace")

        self.assertEqual(existing_namespace,{"exists":"true"})
        self.assertEqual(not_existing_namespace,{})

Expected behavior

As described in the abstract method documentation, the expected behavior is to raise a NoSucheNameSpaceError exception.
the same behavior indeed is also implemented in the GlueCatalog that we use on production code.

@soumya-ghosh
Copy link
Contributor

@kevinjqliu Seems like behavior of SqlCatalog for load_namespace_properties is rectified in current version.
I got it fixed in PR - #591, which was merged a day after this issue was raised.

I checked above code snippet, it raises pyiceberg.exceptions.NoSuchNamespaceError: Namespace not_existing_namespace does not exists

I think we can close this issue and its parent issue #813 as well.

@kevinjqliu
Copy link
Contributor

In the documentation of the abstract method list_namespace_properties there is a reference to an exception being thrown if the namespace does not exist, but the current implementation of SqlCatalog returns an empty dict when the namespace does not exist instead of raising the exception.

@soumya-ghosh I think this test covers the issue reported

@pytest.mark.parametrize(
"catalog",
[
lazy_fixture("catalog_memory"),
lazy_fixture("catalog_sqlite"),
],
)
def test_load_namespace_properties_non_existing_namespace(catalog: SqlCatalog) -> None:
with pytest.raises(NoSuchNamespaceError):
catalog.load_namespace_properties("does_not_exist")

Would you agree?

@soumya-ghosh
Copy link
Contributor

Yes, that's correct. I don't see this test in 0.6.0 tag.

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

No branches or pull requests

3 participants