Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyiceberg/catalog/bigquery_metastore.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ def load_view(self, identifier: str | Identifier) -> View:

@override
def load_namespace_properties(self, namespace: str | Identifier) -> Properties:
dataset_name = self.identifier_to_database(namespace)
dataset_name = self.identifier_to_database(namespace, NoSuchNamespaceError)

try:
dataset = self.client.get_dataset(DatasetReference(project=self.project_id, dataset_id=dataset_name))
Expand Down
73 changes: 49 additions & 24 deletions pyiceberg/cli/console.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@
from pyiceberg.cli.output import ConsoleOutput, JsonOutput, Output
from pyiceberg.exceptions import NoSuchNamespaceError, NoSuchPropertyException, NoSuchTableError
from pyiceberg.io import WAREHOUSE
from pyiceberg.table import TableProperties
from pyiceberg.table import Table, TableProperties
from pyiceberg.table.refs import SnapshotRef, SnapshotRefType
from pyiceberg.typedef import Properties
from pyiceberg.utils.properties import property_as_int


Expand Down Expand Up @@ -142,38 +143,62 @@ def list(ctx: Context, parent: str | None) -> None: # pylint: disable=redefined


@run.command()
@click.option("--entity", type=click.Choice(["any", "namespace", "table"]), default="any")
@click.option(
"--entity",
type=click.Choice(["any", "namespace", "table"]),
default="any",
help="Entity type. 'any' auto-detects and requires --entity when ambiguous.",
)
@click.argument("identifier")
@click.pass_context
@catch_exception()
def describe(ctx: Context, entity: Literal["name", "namespace", "table"], identifier: str) -> None:
def describe(ctx: Context, entity: Literal["any", "namespace", "table"], identifier: str) -> None:
"""Describe a namespace or a table."""
catalog, output = _catalog_and_output(ctx)
identifier_tuple = Catalog.identifier_to_tuple(identifier)

is_namespace = False
if entity in {"namespace", "any"} and len(identifier_tuple) > 0:
try:
namespace_properties = catalog.load_namespace_properties(identifier_tuple)
output.describe_properties(namespace_properties)
is_namespace = True
except NoSuchNamespaceError as exc:
if entity != "any" or len(identifier_tuple) == 1: # type: ignore
raise exc

is_table = False
if entity in {"table", "any"} and len(identifier_tuple) > 1:
try:
catalog_table = catalog.load_table(identifier)
output.describe_table(catalog_table)
is_table = True
except NoSuchTableError as exc:
if entity != "any":
raise exc

if is_namespace is False and is_table is False:
if entity == "namespace":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if entity == "namespace":
if entity == "namespace" or (entity == "any" and len(identifier_tuple) == 1):

This will clean up the if statement at line 167, but it's a bit harder to parse. Your call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i actually did this intentionally. these ifs are for the individual checks namespace / table / and view (in #3926)

starting L167 is the "any" fallback behavior. i can add a inline comment for that to be more explicit

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah that would be great. I saw that's what you were trying to do, but that little bit of duplication was definitely bugging me 😂

output.describe_properties(catalog.load_namespace_properties(identifier_tuple))
return
if entity == "table":
output.describe_table(catalog.load_table(identifier))
return

# For the default "any" entity, auto-detect the entity type.
if len(identifier_tuple) == 1:
Comment thread
kevinjqliu marked this conversation as resolved.
output.describe_properties(catalog.load_namespace_properties(identifier_tuple))
return

matches: tuple[str, ...] = ()
namespace_properties: Properties | None = None
catalog_table: Table | None = None

try:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: comment here describing that this is a namespace attempt could be good.

namespace_properties = catalog.load_namespace_properties(identifier_tuple)
matches += ("namespace",)
except NoSuchNamespaceError:
pass

try:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

same as above but with table

catalog_table = catalog.load_table(identifier)
matches += ("table",)
except NoSuchTableError:
pass

if len(matches) > 1:
raise ValueError(
f"Identifier {identifier} matches multiple entity types: {', '.join(matches)}. Use --entity to disambiguate."
)
if not matches:
raise NoSuchTableError(f"Table or namespace does not exist: {identifier}")

if matches[0] == "namespace":
assert namespace_properties is not None
output.describe_properties(namespace_properties)
else:
assert catalog_table is not None
output.describe_table(catalog_table)


@run.command()
@click.argument("identifier")
Expand Down
15 changes: 14 additions & 1 deletion tests/catalog/test_bigquery_metastore.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
import os
from unittest.mock import MagicMock

import pytest
from google.api_core.exceptions import NotFound
from google.cloud.bigquery import Dataset, DatasetReference, Table, TableReference
from google.cloud.bigquery.external_config import ExternalCatalogDatasetOptions, ExternalCatalogTableOptions
from pytest_mock import MockFixture

from pyiceberg.catalog.bigquery_metastore import ICEBERG_TABLE_TYPE_VALUE, TABLE_TYPE_PROP, BigQueryMetastoreCatalog
from pyiceberg.exceptions import NoSuchTableError
from pyiceberg.exceptions import NoSuchNamespaceError, NoSuchTableError
from pyiceberg.schema import Schema


Expand Down Expand Up @@ -178,3 +179,15 @@ def test_list_namespaces(mocker: MockFixture) -> None:
assert ("dataset1",) in namespaces
assert ("dataset2",) in namespaces
client_mock.list_datasets.assert_called_once()


def test_load_namespace_properties_rejects_multipart_namespace(mocker: MockFixture) -> None:
client_mock = MagicMock()
mocker.patch("pyiceberg.catalog.bigquery_metastore.Client", return_value=client_mock)
mocker.patch.dict(os.environ, values={"PYICEBERG_LEGACY_CURRENT_SNAPSHOT_ID": "True"})
catalog = BigQueryMetastoreCatalog("test_catalog", **{"gcp.bigquery.project-id": "my-project"})

with pytest.raises(NoSuchNamespaceError, match="hierarchical namespaces are not supported"):
catalog.load_namespace_properties(("dataset", "table"))

client_mock.get_dataset.assert_not_called()
35 changes: 33 additions & 2 deletions tests/cli/test_console.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,12 @@ def test_list_namespace(catalog: InMemoryCatalog) -> None:
assert result.output == "default.my_table\n"


def test_describe_namespace(catalog: InMemoryCatalog, namespace_properties: Properties) -> None:
@pytest.mark.parametrize("entity_args", [[], ["--entity", "namespace"]], ids=["any", "namespace"])
def test_describe_namespace(catalog: InMemoryCatalog, namespace_properties: Properties, entity_args: list[str]) -> None:
catalog.create_namespace(TEST_TABLE_NAMESPACE, namespace_properties)

runner = CliRunner()
result = runner.invoke(run, ["describe", "default"])
result = runner.invoke(run, ["describe", *entity_args, "default"])

assert result.exit_code == 0
assert result.output == "location s3://warehouse/database/location\n"
Expand Down Expand Up @@ -222,6 +223,36 @@ def test_describe_table_does_not_exists(catalog: InMemoryCatalog) -> None:
assert result.output == "Table or namespace does not exist: default.doesnotexist\n"


@pytest.mark.parametrize("entity_args", [[], ["--entity", "table"]], ids=["any", "table"])
def test_describe_table_entity_detection(catalog: InMemoryCatalog, mock_datetime_now: None, entity_args: list[str]) -> None:
catalog.create_namespace(TEST_TABLE_NAMESPACE)
catalog.create_table(
identifier=TEST_TABLE_IDENTIFIER,
schema=TEST_TABLE_SCHEMA,
partition_spec=TEST_TABLE_PARTITION_SPEC,
)

runner = CliRunner()
result = runner.invoke(run, ["describe", *entity_args, "default.my_table"])

assert result.exit_code == 0
assert "Table UUID" in result.output
assert "Current schema" in result.output


def test_describe_ambiguous_entity(catalog: InMemoryCatalog, namespace_properties: Properties) -> None:
catalog.create_namespace(TEST_TABLE_NAMESPACE)
catalog.create_table(identifier=TEST_TABLE_IDENTIFIER, schema=TEST_TABLE_SCHEMA)
catalog.create_namespace(TEST_TABLE_IDENTIFIER, namespace_properties)

runner = CliRunner()
result = runner.invoke(run, ["describe", "default.my_table"])
assert result.exit_code == 1
assert " ".join(result.output.split()) == (
"Identifier default.my_table matches multiple entity types: namespace, table. Use --entity to disambiguate."
)


def test_schema(catalog: InMemoryCatalog) -> None:
catalog.create_namespace(TEST_TABLE_NAMESPACE)
catalog.create_table(
Expand Down
Loading