Python: improve Glue catalog using Boto3 types #8304
Conversation
| PROP_GLUE_TABLE = "Table" | ||
| PROP_GLUE_TABLE_TYPE = "TableType" | ||
| PROP_GLUE_TABLE_DESCRIPTION = "Description" | ||
| PROP_GLUE_TABLE_PARAMETERS = "Parameters" | ||
| PROP_GLUE_TABLE_DATABASE_NAME = "DatabaseName" | ||
| PROP_GLUE_TABLE_NAME = "Name" | ||
| PROP_GLUE_TABLE_OWNER = "Owner" | ||
| PROP_GLUE_TABLE_STORAGE_DESCRIPTOR = "StorageDescriptor" |
There was a problem hiding this comment.
I see that with the incorporation of Boto3 types, we now have an enhanced verification mechanism for key names when defining table inputs and parameters. However, in my perspective, it's still beneficial to retain the use of string constants, both to minimize the chance of typos and to ensure consistency throughout our codebase. For context, we've employed this approach with string constants in both hive.py and rest.py:
iceberg/python/pyiceberg/catalog/hive.py
Lines 149 to 152 in cf358a4
iceberg/python/pyiceberg/catalog/rest.py
Lines 90 to 98 in cf358a4
If the concern revolves around the clarity of the variable names, perhaps we can consider simplifying them? For example:
TABLE = "Table"
TABLE_TYPE = "TableType"
...
I'd truly appreciate your perspective on this. Do you think this approach would be beneficial, or do you have alternative suggestions?
There was a problem hiding this comment.
I think mypy sufficiently protects against typos. Having variables with literal strings doesn't seem very useful to me, I prefer just to rely on the types defined by mypy_boto3_glue.
python/pyiceberg/catalog/glue.py
Outdated
| properties: Properties = glue_table["Parameters"] | ||
|
|
||
| def _convert_glue_to_iceberg(self, glue_table: Dict[str, Any]) -> Table: | ||
| properties: Properties = glue_table.get(PROP_GLUE_TABLE_PARAMETERS, {}) | ||
| database_name = glue_table["DatabaseName"] |
There was a problem hiding this comment.
Do we want to add null check for "Parameters" and "DatabaseName" as they are specified as NotRequired according to https://youtype.github.io/boto3_stubs_docs/mypy_boto3_glue/type_defs/#tabletypedef ?
There was a problem hiding this comment.
yes, I've added assertions for those properties.
| @@ -441,12 +441,13 @@ def test_update_namespace_properties_overlap_update_removal( | |||
|
|
|||
| @mock_glue | |||
| def test_passing_profile_name() -> None: | |||
| session_properties = { | |||
| session_properties: Dict[str, Any] = { | |||
There was a problem hiding this comment.
nit
| session_properties: Dict[str, Any] = { | |
| session_properties: Dict[str, Optional[str]] = { |
Improve the type checking of the Glue catalog module by using mypy_boto3_glue type definitions for the AWS API calls. Eliminate the use of many literal types.
Update poetry.lock with the new dependency.