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)