Get, Set, Remove schema_properties#960
Get, Set, Remove schema_properties#960yan-alex wants to merge 32 commits intoduckdb:v1.5-variegatafrom
Conversation
require-env SECRETS_CREATED_AND_CATALOG_ATTACHED
Tmonster
left a comment
Tmonster
left a comment
There was a problem hiding this comment.
Thanks! A couple of questions
| vector<unique_ptr<IcebergTransactionUpdate>> transaction_updates; | ||
| //! The latest state of a table (either points into 'transaction_updates' or 'tables') | ||
| case_insensitive_map_t<IcebergTransactionTableState> current_table_data; | ||
| case_insensitive_map_t<case_insensitive_map_t<string>> current_schema_properties; |
There was a problem hiding this comment.
nit:
could you re-use the SchemaPropertyUpdates struct here as well?
i.e case_insentive_map<SchemaPropertyUpdates>
There was a problem hiding this comment.
Good thing you brought it up, I removed this field for now.
I feel like this transaction state tracking should be done as part of a bigger change to keep track of the transaction schema state which we currently don't do. So for this PR I think we just use IcebergSchemaEntry
|
@Tmonster addressed your feeedback. CI failure is from networking |
Tmonster
left a comment
Tmonster
left a comment
There was a problem hiding this comment.
Thanks! I'm not sure this will work for nested namespaces unfortunately.
| } | ||
| auto create_body = JsonDocToString(std::move(doc_p)); | ||
|
|
||
| IRCAPI::CommitNamespacePropertiesUpdate(context, ic_catalog, create_body, schema_name_no_catalog); |
There was a problem hiding this comment.
It looks like the namespace properties update does consider nested namespaces. Can we add some tests to make sure that also works?
There was a problem hiding this comment.
@Tmonster added support for nested namespaces
3 participants
https://github.com/duckdblabs/duckdb-internal/issues/8996
Added the following functions
iceberg_schema_properties()set_iceberg_schema_properties()remove_iceberg_schema_properties()