From a30f8063d4e4cc8bfcb788c6fc62aa30076fd7d0 Mon Sep 17 00:00:00 2001
From: iting0321 <0321iting@gmail.com>
Date: Sat, 22 Mar 2025 15:55:10 +0800
Subject: [PATCH 1/2] fix : command order and catalog not found
---
pyiceberg/catalog/__init__.py | 9 +++++++--
pyiceberg/cli/console.py | 36 ++++++++++++++++++++++++++++-------
pyiceberg/exceptions.py | 2 ++
tests/cli/test_console.py | 36 +++++++++++++++++++++++++++++------
4 files changed, 68 insertions(+), 15 deletions(-)
diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py
index cf649ba7d6..2f266a7ffa 100644
--- a/pyiceberg/catalog/__init__.py
+++ b/pyiceberg/catalog/__init__.py
@@ -43,6 +43,7 @@
NoSuchTableError,
NotInstalledError,
TableAlreadyExistsError,
+ NoCatalogError
)
from pyiceberg.io import FileIO, load_file_io
from pyiceberg.manifest import ManifestFile
@@ -234,9 +235,14 @@ def load_catalog(name: Optional[str] = None, **properties: Optional[str]) -> Cat
env = _ENV_CONFIG.get_catalog_config(name)
conf: RecursiveDict = merge_config(env or {}, cast(RecursiveDict, properties))
+ if conf == {}:
+ raise NoCatalogError(
+ f"{name} catalog not found, please provide a catalog type using --type"
+ )
+
catalog_type: Optional[CatalogType]
provided_catalog_type = conf.get(TYPE)
-
+
if catalog_impl := properties.get(PY_CATALOG_IMPL):
if provided_catalog_type:
raise ValueError(
@@ -255,7 +261,6 @@ def load_catalog(name: Optional[str] = None, **properties: Optional[str]) -> Cat
catalog_type = CatalogType(provided_catalog_type.lower())
elif not provided_catalog_type:
catalog_type = infer_catalog_type(name, conf)
-
if catalog_type:
return AVAILABLE_CATALOGS[catalog_type](name, cast(Dict[str, str], conf))
diff --git a/pyiceberg/cli/console.py b/pyiceberg/cli/console.py
index 83e67a3cbb..0f232a82e4 100644
--- a/pyiceberg/cli/console.py
+++ b/pyiceberg/cli/console.py
@@ -31,7 +31,7 @@
from pyiceberg import __version__
from pyiceberg.catalog import Catalog, load_catalog
from pyiceberg.cli.output import ConsoleOutput, JsonOutput, Output
-from pyiceberg.exceptions import NoSuchNamespaceError, NoSuchPropertyException, NoSuchTableError
+from pyiceberg.exceptions import NoSuchNamespaceError, NoSuchPropertyException, NoSuchTableError, NoCatalogError
from pyiceberg.table import TableProperties
from pyiceberg.table.refs import SnapshotRef
from pyiceberg.utils.properties import property_as_int
@@ -43,6 +43,11 @@ def decorator(func: Callable) -> Callable: # type: ignore
def wrapper(*args: Any, **kwargs: Any): # type: ignore
try:
return func(*args, **kwargs)
+ except NoCatalogError as e:
+ ctx: Context = click.get_current_context(silent=True)
+ ctx.obj["output"].exception(e)
+ ctx.exit(1)
+
except Exception as e:
ctx: Context = click.get_current_context(silent=True)
_, output = _catalog_and_output(ctx)
@@ -86,31 +91,48 @@ def run(
ctx.obj["output"] = JsonOutput(verbose=verbose)
try:
- ctx.obj["catalog"] = load_catalog(catalog, **properties)
+ ctx.obj["first_level_catalog"] = catalog
+ ctx.obj["second_level_catalog"] = None
+ ctx.obj["properties"] = properties
+
except Exception as e:
ctx.obj["output"].exception(e)
ctx.exit(1)
+
+def _load_the_catalog_with_first_level_catalog(ctx: Context) -> None:
+ if ctx.obj["first_level_catalog"] is None and ctx.obj["second_level_catalog"] is None:
+ ctx.obj["catalog"] = load_catalog(None, **ctx.obj["properties"])
+ elif ctx.obj["first_level_catalog"] is not None and ctx.obj["second_level_catalog"] is None:
+ ctx.obj["catalog"] = load_catalog(ctx.obj["first_level_catalog"],**ctx.obj["properties"])
+ else:
+ ctx.obj["catalog"] = load_catalog(ctx.obj["second_level_catalog"],**ctx.obj["properties"])
+ print("cataloglll",ctx.obj["catalog"])
+
+
if not isinstance(ctx.obj["catalog"], Catalog):
ctx.obj["output"].exception(
- ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported")
+ ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported")
)
ctx.exit(1)
-
def _catalog_and_output(ctx: Context) -> Tuple[Catalog, Output]:
- """Small helper to set the types."""
+ """Small helper to set the types and load the catalog."""
+ _load_the_catalog_with_first_level_catalog(ctx)
return ctx.obj["catalog"], ctx.obj["output"]
@run.command()
+@click.option("--catalog")
@click.pass_context
@click.argument("parent", required=False)
@catch_exception()
-def list(ctx: Context, parent: Optional[str]) -> None: # pylint: disable=redefined-builtin
+def list(ctx: Context, catalog: Optional[str], parent: Optional[str]) -> None: # pylint: disable=redefined-builtin
"""List tables or namespaces."""
+ if ctx.obj["first_level_catalog"] == None:
+ ctx.obj["second_level_catalog"] = catalog
+
catalog, output = _catalog_and_output(ctx)
-
identifiers = []
if parent:
# Do we have tables under parent namespace?
diff --git a/pyiceberg/exceptions.py b/pyiceberg/exceptions.py
index 56574ff471..2f38f9125b 100644
--- a/pyiceberg/exceptions.py
+++ b/pyiceberg/exceptions.py
@@ -27,6 +27,8 @@ class NamespaceNotEmptyError(Exception):
class NamespaceAlreadyExistsError(Exception):
"""Raised when a name-space being created already exists in the catalog."""
+class NoCatalogError(Exception):
+ """Raised when no catalog is set."""
class ValidationError(Exception):
"""Raises when there is an issue with the schema."""
diff --git a/tests/cli/test_console.py b/tests/cli/test_console.py
index 70e04071ad..0e31947c4b 100644
--- a/tests/cli/test_console.py
+++ b/tests/cli/test_console.py
@@ -35,18 +35,28 @@
from pyiceberg.types import LongType, NestedField
from pyiceberg.utils.config import Config
+def test_no_catalog_error(mocker: MockFixture, empty_home_dir_path: str) -> None:
+ mocker.patch.dict(os.environ, values={"HOME": empty_home_dir_path, "PYICEBERG_HOME": empty_home_dir_path})
+ runner = CliRunner()
+ result = runner.invoke(run, ["list", "--catalog", "nocatalog"], catch_exceptions=False)
+ assert result.exit_code == 1
+ assert result.output == "nocatalog catalog not found, please provide a catalog type using --type\n"
+
+
def test_missing_uri(mocker: MockFixture, empty_home_dir_path: str) -> None:
# mock to prevent parsing ~/.pyiceberg.yaml or {PYICEBERG_HOME}/.pyiceberg.yaml
mocker.patch.dict(os.environ, values={"HOME": empty_home_dir_path, "PYICEBERG_HOME": empty_home_dir_path})
- mocker.patch("pyiceberg.catalog._ENV_CONFIG", return_value=Config())
+ mocker.patch("pyiceberg.catalog._ENV_CONFIG.get_catalog_config", return_value={
+ "catalog": {"production": {}},
+ "home": empty_home_dir_path,
+ }
+ )
runner = CliRunner()
- result = runner.invoke(run, ["list"])
-
- assert result.exit_code == 1
- assert result.output == "Could not initialize catalog with the following properties: {}\n"
-
+ with pytest.raises(ValueError, match="URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__PRODUCTION__URI"):
+ result = runner.invoke(run, ["list", "--catalog", "production"], catch_exceptions=False)
+ assert result.exit_code == 1
@pytest.fixture(autouse=True)
def env_vars(mocker: MockFixture) -> None:
@@ -115,6 +125,20 @@ def test_list_namespace(catalog: InMemoryCatalog) -> None:
assert result.exit_code == 0
assert result.output == "default.my_table\n"
+def test_list_with_catalog_option_wrong_order(catalog: InMemoryCatalog):
+ """Test that `pyiceberg list --catalog hive` raises an error due to wrong order."""
+ catalog.create_namespace(TEST_TABLE_NAMESPACE)
+ catalog.create_table(
+ identifier=TEST_TABLE_IDENTIFIER,
+ schema=TEST_TABLE_SCHEMA,
+ partition_spec=TEST_TABLE_PARTITION_SPEC,
+ properties=TEST_TABLE_PROPERTIES,
+ )
+ runner = CliRunner()
+ result = runner.invoke(run, ["list", "--catalog", "hive"])
+ assert result.exit_code == 0
+ print("result.output", result.output)
+
def test_describe_namespace(catalog: InMemoryCatalog, namespace_properties: Properties) -> None:
catalog.create_namespace(TEST_TABLE_NAMESPACE, namespace_properties)
From f4cc40f2376ea972e679c22314f8dd984011f6a9 Mon Sep 17 00:00:00 2001
From: iting0321 <0321iting@gmail.com>
Date: Sat, 22 Mar 2025 23:40:42 +0800
Subject: [PATCH 2/2] refactor: clean up code formatting and lint errors
---
pyiceberg/catalog/__init__.py | 10 ++++-----
pyiceberg/cli/console.py | 39 ++++++++++++++++-------------------
pyiceberg/exceptions.py | 2 ++
tests/cli/test_console.py | 16 +++++++++-----
4 files changed, 35 insertions(+), 32 deletions(-)
diff --git a/pyiceberg/catalog/__init__.py b/pyiceberg/catalog/__init__.py
index 2f266a7ffa..f63ec2fbea 100644
--- a/pyiceberg/catalog/__init__.py
+++ b/pyiceberg/catalog/__init__.py
@@ -39,11 +39,11 @@
from pyiceberg.exceptions import (
NamespaceAlreadyExistsError,
+ NoCatalogError,
NoSuchNamespaceError,
NoSuchTableError,
NotInstalledError,
TableAlreadyExistsError,
- NoCatalogError
)
from pyiceberg.io import FileIO, load_file_io
from pyiceberg.manifest import ManifestFile
@@ -236,13 +236,11 @@ def load_catalog(name: Optional[str] = None, **properties: Optional[str]) -> Cat
conf: RecursiveDict = merge_config(env or {}, cast(RecursiveDict, properties))
if conf == {}:
- raise NoCatalogError(
- f"{name} catalog not found, please provide a catalog type using --type"
- )
-
+ raise NoCatalogError(f"{name} catalog not found, please provide a catalog type using --type")
+
catalog_type: Optional[CatalogType]
provided_catalog_type = conf.get(TYPE)
-
+
if catalog_impl := properties.get(PY_CATALOG_IMPL):
if provided_catalog_type:
raise ValueError(
diff --git a/pyiceberg/cli/console.py b/pyiceberg/cli/console.py
index 0f232a82e4..4b12383e0b 100644
--- a/pyiceberg/cli/console.py
+++ b/pyiceberg/cli/console.py
@@ -31,7 +31,7 @@
from pyiceberg import __version__
from pyiceberg.catalog import Catalog, load_catalog
from pyiceberg.cli.output import ConsoleOutput, JsonOutput, Output
-from pyiceberg.exceptions import NoSuchNamespaceError, NoSuchPropertyException, NoSuchTableError, NoCatalogError
+from pyiceberg.exceptions import NoCatalogError, NoSuchNamespaceError, NoSuchPropertyException, NoSuchTableError
from pyiceberg.table import TableProperties
from pyiceberg.table.refs import SnapshotRef
from pyiceberg.utils.properties import property_as_int
@@ -42,14 +42,13 @@ def decorator(func: Callable) -> Callable: # type: ignore
@wraps(func)
def wrapper(*args: Any, **kwargs: Any): # type: ignore
try:
+ ctx: Context = click.get_current_context(silent=True)
return func(*args, **kwargs)
except NoCatalogError as e:
- ctx: Context = click.get_current_context(silent=True)
ctx.obj["output"].exception(e)
ctx.exit(1)
-
+
except Exception as e:
- ctx: Context = click.get_current_context(silent=True)
_, output = _catalog_and_output(ctx)
output.exception(e)
ctx.exit(1)
@@ -94,52 +93,50 @@ def run(
ctx.obj["first_level_catalog"] = catalog
ctx.obj["second_level_catalog"] = None
ctx.obj["properties"] = properties
-
+
except Exception as e:
ctx.obj["output"].exception(e)
ctx.exit(1)
def _load_the_catalog_with_first_level_catalog(ctx: Context) -> None:
- if ctx.obj["first_level_catalog"] is None and ctx.obj["second_level_catalog"] is None:
- ctx.obj["catalog"] = load_catalog(None, **ctx.obj["properties"])
- elif ctx.obj["first_level_catalog"] is not None and ctx.obj["second_level_catalog"] is None:
- ctx.obj["catalog"] = load_catalog(ctx.obj["first_level_catalog"],**ctx.obj["properties"])
- else:
- ctx.obj["catalog"] = load_catalog(ctx.obj["second_level_catalog"],**ctx.obj["properties"])
- print("cataloglll",ctx.obj["catalog"])
-
+ catalog_option = ctx.obj.get("first_level_catalog", None) or ctx.obj.get("second_level_catalog", None)
+ ctx.obj["catalog"] = load_catalog(catalog_option, **ctx.obj["properties"])
if not isinstance(ctx.obj["catalog"], Catalog):
ctx.obj["output"].exception(
- ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported")
+ ValueError("Could not determine catalog type from uri. REST (http/https) and Hive (thrift) is supported")
)
ctx.exit(1)
+
def _catalog_and_output(ctx: Context) -> Tuple[Catalog, Output]:
"""Small helper to set the types and load the catalog."""
_load_the_catalog_with_first_level_catalog(ctx)
return ctx.obj["catalog"], ctx.obj["output"]
-@run.command()
+@run.command(name="list")
@click.option("--catalog")
@click.pass_context
@click.argument("parent", required=False)
@catch_exception()
-def list(ctx: Context, catalog: Optional[str], parent: Optional[str]) -> None: # pylint: disable=redefined-builtin
+def list_entities(ctx: Context, catalog: Optional[str], parent: Optional[str]) -> None: # pylint: disable=redefined-builtin
"""List tables or namespaces."""
- if ctx.obj["first_level_catalog"] == None:
+ if ctx.obj["first_level_catalog"] is None:
ctx.obj["second_level_catalog"] = catalog
-
- catalog, output = _catalog_and_output(ctx)
+
+ catalog_obj, output = _catalog_and_output(ctx)
+
identifiers = []
+
if parent:
# Do we have tables under parent namespace?
- identifiers = catalog.list_tables(parent)
+ identifiers = catalog_obj.list_tables(parent)
+
if not identifiers:
# List hierarchical namespaces if parent, root namespaces otherwise.
- identifiers = catalog.list_namespaces(parent or ())
+ identifiers = catalog_obj.list_namespaces(parent or ())
output.identifiers(identifiers)
diff --git a/pyiceberg/exceptions.py b/pyiceberg/exceptions.py
index 2f38f9125b..8eb8362669 100644
--- a/pyiceberg/exceptions.py
+++ b/pyiceberg/exceptions.py
@@ -27,9 +27,11 @@ class NamespaceNotEmptyError(Exception):
class NamespaceAlreadyExistsError(Exception):
"""Raised when a name-space being created already exists in the catalog."""
+
class NoCatalogError(Exception):
"""Raised when no catalog is set."""
+
class ValidationError(Exception):
"""Raises when there is an issue with the schema."""
diff --git a/tests/cli/test_console.py b/tests/cli/test_console.py
index 0e31947c4b..cec4c69f9c 100644
--- a/tests/cli/test_console.py
+++ b/tests/cli/test_console.py
@@ -33,7 +33,7 @@
from pyiceberg.transforms import IdentityTransform
from pyiceberg.typedef import Properties
from pyiceberg.types import LongType, NestedField
-from pyiceberg.utils.config import Config
+
def test_no_catalog_error(mocker: MockFixture, empty_home_dir_path: str) -> None:
mocker.patch.dict(os.environ, values={"HOME": empty_home_dir_path, "PYICEBERG_HOME": empty_home_dir_path})
@@ -41,23 +41,28 @@ def test_no_catalog_error(mocker: MockFixture, empty_home_dir_path: str) -> None
result = runner.invoke(run, ["list", "--catalog", "nocatalog"], catch_exceptions=False)
assert result.exit_code == 1
assert result.output == "nocatalog catalog not found, please provide a catalog type using --type\n"
-
def test_missing_uri(mocker: MockFixture, empty_home_dir_path: str) -> None:
# mock to prevent parsing ~/.pyiceberg.yaml or {PYICEBERG_HOME}/.pyiceberg.yaml
mocker.patch.dict(os.environ, values={"HOME": empty_home_dir_path, "PYICEBERG_HOME": empty_home_dir_path})
- mocker.patch("pyiceberg.catalog._ENV_CONFIG.get_catalog_config", return_value={
+ mocker.patch(
+ "pyiceberg.catalog._ENV_CONFIG.get_catalog_config",
+ return_value={
"catalog": {"production": {}},
"home": empty_home_dir_path,
- }
+ },
)
runner = CliRunner()
- with pytest.raises(ValueError, match="URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__PRODUCTION__URI"):
+ with pytest.raises(
+ ValueError,
+ match="URI missing, please provide using --uri, the config or environment variable PYICEBERG_CATALOG__PRODUCTION__URI",
+ ):
result = runner.invoke(run, ["list", "--catalog", "production"], catch_exceptions=False)
assert result.exit_code == 1
+
@pytest.fixture(autouse=True)
def env_vars(mocker: MockFixture) -> None:
mocker.patch.dict(os.environ, MOCK_ENVIRONMENT)
@@ -125,6 +130,7 @@ def test_list_namespace(catalog: InMemoryCatalog) -> None:
assert result.exit_code == 0
assert result.output == "default.my_table\n"
+
def test_list_with_catalog_option_wrong_order(catalog: InMemoryCatalog):
"""Test that `pyiceberg list --catalog hive` raises an error due to wrong order."""
catalog.create_namespace(TEST_TABLE_NAMESPACE)