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.
Let me know if its preferred to make the change.
| // 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)
There was a problem hiding this comment.
I had a deeper look. AI helped me find that sqlx actually abstracts this behind a describe method, and provides an agnostic way to get a list of columns from a SELECT. For the backend, it may execute 2 or more queries to do this compared to the single query for the previous probing approach, but I think that's justified for better error handling.
I've updated a PR that uses this, so it'll propagate errors if it couldn't get a list of columns, otherwise we check the list for the presence of the iceberg_type column.
There was a problem hiding this comment.
Pull request overview
This PR updates the SQL catalog implementation to interoperate with existing Iceberg SQL catalog databases that still use the legacy (V0) iceberg_tables schema (without the iceberg_type column), while keeping the current (V1) behavior available via an explicit opt-in migration flag.
Changes:
- Added schema version detection for
iceberg_tablesand a newsql.schema-versionproperty to optionally migrate V0 → V1. - Updated table lookup queries to only apply the
iceberg_typefilter when the detected schema supports it. - Added unit tests covering schema detection, migration opt-in, legacy bind-style key compatibility, and invalid schema-version validation.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/catalog/sql/src/catalog.rs | Implements schema version detection/migration and conditionally filters table queries based on schema version; adds new tests. |
| crates/catalog/sql/public-api.txt | Updates the public API surface to include SchemaVersion and the new SQL_CATALOG_PROP_SCHEMA_VERSION constant. |
| crates/catalog/sql/Cargo.toml | Adds tracing dependency for new diagnostic logging. |
| Cargo.lock | Locks the added tracing dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ail, clarify docs on created catalog table behavior
25fdb9b to
36d4c5a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/catalog/sql/src/catalog.rs:343
SchemaVersion::detectchecks for theiceberg_typecolumn using a case-sensitive string comparison. SQL identifiers are generally case-insensitive and some backends/driver metadata can report column names with different casing, which could mis-detect a V1 table as V0. Consider using an ASCII-case-insensitive comparison here (and in the similar test helper) to make detection robust across backends.
let has_type_column = catalog_table_description
.columns()
.iter()
.any(|column| column.name() == CATALOG_FIELD_RECORD_TYPE);
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.SQL catalog tables are always created V1, regardless of the config. This does diverge with Java for now.
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
AI was used to find out that
sqlxdoes offer a backend-agnostic way to get a list of columns for the table. It also drafted extended tests for the newdetectmethod on the schema version enum.