feat: support V0 iceberg_tables schema for SqlCatalog #2380
Conversation
rchowell
changed the title
|
Ok @blackmwk thanks again for reviewing, I was mistaken and have fixed this PR to match iceberg-java much more closely. I also updated the PR title and description to be more accurate since this is more of a feature to support reading V0 tables in iceberg-rust. Thanks! |
| let tbl_metadata_location_str = tbl_metadata_location.to_string(); | ||
| self.execute(&format!( | ||
| "INSERT INTO {CATALOG_TABLE_NAME} | ||
| ({CATALOG_FIELD_CATALOG_NAME}, {CATALOG_FIELD_TABLE_NAMESPACE}, {CATALOG_FIELD_TABLE_NAME}, {CATALOG_FIELD_METADATA_LOCATION_PROP}, {CATALOG_FIELD_RECORD_TYPE}) |
There was a problem hiding this comment.
Should we consider V0 schema here as well?
There was a problem hiding this comment.
In iceberg-java, it looks like we only support creating V0 table. So I think it's fine for us to leave it here.
But still quite curious about the case when TableCreation has V1 schema.
There was a problem hiding this comment.
To clarify on outcome, we fully support V1 catalog, migration from V0 to V1 catalog, and we support all catalog operations on V0 except for table creation and registration.
If we really want, we could later support mutations of V0 SQL catalog (outside of migration to V1).
Can we consider (maybe its OK as follow-up) to explicitly error to say this isn't supported?
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
Generally LGTM. One small comment on the error handling around table creation/registration.
| let tbl_metadata_location_str = tbl_metadata_location.to_string(); | ||
| self.execute(&format!( | ||
| "INSERT INTO {CATALOG_TABLE_NAME} | ||
| ({CATALOG_FIELD_CATALOG_NAME}, {CATALOG_FIELD_TABLE_NAMESPACE}, {CATALOG_FIELD_TABLE_NAME}, {CATALOG_FIELD_METADATA_LOCATION_PROP}, {CATALOG_FIELD_RECORD_TYPE}) |
There was a problem hiding this comment.
To clarify on outcome, we fully support V1 catalog, migration from V0 to V1 catalog, and we support all catalog operations on V0 except for table creation and registration.
If we really want, we could later support mutations of V0 SQL catalog (outside of migration to V1).
Can we consider (maybe its OK as follow-up) to explicitly error to say this isn't supported?
4 participants
Which issue does this PR close?
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. I also added logging at the same level as iceberg-java.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?