Copilot commented on code in PR #3032:
URL: https://github.com/apache/iceberg-rust/pull/3032#discussion_r3831514627
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -268,6 +310,62 @@ pub struct SqlCatalog {
sql_bind_style: SqlBindStyle,
runtime: Runtime,
kms_client: Option<Arc<dyn KeyManagementClient>>,
+ schema_version: SchemaVersion,
+}
+
+#[derive(Debug, Clone, Copy, PartialEq, strum::EnumString, strum::Display)]
+#[strum(ascii_case_insensitive)]
+/// Schema version of the `iceberg_tables` catalog table.
+pub enum SchemaVersion {
+ /// Original schema without the `iceberg_type` column.
+ V0,
+ /// Extended schema with the `iceberg_type` column for view support.
+ V1,
+}
+
+impl SchemaVersion {
+ /// Detect the schema version of an existing catalog table by
introspecting its columns.
+ async fn detect(pool: &AnyPool) -> Result<Self> {
+ let catalog_table_description = pool
+ .describe(&format!("SELECT * FROM {CATALOG_TABLE_NAME}"))
+ .await
+ .map_err(from_sqlx_error)?;
+
+ let has_type_column = catalog_table_description
+ .columns()
+ .iter()
+ .any(|column| column.name() == CATALOG_FIELD_RECORD_TYPE);
+
+ Ok(if has_type_column {
+ SchemaVersion::V1
+ } else {
+ SchemaVersion::V0
+ })
+ }
+
+ /// The trailing SQL `AND` clause used to exclude view rows when querying
for tables.
+ ///
+ /// `V1` schemas carry an `iceberg_type` column, so table rows are those
tagged `TABLE`
+ /// (or `NULL`, for rows written before the column existed). `V0` schemas
have no such
+ /// column, so no filter is applied.
+ fn record_type_filter(self) -> &'static str {
+ match self {
+ SchemaVersion::V1 => "AND (iceberg_type = 'TABLE' OR iceberg_type
IS NULL)",
+ SchemaVersion::V0 => "",
+ }
Review Comment:
`record_type_filter()` hard-codes both the `iceberg_type` column name and
the `'TABLE'` record value even though the module already defines
`CATALOG_FIELD_RECORD_TYPE` and `CATALOG_FIELD_TABLE_RECORD_TYPE`. This makes
the filter easy to drift from the rest of the SQL if those constants ever
change.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -343,6 +441,52 @@ impl SqlCatalog {
.await
.map_err(from_sqlx_error)?;
+ let detected_schema_version = SchemaVersion::detect(&pool).await?;
+ let desired_schema_version = config.schema_version;
+
+ // Detect schema by describing columns. If desired schema is
configured then automigrate, otherwise gracefully support older schemas.
+ let schema_version = match (detected_schema_version,
desired_schema_version) {
+ (SchemaVersion::V1, Some(SchemaVersion::V1) | None) => {
+ tracing::debug!(
+ "detected {CATALOG_TABLE_NAME} schema {} which already
supports views",
+ detected_schema_version,
+ );
+ SchemaVersion::V1
+ }
+ (SchemaVersion::V0, Some(desired_schema_version @
SchemaVersion::V1)) => {
+ tracing::warn!(
+ "table {CATALOG_TABLE_NAME} has inferred schema {} but
desired schema {}, performing migration",
+ detected_schema_version,
+ desired_schema_version,
+ );
+ if let Some(migration_sql) = SchemaVersion::V1.migration_sql()
{
+ sqlx::query(&migration_sql)
+ .execute(&pool)
+ .await
+ .map_err(from_sqlx_error)?;
+ }
Review Comment:
The V0→V1 migration is not idempotent under concurrent catalog
initialization: two processes can both detect V0 and race to `ALTER TABLE ...
ADD COLUMN`, causing one to fail with a duplicate-column error even though the
schema is effectively migrated. Consider treating a failed ALTER as success if
a re-detect shows the column now exists, and only surfacing the error if the
schema is still V0.
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -343,6 +441,52 @@ impl SqlCatalog {
.await
.map_err(from_sqlx_error)?;
+ let detected_schema_version = SchemaVersion::detect(&pool).await?;
+ let desired_schema_version = config.schema_version;
+
+ // Detect schema by describing columns. If desired schema is
configured then automigrate, otherwise gracefully support older schemas.
+ let schema_version = match (detected_schema_version,
desired_schema_version) {
+ (SchemaVersion::V1, Some(SchemaVersion::V1) | None) => {
+ tracing::debug!(
+ "detected {CATALOG_TABLE_NAME} schema {} which already
supports views",
+ detected_schema_version,
+ );
+ SchemaVersion::V1
+ }
+ (SchemaVersion::V0, Some(desired_schema_version @
SchemaVersion::V1)) => {
+ tracing::warn!(
+ "table {CATALOG_TABLE_NAME} has inferred schema {} but
desired schema {}, performing migration",
+ detected_schema_version,
+ desired_schema_version,
+ );
+ if let Some(migration_sql) = SchemaVersion::V1.migration_sql()
{
+ sqlx::query(&migration_sql)
+ .execute(&pool)
+ .await
+ .map_err(from_sqlx_error)?;
+ }
+ SchemaVersion::V1
+ }
+ (SchemaVersion::V0, Some(SchemaVersion::V0) | None) => {
+ tracing::warn!(
+ "table {CATALOG_TABLE_NAME} has inferred schema {}; SQL
catalog is initialized without view support, table creation, and table
registration. \
+ To auto-migrate the database schema, set {}=V1",
+ detected_schema_version,
+ SQL_CATALOG_PROP_SCHEMA_VERSION,
+ );
+ SchemaVersion::V0
+ }
+ (SchemaVersion::V1, Some(desired_schema_version @
SchemaVersion::V0)) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "table {CATALOG_TABLE_NAME} has inferred schema {} but
desired schema {}, downgrade migration is not supported",
+ detected_schema_version, desired_schema_version,
+ ),
+ ));
+ }
Review Comment:
The `sql.schema-version` doc comment says migration is attempted only when
the configured version is newer than the detected version. However, when the DB
is already V1 and the user configures `sql.schema-version=V0`, initialization
currently returns `FeatureUnsupported` (a “downgrade”), which contradicts that
contract and prevents using `V0` as a “do not migrate” setting against
already-migrated databases (and can also break first-time initialization).
Consider treating this case as a no-op (keep detected V1) and optionally log a
warning/debug message.
--
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]
