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)