Copilot commented on code in PR #2660:
URL: https://github.com/apache/iceberg-python/pull/2660#discussion_r2462246335
##########
pyiceberg/catalog/glue.py:
##########
@@ -355,34 +355,29 @@ def __init__(self, name: str, client:
Optional["GlueClient"] = None, **propertie
_register_glue_catalog_id_with_glue_client(self.glue,
glue_catalog_id)
def _convert_glue_to_iceberg(self, glue_table: "TableTypeDef") -> Table:
- properties: Properties = glue_table["Parameters"]
-
- database_name = glue_table.get("DatabaseName", None)
- if database_name is None:
+ if (database_name := glue_table.get("DatabaseName")) is None:
raise ValueError("Glue table is missing DatabaseName property")
- parameters = glue_table.get("Parameters", None)
- if parameters is None:
- raise ValueError("Glue table is missing Parameters property")
+ if (table_name := glue_table.get("Name")) is None:
+ raise ValueError("Glue table is missing Name property")
- table_name = glue_table["Name"]
+ if (parameters := glue_table.get("Parameters")) is None:
+ raise ValueError("Glue table is missing Parameters property")
- if TABLE_TYPE not in properties:
+ if (glue_table_type := parameters.get(TABLE_TYPE)) is None:
raise NoSuchPropertyException(
f"Property {TABLE_TYPE} missing, could not determine type:
{database_name}.{table_name}"
)
- glue_table_type = properties[TABLE_TYPE]
if glue_table_type.lower() != ICEBERG:
raise NoSuchIcebergTableError(
f"Property table_type is {glue_table_type}, expected
{ICEBERG}: {database_name}.{table_name}"
)
- if METADATA_LOCATION not in properties:
+ if (metadata_location := parameters.get(METADATA_LOCATION)) is None:
raise NoSuchPropertyException(
f"Table property {METADATA_LOCATION} is missing, cannot find
metadata for: {database_name}.{table_name}"
Review Comment:
The condition `is None` will not catch empty strings. If `METADATA_LOCATION`
could be an empty string, this check would incorrectly allow it through,
potentially causing issues when trying to load the file. Consider using `if not
(metadata_location := parameters.get(METADATA_LOCATION)):` to reject empty
strings as well.
```suggestion
if not (metadata_location := parameters.get(METADATA_LOCATION)):
raise NoSuchPropertyException(
f"Table property {METADATA_LOCATION} is missing or empty,
cannot find metadata for: {database_name}.{table_name}"
```
##########
pyiceberg/catalog/glue.py:
##########
@@ -355,34 +355,29 @@ def __init__(self, name: str, client:
Optional["GlueClient"] = None, **propertie
_register_glue_catalog_id_with_glue_client(self.glue,
glue_catalog_id)
def _convert_glue_to_iceberg(self, glue_table: "TableTypeDef") -> Table:
- properties: Properties = glue_table["Parameters"]
-
- database_name = glue_table.get("DatabaseName", None)
- if database_name is None:
+ if (database_name := glue_table.get("DatabaseName")) is None:
raise ValueError("Glue table is missing DatabaseName property")
- parameters = glue_table.get("Parameters", None)
- if parameters is None:
- raise ValueError("Glue table is missing Parameters property")
+ if (table_name := glue_table.get("Name")) is None:
+ raise ValueError("Glue table is missing Name property")
- table_name = glue_table["Name"]
+ if (parameters := glue_table.get("Parameters")) is None:
+ raise ValueError("Glue table is missing Parameters property")
- if TABLE_TYPE not in properties:
+ if (glue_table_type := parameters.get(TABLE_TYPE)) is None:
raise NoSuchPropertyException(
f"Property {TABLE_TYPE} missing, could not determine type:
{database_name}.{table_name}"
Review Comment:
The condition `is None` will not catch empty strings or other falsy values.
If `TABLE_TYPE` could be an empty string, this check would incorrectly allow it
through. Consider using `if not (glue_table_type :=
parameters.get(TABLE_TYPE)):` to reject empty strings as well, or keep `is
None` if empty strings are valid values for this parameter.
```suggestion
if not (glue_table_type := parameters.get(TABLE_TYPE)):
raise NoSuchPropertyException(
f"Property {TABLE_TYPE} missing or empty, could not
determine type: {database_name}.{table_name}"
```
--
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]
