Fokko commented on code in PR #8570:
URL: https://github.com/apache/iceberg/pull/8570#discussion_r1329715528
##########
python/pyiceberg/cli/console.py:
##########
@@ -242,6 +242,29 @@ def rename(ctx: Context, from_identifier: str,
to_identifier: str) -> None:
catalog.rename_table(from_identifier, to_identifier)
output.text(f"Renamed table from {from_identifier} to {to_identifier}")
[email protected]()
[email protected]("identifier")
[email protected]("--type", required=False)
[email protected]("--verbose", type=click.BOOL)
[email protected]_context
+@catch_exception()
+def list_refs(ctx: Context, identifier: str, type: str, verbose: bool) -> None:
+ """List all the refs in the provided table"""
+ catalog, output = _catalog_and_output(ctx)
+ table = catalog.load_table(identifier)
+ refs = table.metadata.refs
+ if type:
+ type = type.lower()
+ if type != "branch" and type != "tag":
+ raise ValueError("Type must be either branch or tag")
+
+ relevant_refs = []
Review Comment:
A bit more pythonic, you can do this on a single line as well:
```suggestion
relevant_refs = [
ref_name, ref for ref_name, ref in refs.items() if not type or
ref.snapshot_ref_type == type
]
```
##########
python/pyiceberg/cli/console.py:
##########
@@ -242,6 +242,29 @@ def rename(ctx: Context, from_identifier: str,
to_identifier: str) -> None:
catalog.rename_table(from_identifier, to_identifier)
output.text(f"Renamed table from {from_identifier} to {to_identifier}")
[email protected]()
[email protected]("identifier")
[email protected]("--type", required=False)
[email protected]("--verbose", type=click.BOOL)
[email protected]_context
+@catch_exception()
+def list_refs(ctx: Context, identifier: str, type: str, verbose: bool) -> None:
+ """List all the refs in the provided table"""
+ catalog, output = _catalog_and_output(ctx)
+ table = catalog.load_table(identifier)
+ refs = table.metadata.refs
Review Comment:
We try to avoid accessing the metadata directly (that should have been
private ..)
##########
python/pyiceberg/cli/console.py:
##########
@@ -242,6 +242,29 @@ def rename(ctx: Context, from_identifier: str,
to_identifier: str) -> None:
catalog.rename_table(from_identifier, to_identifier)
output.text(f"Renamed table from {from_identifier} to {to_identifier}")
[email protected]()
[email protected]("identifier")
[email protected]("--type", required=False)
[email protected]("--verbose", type=click.BOOL)
[email protected]_context
+@catch_exception()
+def list_refs(ctx: Context, identifier: str, type: str, verbose: bool) -> None:
+ """List all the refs in the provided table"""
+ catalog, output = _catalog_and_output(ctx)
+ table = catalog.load_table(identifier)
+ refs = table.metadata.refs
+ if type:
+ type = type.lower()
+ if type != "branch" and type != "tag":
Review Comment:
Nit: I would consider this more Pythonic:
```suggestion
if type not in {"branch", "tag"}:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -127,6 +132,33 @@ def describe_table(self, table: Table) -> None:
output_table.add_row("Properties", table_properties)
Console().print(output_table)
+ def describe_refs(self, ref_tups: List[Tuple[str, SnapshotRef]]):
+ refs_table = RichTable(title="Snapshot Refs")
+ refs_table.add_column("Type")
+ refs_table.add_column("Ref")
+ refs_table.add_column("Max ref age ms")
+ refs_table.add_column("Min snapshots to keep")
+ refs_table.add_column("Max snapshot age ms")
+
+ for ref_tup in ref_tups:
Review Comment:
Less code is more:
```suggestion
for name, ref in ref_tups:
```
##########
python/pyiceberg/cli/output.py:
##########
@@ -127,6 +128,33 @@ def describe_table(self, table: Table) -> None:
output_table.add_row("Properties", table_properties)
Console().print(output_table)
+ def describe_refs(self, ref_tups: List[Tuple[str, SnapshotRef]]):
+ refs_table = RichTable(title="Snapshot Refs")
+ refs_table.add_column("Type")
+ refs_table.add_column("Ref")
+ refs_table.add_column("Max ref age ms")
+ refs_table.add_column("Min snapshots to keep")
+ refs_table.add_column("Max snapshot age ms")
+
+ for ref_tup in ref_tups:
+ name = ref_tup[0]
+ ref = ref_tup[1]
+ if ref.snapshot_ref_type == 'branch':
+ min_snapshots_to_keep = str(ref.min_snapshots_to_keep)
Review Comment:
I agree that it is best to pull in the table properties, and show the fall
back expiration time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
