dimas-b commented on code in PR #3352: URL: https://github.com/apache/polaris/pull/3352#discussion_r2885316714
########## runtime/test-common/src/main/java/org/apache/polaris/test/commons/CockroachRelationalJdbcLifeCycleManagement.java: ########## @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.test.commons; + +import io.quarkus.test.common.DevServicesContext; +import io.quarkus.test.common.QuarkusTestResourceLifecycleManager; +import java.util.Map; +import org.testcontainers.cockroachdb.CockroachContainer; + +public class CockroachRelationalJdbcLifeCycleManagement + implements QuarkusTestResourceLifecycleManager, DevServicesContext.ContextAware { + public static final String INIT_SCRIPT = "init-script"; + + private CockroachContainer cockroach; + private String initScript; + private DevServicesContext context; + + @Override + public void init(Map<String, String> initArgs) { + initScript = initArgs.get(INIT_SCRIPT); + } + + @Override + public Map<String, String> start() { + // CockroachDB testcontainers uses PostgreSQL wire protocol compatibility + cockroach = new CockroachContainer("cockroachdb/cockroach:v24.3.0"); Review Comment: ping :) ########## persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java: ########## @@ -36,28 +45,128 @@ public String getDisplayName() { return displayName; } + /** + * Returns the latest schema version available for this database type. This is used as the default + * schema version for new installations. + */ + public int getLatestSchemaVersion() { + return switch (this) { + case POSTGRES -> 4; // PostgreSQL has schemas v1, v2, v3, v4 + case COCKROACHDB -> 4; // CockroachDB schema version kept in sync with PostgreSQL + case H2 -> 4; // H2 uses same schemas as PostgreSQL + }; + } + public static DatabaseType fromDisplayName(String displayName) { return switch (displayName.toLowerCase(Locale.ROOT)) { case "h2" -> DatabaseType.H2; case "postgresql" -> DatabaseType.POSTGRES; + case "cockroachdb" -> DatabaseType.COCKROACHDB; default -> throw new IllegalStateException("Unsupported DatabaseType: '" + displayName + "'"); }; } + /** + * Determines the database type from JDBC connection metadata and validates against configured + * type. + * + * <p>Logic: + * + * <ul> + * <li>If configuredType is provided: uses it as the authoritative type and validates it matches + * what the connection reports. Throws an exception if there's a mismatch. + * <li>If configuredType is null: infers the type from connection metadata. + * </ul> + * + * @param connection JDBC connection to inspect + * @param configuredType Explicitly configured database type (may be null) + * @return The database type (configured if provided, otherwise inferred) + * @throws IllegalStateException if configured type doesn't match connection metadata + */ + public static DatabaseType inferFromConnection( + java.sql.Connection connection, DatabaseType configuredType) { + try { + var metaData = connection.getMetaData(); + String productName = metaData.getDatabaseProductName().toLowerCase(Locale.ROOT); + + // Infer database type from connection metadata + DatabaseType inferredType = null; + + if (productName.contains("cockroach")) { + inferredType = DatabaseType.COCKROACHDB; + } else if (productName.contains("postgresql")) { + inferredType = DatabaseType.POSTGRES; + } else if (productName.contains("h2")) { + inferredType = DatabaseType.H2; + } + + // If a type was explicitly configured, use it and validate + if (configuredType != null) { + if (inferredType != null && inferredType != configuredType) { + // Special case: CockroachDB uses PostgreSQL JDBC driver and wire protocol + // So configured=COCKROACHDB with inferred=POSTGRES is valid + boolean isValidCockroachConfig = + configuredType == DatabaseType.COCKROACHDB && inferredType == DatabaseType.POSTGRES; + + if (!isValidCockroachConfig) { + throw new IllegalStateException( + String.format( + "Configured database type '%s' does not match detected type '%s' from connection (product: '%s'). " + + "Please check your polaris.persistence.relational.jdbc.database-type configuration.", + configuredType, inferredType, productName)); + } + } + // Use configured type (either it matches, it's valid CockroachDB, or we couldn't infer) + return configuredType; + } + + // No configured type - use inferred type + if (inferredType != null) { + return inferredType; + } + + // Couldn't infer and no configured type + throw new IllegalStateException( + "Cannot infer database type from product name: '" + + productName + + "'. " + + "Please set polaris.persistence.relational.jdbc.database-type explicitly."); + + } catch (java.sql.SQLException e) { Review Comment: nit: import? ########## runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java: ########## @@ -1044,60 +1045,82 @@ public void commitTransaction(CommitTransactionRequest commitTransactionRequest) new TransactionWorkspaceMetaStoreManager(diagnostics(), metaStoreManager()); ((IcebergCatalog) baseCatalog).setMetaStoreManager(transactionMetaStoreManager); + // Group all changes by table identifier to handle them atomically. + // This prevents conflicts when multiple changes target the same table entity. + // LinkedHashMap preserves insertion order for deterministic processing. + Map<TableIdentifier, List<UpdateTableRequest>> changesByTable = new LinkedHashMap<>(); Review Comment: hmmm.... is this related to CockroachDB ? IIRC, we have a separate PR for the multi-table commit 🤔 -- 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]
