fix: automatically migrate V0 SqlCatalog catalog tables#2380
fix: automatically migrate V0 SqlCatalog catalog tables#2380rchowell wants to merge 1 commit intoapache:mainfrom
Conversation
| .map_err(from_sqlx_error)?; | ||
|
|
||
| // Check if the catalog table has the iceberg_type column, if not, it's a V0 schema. | ||
| let needs_migration = sqlx::query(&format!( |
There was a problem hiding this comment.
I don't think it's a good idea to do migration silently. We should do followings:
- Add a config to do auto migration if need, and this should be disabled by default.
- If auto migration is disabled, and we detected that it's v0, we should return error.
- Add a pub api in SqlCatalog to allow user to do migration.
There was a problem hiding this comment.
I agree, however this is the same behavior as iceberg-java which by default does auto-migration. https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java#L157
I am happy to change this if you still believe iceberg-rust should have a different behavior. Any thoughts on the config name since it does not exist in other libs?
There was a problem hiding this comment.
I still don't think this is the right behavior to silently upgrade db schema. Also it's not upgrading silently, it will check a property: https://github.com/apache/iceberg/blob/b809dcd770d3c10cc6d81b70dc198422749cfa0e/core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java#L246
As with the property name, how about make it similar java version: sql.schema-version ?
2 participants
Which issue does this PR close?
What changes are included in this PR?
Probe to see if we have a V0 or V1 table, then add the iceberg_type column if it does not exist. More details in apache/iceberg-python#3263
Are these changes tested?