blackmwk commented on code in PR #2380:
URL: https://github.com/apache/iceberg-rust/pull/2380#discussion_r3217364297
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -41,6 +41,8 @@ pub const SQL_CATALOG_PROP_URI: &str = "uri";
pub const SQL_CATALOG_PROP_WAREHOUSE: &str = "warehouse";
/// catalog sql bind style
pub const SQL_CATALOG_PROP_BIND_STYLE: &str = "sql_bind_style";
+/// catalog schema version, setting to "V1" will migrate from V0 to V1 schema
Review Comment:
Please make the comment more clear: this is expected catalog schema version.
If it's set to `v1` and it's actually `v0`, the catalog will do a migration.
##########
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");
Review Comment:
This is confusing, why not just print messages like `detect v1 schema`
##########
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:
JDBC has a method to check meta table to inspect the column if exists. Is
there anything similar in sqlx?
##########
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())
+ .unwrap_or(SchemaVersion::V0);
+ if requested == SchemaVersion::V1 {
+ tracing::debug!("{CATALOG_TABLE_NAME} is being updated to
support views");
Review Comment:
This should be a warning, and the message should be sth like `It's going to
be updated to v1`
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -41,6 +41,8 @@ pub const SQL_CATALOG_PROP_URI: &str = "uri";
pub const SQL_CATALOG_PROP_WAREHOUSE: &str = "warehouse";
/// catalog sql bind style
pub const SQL_CATALOG_PROP_BIND_STYLE: &str = "sql_bind_style";
+/// catalog schema version, setting to "V1" will migrate from V0 to V1 schema
+pub const SQL_CATALOG_PROP_SCHEMA_VERSION: &str = "sql.schema-version";
Review Comment:
+1.
##########
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())
+ .unwrap_or(SchemaVersion::V0);
+ if requested == SchemaVersion::V1 {
+ tracing::debug!("{CATALOG_TABLE_NAME} is being updated to
support views");
+ sqlx::query(&format!(
+ "ALTER TABLE {CATALOG_TABLE_NAME} \
+ ADD COLUMN {CATALOG_FIELD_RECORD_TYPE} VARCHAR(5)"
+ ))
+ .execute(&pool)
+ .await
+ .map_err(from_sqlx_error)?;
+ SchemaVersion::V1
+ } else {
+ tracing::warn!(
+ "SqlCatalog is initialized without view support. To
auto-migrate the database's schema and enable view support, set {}=V1",
+ SQL_CATALOG_PROP_SCHEMA_VERSION
+ );
+ SchemaVersion::V0
+ }
+ };
+
Ok(SqlCatalog {
name: config.name.to_owned(),
connection: pool,
warehouse_location: config.warehouse_location,
fileio,
sql_bind_style: config.sql_bind_style,
+ schema_version,
})
}
+ fn record_type_filter(&self) -> &'static str {
+ match self.schema_version {
+ SchemaVersion::V1 => "AND (iceberg_type = 'TABLE' OR iceberg_type
IS NULL)",
Review Comment:
I prefer to put these sqls into the `SchemaVersion` enum, as following:
```
impl SchemaVersion {
fn alter_table_sql(&self) -> Result<String> {
match self {
V0 => ...
V1 => ...
}
}
}
```
##########
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:
+1
--
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]
