feat: support V0 iceberg_tables schema for SqlCatalog - #3032
Conversation
CTTY
left a comment
CTTY
left a comment
There was a problem hiding this comment.
Thanks for reviving this PR! mostly LGTM
| let sql_bind_style = self | ||
| .config | ||
| .props | ||
| .remove(SQL_CATALOG_PROP_BIND_STYLE) |
There was a problem hiding this comment.
Why do we need to consume the property here?
There was a problem hiding this comment.
I noticed this too but didn't have a preference. It is already established convention in the file. The URI and warehouse location would need to clone if we only borrowed from the config map.
I'm happy with it as is, but I can also move the rest of the load method to borrow (and clone if needed) instead of consume.
| // Parse the requested schema version up front so invalid values fail fast rather than | ||
| // silently falling back to V0. | ||
| let mut valid_schema_version = true; | ||
| if let Some(schema_version) = self.config.props.remove(SQL_CATALOG_PROP_SCHEMA_VERSION) { |
There was a problem hiding this comment.
Will address comment in this thread: #3032 (comment)
| Ok(_) => true, | ||
| // The database rejected the query: the `iceberg_type` column (or table) is absent, | ||
| // so this is a genuine V0 schema. | ||
| Err(sqlx::Error::Database(_)) => false, |
There was a problem hiding this comment.
This is still my biggest concern, there is no way for us to ensure that this is not a permission failure. I was thinking about something like an unexhastive enum to at least check the specific errors for Postgres, MySql, Sqlite. or let's create a follow up to add verification calls directly via different drivers like mentioned here: #2380 (comment)
3 participants
Which issue does this PR close?
Closes #2068.
This change supercedes #2380 originally authored by @rchowell.
What changes are included in this PR?
This PR adds support for using a V0 SqlCatalog from other implementations like iceberg-python or iceberg-java, and it follows the iceberg-java behavior of checking an explicit
schema-versionproperty and migrating to V1 only if the user requested this.The catalog probes to see if we have a V0 or V1 table, then add the iceberg_type column if it does not exist. Reference: apache/iceberg-python#3263
Are these changes tested?
AI Disclosure
No AI has been used on the changes since #2380.