CTTY commented on code in PR #2380:
URL: https://github.com/apache/iceberg-rust/pull/2380#discussion_r3210731273
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -297,15 +310,60 @@ impl SqlCatalog {
.await
.map_err(from_sqlx_error)?;
+ // Check if the catalog table supports views, indicating that the
schema is V1
+ let is_v1 = sqlx::query(&format!(
+ "SELECT {CATALOG_FIELD_RECORD_TYPE} FROM {CATALOG_TABLE_NAME}
LIMIT 0"
+ ))
+ .execute(&pool)
+ .await
+ .is_ok();
Review Comment:
What if the SELECT failed here but the schema is actually V1?
I assume in that case it would pass silently here and fail later when trying
to update it. This would be confusing to users
With above said, I don't have a better idea to probe the schema type... at
least we should check the error type? any thoughts?
##########
crates/catalog/sql/src/catalog.rs:
##########
Review Comment:
Should we consider V0 schema here as well?
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -297,15 +310,60 @@ impl SqlCatalog {
.await
.map_err(from_sqlx_error)?;
+ // Check if the catalog table supports views, indicating that the
schema is V1
+ let is_v1 = sqlx::query(&format!(
+ "SELECT {CATALOG_FIELD_RECORD_TYPE} FROM {CATALOG_TABLE_NAME}
LIMIT 0"
+ ))
+ .execute(&pool)
+ .await
+ .is_ok();
+
+ // Migrate the schema to V1 if the catalog table does not support
views and the caller opted in.
+ let schema_version = if is_v1 {
+ tracing::debug!("{CATALOG_TABLE_NAME} already supports views");
+ SchemaVersion::V1
+ } else {
+ let requested: SchemaVersion = config
+ .props
+ .get(SQL_CATALOG_PROP_SCHEMA_VERSION)
+ .and_then(|v| v.parse().ok())
Review Comment:
We should fail on parse error here to avoid silent fallback for misinputs
Similar to sql_bind_style parsing:
https://github.com/apache/iceberg-rust/blob/main/crates/catalog/sql/src/catalog.rs#L183
##########
crates/catalog/sql/src/lib.rs:
##########
@@ -29,7 +29,7 @@
//! SqlBindStyle, SqlCatalogBuilder,
//! };
//!
-//! #[tokio::main]
+//! #[tokio::main(flavor = "current_thread")]
Review Comment:
why do we need to change this?
##########
crates/catalog/sql/src/catalog.rs:
##########
Review 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.
https://github.com/apache/iceberg/blob/bb37293484468f0502de9986955860505aef9776/core/src/main/java/org/apache/iceberg/jdbc/JdbcUtil.java#L128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
