added support to dynamically registering/adding partitions #327
Conversation
…specifying the partition columns in the glue table but for each external s3 write,we want to register those partitions and their s3 locations in glue too)
dbt/adapters/duckdb/plugins/glue.py
Outdated
| @@ -132,12 +133,64 @@ def _convert_columns(column_list: Sequence[Column]) -> Sequence["ColumnTypeDef"] | |||
| return column_types | |||
|
|
|||
|
|
|||
| def _create_table(client: "GlueClient", database: str, table_def: "TableInputTypeDef") -> None: | |||
| def _create_table( | |||
| client: "GlueClient", database: str, table_def: "TableInputTypeDef", partition_columns | |||
There was a problem hiding this comment.
let's throw some type info on the partition_columns to be consistent here
There was a problem hiding this comment.
Done! Decided to stick with List[Dict[str, str]] as I use the partition_columns variable for both creating & updating partitions, I would've preferred: List[ColumnTypeDef] but that doesn't line up with what PartitionInputTypeDef requires. Is that ok?
dbt/adapters/duckdb/plugins/glue.py
Outdated
|
|
||
| partition_location = table_def["StorageDescriptor"]["Location"] | ||
| for name, value in zip(partition_names, partition_values): | ||
| partition_location += f"/{name}={value}" |
There was a problem hiding this comment.
I have an aesthetic preference for something like:
partition_location = table_def["StorageDescriptor"]["Location"]
partition_components = [partition_location]
for c in partition_columns:
partition_components.append("=".join((c["Name"], c["Value"]))
partition_location = "/".join(partition_components)
dbt/adapters/duckdb/plugins/glue.py
Outdated
|
|
||
|
|
||
| def _update_table( | ||
| client: "GlueClient", database: str, table_def: "TableInputTypeDef", partition_columns |
There was a problem hiding this comment.
type info for partition_columns as above
dbt/adapters/duckdb/plugins/glue.py
Outdated
|
|
||
| partition_location = table_def["StorageDescriptor"]["Location"] | ||
| for name, value in zip(partition_names, partition_values): | ||
| partition_location += f"/{name}={value}" |
There was a problem hiding this comment.
whatever the logic ends up being for this partition path construction, let's move it to a method so we can unit test it and not repeat it in create/update table
| partition_input["StorageDescriptor"] = table_def["StorageDescriptor"] | ||
| partition_input["StorageDescriptor"]["Location"] = partition_location | ||
|
|
||
| return partition_input, partition_values |
There was a problem hiding this comment.
sorry, does this still work if partition_columns is empty? I figured it would throw an error since partition_values wouldn't be defined.
There was a problem hiding this comment.
I'm checking if partition_columns != [] before calling this function as it shouldn't be called with empty partition_colums. Do you prefer that check inside the _parse_partition_columns function?
There was a problem hiding this comment.
Oh I just meant that a default None for partition_values should be on the same line as the default value for partition_input
Added support to add new partitions (/locations) to existing or new Glue tables:
Previous PR only registered the Glue table and merely specified its partition columns.
What if, for each external s3 write, we want to register a new partition in Glue (2024/1/01; 2024/01/02; ...etc).