From 119fbdb9b8ce1d106505735a99f6a5245f6ee1b6 Mon Sep 17 00:00:00 2001 From: Prashant Date: Mon, 6 Oct 2025 09:14:50 -0700 Subject: [PATCH 1/6] JDBC: Fix Bootstrap with schema options --- .../relational/jdbc/DatabaseType.java | 6 ++- .../jdbc/JdbcMetaStoreManagerFactory.java | 35 ++++++++++++++ .../persistence/bootstrap/SchemaOptions.java | 4 +- .../polaris/admintool/BootstrapCommand.java | 48 ++++++++++++------- .../RelationalJdbcBootstrapCommandTest.java | 24 +++++++++- 5 files changed, 95 insertions(+), 22 deletions(-) diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java index c617e505e6..c782b007d8 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java @@ -23,6 +23,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.Locale; +import java.util.Objects; import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; public enum DatabaseType { @@ -53,9 +54,10 @@ public static DatabaseType fromDisplayName(String displayName) { * caller. */ public InputStream openInitScriptResource(@Nonnull SchemaOptions schemaOptions) { - if (schemaOptions.schemaFile() != null) { + if (schemaOptions.schemaFile() != null + && !Objects.requireNonNull(schemaOptions.schemaFile()).isEmpty()) { try { - return new FileInputStream(schemaOptions.schemaFile()); + return new FileInputStream(Objects.requireNonNull(schemaOptions.schemaFile())); } catch (IOException e) { throw new IllegalArgumentException("Unable to load file " + schemaOptions.schemaFile(), e); } diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java index b1609522c6..e2a28cb305 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java @@ -18,6 +18,7 @@ */ package org.apache.polaris.persistence.relational.jdbc; +import com.google.common.base.Preconditions; import io.smallrye.common.annotation.Identifier; import jakarta.annotation.Nullable; import jakarta.enterprise.context.ApplicationScoped; @@ -29,6 +30,8 @@ import java.util.Map; import java.util.Optional; import java.util.function.Supplier; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.sql.DataSource; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; @@ -69,6 +72,8 @@ public class JdbcMetaStoreManagerFactory implements MetaStoreManagerFactory { final Map metaStoreManagerMap = new HashMap<>(); final Map entityCacheMap = new HashMap<>(); final Map> sessionSupplierMap = new HashMap<>(); + // Define a pattern to find 'v' followed by one or more digits (\d+) + final Pattern pattern = Pattern.compile("(v\\d+)"); @Inject Clock clock; @Inject PolarisDiagnostics diagnostics; @@ -154,6 +159,17 @@ public synchronized Map bootstrapRealms( RealmContext realmContext = () -> realm; if (!metaStoreManagerMap.containsKey(realm)) { DatasourceOperations datasourceOperations = getDatasourceOperations(); + int schemaVersion = + JdbcBasePersistenceImpl.loadSchemaVersion( + datasourceOperations, + configurationStore.getConfiguration( + realmContext, BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); + int requestedSchemaVersion = getSchemaVersion(bootstrapOptions); + Preconditions.checkState( + (requestedSchemaVersion == schemaVersion) || (schemaVersion == 0), + "Cannot bootstrap due to schema version mismatch. Current: %s, Requested: %s", + schemaVersion, // "Current" version + requestedSchemaVersion); try { // Run the set-up script to create the tables. datasourceOperations.executeScript( @@ -281,4 +297,23 @@ private void checkPolarisServiceBootstrappedForRealm(RealmContext realmContext) "Realm is not bootstrapped, please run server in bootstrap mode."); } } + + private int getSchemaVersion(BootstrapOptions bootstrapOptions) { + SchemaOptions schemaOptions = bootstrapOptions.schemaOptions(); + if (schemaOptions != null) { + Integer version = schemaOptions.schemaVersion(); + if (version != null) { + return version; + } + String schemaFile = schemaOptions.schemaFile(); + if (schemaFile != null) { + Matcher matcher = pattern.matcher(schemaFile); + if (matcher.find()) { + String versionStr = matcher.group(1); // "v3" + return Integer.parseInt(versionStr.substring(1)); // 3 + } + } + } + return 0; + } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java index f0779cc8e1..6728c380d6 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java @@ -20,6 +20,7 @@ package org.apache.polaris.core.persistence.bootstrap; import jakarta.annotation.Nullable; +import java.util.Objects; import org.apache.polaris.immutables.PolarisImmutable; import org.immutables.value.Value; @@ -33,7 +34,8 @@ public interface SchemaOptions { @Value.Check default void validate() { - if (schemaVersion() != null && schemaFile() != null) { + if (schemaVersion() != null + && (schemaFile() != null && !Objects.requireNonNull(schemaFile()).isEmpty())) { throw new IllegalStateException("Only one of schemaVersion or schemaFile can be set."); } } diff --git a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java index 097c5e6629..63a9d205fd 100644 --- a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java +++ b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java @@ -35,20 +35,28 @@ description = "Bootstraps realms and root principal credentials.") public class BootstrapCommand extends BaseCommand { - @CommandLine.ArgGroup(multiplicity = "1") - InputOptions inputOptions; + @CommandLine.Mixin InputOptions inputOptions; static class InputOptions { - @CommandLine.ArgGroup(multiplicity = "1", exclusive = false) - StandardInputOptions stdinOptions; - + // This ArgGroup enforces the mandatory, exclusive choice. @CommandLine.ArgGroup(multiplicity = "1") - FileInputOptions fileOptions; + ExclusiveOptions exclusiveOptions; - @CommandLine.ArgGroup(multiplicity = "1") - SchemaInputOptions schemaInputOptions; + // This @Mixin provides independent, optional schema flags. + @CommandLine.Mixin SchemaInputOptions schemaInputOptions = new SchemaInputOptions(); + + // This static inner class encapsulates the mutually exclusive choices. + static class ExclusiveOptions { + + @CommandLine.ArgGroup(exclusive = false, heading = "Standard Input Options:%n") + StandardInputOptions stdinOptions; + @CommandLine.ArgGroup(exclusive = false, heading = "File Input Options:%n") + FileInputOptions fileOptions; + } + + // Option container classes static class StandardInputOptions { @CommandLine.Option( @@ -75,6 +83,7 @@ static class FileInputOptions { @CommandLine.Option( names = {"-f", "--credentials-file"}, paramLabel = "", + required = true, description = "A file containing root principal credentials to bootstrap.") Path file; } @@ -102,19 +111,21 @@ public Integer call() { RootCredentialsSet rootCredentialsSet; List realms; // TODO Iterable - if (inputOptions.fileOptions != null) { - rootCredentialsSet = RootCredentialsSet.fromUri(inputOptions.fileOptions.file.toUri()); + if (inputOptions.exclusiveOptions.fileOptions != null) { + rootCredentialsSet = + RootCredentialsSet.fromUri(inputOptions.exclusiveOptions.fileOptions.file.toUri()); realms = rootCredentialsSet.credentials().keySet().stream().toList(); } else { - realms = inputOptions.stdinOptions.realms; + realms = inputOptions.exclusiveOptions.stdinOptions.realms; rootCredentialsSet = - inputOptions.stdinOptions.credentials == null - || inputOptions.stdinOptions.credentials.isEmpty() + inputOptions.exclusiveOptions.stdinOptions.credentials == null + || inputOptions.exclusiveOptions.stdinOptions.credentials.isEmpty() ? RootCredentialsSet.EMPTY - : RootCredentialsSet.fromList(inputOptions.stdinOptions.credentials); - if (inputOptions.stdinOptions.credentials == null - || inputOptions.stdinOptions.credentials.isEmpty()) { - if (!inputOptions.stdinOptions.printCredentials) { + : RootCredentialsSet.fromList( + inputOptions.exclusiveOptions.stdinOptions.credentials); + if (inputOptions.exclusiveOptions.stdinOptions.credentials == null + || inputOptions.exclusiveOptions.stdinOptions.credentials.isEmpty()) { + if (!inputOptions.exclusiveOptions.stdinOptions.printCredentials) { spec.commandLine() .getErr() .println( @@ -153,7 +164,8 @@ public Integer call() { if (result.getValue().isSuccess()) { String realm = result.getKey(); spec.commandLine().getOut().printf("Realm '%s' successfully bootstrapped.%n", realm); - if (inputOptions.stdinOptions != null && inputOptions.stdinOptions.printCredentials) { + if (inputOptions.exclusiveOptions.stdinOptions != null + && inputOptions.exclusiveOptions.stdinOptions.printCredentials) { String msg = String.format( "realm: %1s root principal credentials: %2s:%3s", diff --git a/runtime/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcBootstrapCommandTest.java b/runtime/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcBootstrapCommandTest.java index dcd5eb1408..31f3a9eea0 100644 --- a/runtime/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcBootstrapCommandTest.java +++ b/runtime/admin/src/test/java/org/apache/polaris/admintool/relational/jdbc/RelationalJdbcBootstrapCommandTest.java @@ -18,8 +18,30 @@ */ package org.apache.polaris.admintool.relational.jdbc; +import static org.assertj.core.api.Assertions.assertThat; + import io.quarkus.test.junit.TestProfile; +import io.quarkus.test.junit.main.LaunchResult; +import io.quarkus.test.junit.main.QuarkusMainLauncher; import org.apache.polaris.admintool.BootstrapCommandTestBase; +import org.junit.jupiter.api.Test; @TestProfile(RelationalJdbcAdminProfile.class) -public class RelationalJdbcBootstrapCommandTest extends BootstrapCommandTestBase {} +public class RelationalJdbcBootstrapCommandTest extends BootstrapCommandTestBase { + + @Test + public void testBootstrapFailsWhenAddingRealmWithDifferentSchemaVersion( + QuarkusMainLauncher launcher) { + // First, bootstrap the schema to version 1 + LaunchResult result1 = + launcher.launch("bootstrap", "-v", "1", "-r", "realm1", "-c", "realm1,root,s3cr3t"); + assertThat(result1.exitCode()).isEqualTo(0); + assertThat(result1.getOutput()).contains("Bootstrap completed successfully."); + + // TODO: enable this once we enable postgres container reuse in the same test. + // LaunchResult result2 = launcher.launch("bootstrap", "-v", "2", "-r", "realm2", "-c", + // "realm2,root,s3cr3t"); + // assertThat(result2.exitCode()).isEqualTo(EXIT_CODE_BOOTSTRAP_ERROR); + // assertThat(result2.getOutput()).contains("Cannot bootstrap due to schema version mismatch."); + } +} From 6a5348442db1d01526fc0eeacb5244156e3e60d3 Mon Sep 17 00:00:00 2001 From: Prashant Date: Mon, 6 Oct 2025 09:58:09 -0700 Subject: [PATCH 2/6] fix tests --- .../relational/jdbc/JdbcMetaStoreManagerFactory.java | 6 ++++-- .../java/org/apache/polaris/admintool/BootstrapCommand.java | 1 - .../apache/polaris/admintool/BootstrapCommandTestBase.java | 4 +--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java index e2a28cb305..aa06524393 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java @@ -164,9 +164,11 @@ public synchronized Map bootstrapRealms( datasourceOperations, configurationStore.getConfiguration( realmContext, BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); + // skip validation if no schema is specified. int requestedSchemaVersion = getSchemaVersion(bootstrapOptions); Preconditions.checkState( - (requestedSchemaVersion == schemaVersion) || (schemaVersion == 0), + (requestedSchemaVersion == schemaVersion) + || (schemaVersion == 0 || requestedSchemaVersion == -1), "Cannot bootstrap due to schema version mismatch. Current: %s, Requested: %s", schemaVersion, // "Current" version requestedSchemaVersion); @@ -314,6 +316,6 @@ private int getSchemaVersion(BootstrapOptions bootstrapOptions) { } } } - return 0; + return -1; } } diff --git a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java index 63a9d205fd..88c0ab2c8d 100644 --- a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java +++ b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java @@ -83,7 +83,6 @@ static class FileInputOptions { @CommandLine.Option( names = {"-f", "--credentials-file"}, paramLabel = "", - required = true, description = "A file containing root principal credentials to bootstrap.") Path file; } diff --git a/runtime/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java b/runtime/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java index 70a0b5ed69..9286c53b72 100644 --- a/runtime/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java +++ b/runtime/admin/src/test/java/org/apache/polaris/admintool/BootstrapCommandTestBase.java @@ -91,9 +91,7 @@ public void testBootstrapInvalidCredentials(LaunchResult result) { public void testBootstrapInvalidArguments(LaunchResult result) { assertThat(result.getErrorOutput()) .contains( - "(-r= [-r=]... [-c=]... [-p]) and -f= " - + "and (-v= | [--schema-file=]) are mutually exclusive " - + "(specify only one)"); + "Error: [-r= [-r=]... [-c=]... [-p]] and [[-f=]] are mutually exclusive (specify only one)"); } @Test From 7b9054a9628d2a758220bac4fd5af4c03f5f05c4 Mon Sep 17 00:00:00 2001 From: Prashant Date: Mon, 6 Oct 2025 10:37:17 -0700 Subject: [PATCH 3/6] Add status code handling for H2 --- .../persistence/relational/jdbc/DatasourceOperations.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java index 43cfe702c4..2f387e1517 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java @@ -52,9 +52,13 @@ public class DatasourceOperations { private static final Logger LOGGER = LoggerFactory.getLogger(DatasourceOperations.class); + // PG STATUS CODES private static final String CONSTRAINT_VIOLATION_SQL_CODE = "23505"; private static final String RELATION_DOES_NOT_EXIST = "42P01"; + // H2 STATUS CODES + private static final String H2_RELATION_DOES_NOT_EXIST = "90079"; + // POSTGRES RETRYABLE EXCEPTIONS private static final String SERIALIZATION_FAILURE_SQL_CODE = "40001"; @@ -396,7 +400,8 @@ public boolean isConstraintViolation(SQLException e) { } public boolean isRelationDoesNotExist(SQLException e) { - return RELATION_DOES_NOT_EXIST.equals(e.getSQLState()); + return RELATION_DOES_NOT_EXIST.equals(e.getSQLState()) + || H2_RELATION_DOES_NOT_EXIST.equals(e.getSQLState()); } private Connection borrowConnection() throws SQLException { From 299868a2856aab3300f0a61289b4ff632aedc4a9 Mon Sep 17 00:00:00 2001 From: Prashant Date: Mon, 6 Oct 2025 22:10:07 -0700 Subject: [PATCH 4/6] Address review feedback --- .../relational/jdbc/DatabaseType.java | 35 +--- .../relational/jdbc/DatasourceOperations.java | 5 +- .../jdbc/JdbcBasePersistenceImpl.java | 17 ++ .../relational/jdbc/JdbcBootstrapUtils.java | 104 +++++++++++ .../jdbc/JdbcMetaStoreManagerFactory.java | 42 +---- .../relational/jdbc/QueryGenerator.java | 8 + .../jdbc/JdbcBootstrapUtilsTest.java | 173 ++++++++++++++++++ .../persistence/bootstrap/SchemaOptions.java | 12 +- .../polaris/admintool/BootstrapCommand.java | 43 +++-- 9 files changed, 351 insertions(+), 88 deletions(-) create mode 100644 persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java create mode 100644 persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java index c782b007d8..747f7a0401 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java @@ -18,13 +18,8 @@ */ package org.apache.polaris.persistence.relational.jdbc; -import jakarta.annotation.Nonnull; -import java.io.FileInputStream; -import java.io.IOException; import java.io.InputStream; import java.util.Locale; -import java.util.Objects; -import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; public enum DatabaseType { POSTGRES("postgres"), @@ -53,27 +48,15 @@ public static DatabaseType fromDisplayName(String displayName) { * Open an InputStream that contains data from an init script. This stream should be closed by the * caller. */ - public InputStream openInitScriptResource(@Nonnull SchemaOptions schemaOptions) { - if (schemaOptions.schemaFile() != null - && !Objects.requireNonNull(schemaOptions.schemaFile()).isEmpty()) { - try { - return new FileInputStream(Objects.requireNonNull(schemaOptions.schemaFile())); - } catch (IOException e) { - throw new IllegalArgumentException("Unable to load file " + schemaOptions.schemaFile(), e); - } - } else { - final String schemaSuffix; - switch (schemaOptions.schemaVersion()) { - case null -> schemaSuffix = "schema-v3.sql"; - case 1 -> schemaSuffix = "schema-v1.sql"; - case 2 -> schemaSuffix = "schema-v2.sql"; - case 3 -> schemaSuffix = "schema-v3.sql"; - default -> - throw new IllegalArgumentException( - "Unknown schema version " + schemaOptions.schemaVersion()); - } - ClassLoader classLoader = DatasourceOperations.class.getClassLoader(); - return classLoader.getResourceAsStream(this.getDisplayName() + "/" + schemaSuffix); + public InputStream openInitScriptResource(int schemaVersion) { + final String schemaSuffix; + switch (schemaVersion) { + case 1 -> schemaSuffix = "schema-v1.sql"; + case 2 -> schemaSuffix = "schema-v2.sql"; + case 3 -> schemaSuffix = "schema-v3.sql"; + default -> throw new IllegalArgumentException("Unknown schema version " + schemaVersion); } + ClassLoader classLoader = DatasourceOperations.class.getClassLoader(); + return classLoader.getResourceAsStream(this.getDisplayName() + "/" + schemaSuffix); } } diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java index 2f387e1517..e44de3a94c 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java @@ -400,8 +400,9 @@ public boolean isConstraintViolation(SQLException e) { } public boolean isRelationDoesNotExist(SQLException e) { - return RELATION_DOES_NOT_EXIST.equals(e.getSQLState()) - || H2_RELATION_DOES_NOT_EXIST.equals(e.getSQLState()); + return (RELATION_DOES_NOT_EXIST.equals(e.getSQLState()) + && databaseType == DatabaseType.POSTGRES) + || (H2_RELATION_DOES_NOT_EXIST.equals(e.getSQLState()) && databaseType == DatabaseType.H2); } private Connection borrowConnection() throws SQLException { diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java index 0d90fe2774..b9070fd192 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java @@ -756,6 +756,23 @@ static int loadSchemaVersion( } } + static boolean entityTableExists(DatasourceOperations datasourceOperations) { + PreparedQuery query = QueryGenerator.generateEntityTableExistQuery(); + try { + List entities = + datasourceOperations.executeSelect(query, new ModelEntity()); + if (entities == null || entities.isEmpty()) { + throw new IllegalStateException("Failed to check if Entities table exist"); + } + return true; + } catch (SQLException e) { + if (datasourceOperations.isRelationDoesNotExist(e)) { + return false; + } + throw new IllegalStateException("Failed to check if Entities table exists", e); + } + } + /** {@inheritDoc} */ @Override public diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java new file mode 100644 index 0000000000..21b924ade9 --- /dev/null +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java @@ -0,0 +1,104 @@ +/* + * 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.persistence.relational.jdbc; + +import java.util.Optional; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions; +import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; + +public class JdbcBootstrapUtils { + + // Define a pattern to find 'v' followed by one or more digits (\d+) + private static final Pattern pattern = Pattern.compile("(v\\d+)"); + + private JdbcBootstrapUtils() {} + + /** + * Determines the correct schema version to use for bootstrapping a realm. + * + * @param currentSchemaVersion The current version of the database schema. + * @param requiredSchemaVersion The requested schema version (-1 for auto-detection). + * @param hasAlreadyBootstrappedRealms Flag indicating if any realms already exist. + * @return The calculated bootstrap schema version. + * @throws IllegalStateException if the combination of parameters represents an invalid state. + */ + public static int getRealmBootstrapSchemaVersion( + int currentSchemaVersion, int requiredSchemaVersion, boolean hasAlreadyBootstrappedRealms) { + + // If versions already match, no change is needed. + if (currentSchemaVersion == requiredSchemaVersion) { + return requiredSchemaVersion; + } + + // Handle fresh installations where no schema version is recorded (version 0). + if (currentSchemaVersion == 0) { + if (hasAlreadyBootstrappedRealms) { + // System was bootstrapped with v1 before schema versioning was introduced. + if (requiredSchemaVersion == -1 || requiredSchemaVersion == 1) { + return 1; + } + } else { + // A truly fresh start. Default to v3 for auto-detection, otherwise use the specified + // version. + return requiredSchemaVersion == -1 ? 3 : requiredSchemaVersion; + } + } + + // Handle auto-detection on an existing installation (current version > 0). + if (requiredSchemaVersion == -1) { + // Use the current version if realms already exist; otherwise, use v3 for the new realm. + return hasAlreadyBootstrappedRealms ? currentSchemaVersion : 3; + } + + // Any other combination is an unhandled or invalid migration path. + throw new IllegalStateException( + String.format( + "Cannot determine bootstrap schema version. Current: %d, Required: %d, Bootstrapped: %b", + currentSchemaVersion, requiredSchemaVersion, hasAlreadyBootstrappedRealms)); + } + + /** + * Extracts the requested schema version from the provided BootstrapOptions. + * + * @param bootstrapOptions: The bootstrap options containing schema information from which to + * extract the version. + * @return The requested schema version, or -1 if not specified. + */ + public static int getRequestedSchemaVersion(BootstrapOptions bootstrapOptions) { + SchemaOptions schemaOptions = bootstrapOptions.schemaOptions(); + if (schemaOptions != null) { + Optional version = schemaOptions.schemaVersion(); + if (version.isPresent()) { + return version.get(); + } + Optional schemaFile = schemaOptions.schemaFile(); + if (schemaFile.isPresent()) { + Matcher matcher = pattern.matcher(schemaFile.get()); + if (matcher.find()) { + String versionStr = matcher.group(1); // "v3" + return Integer.parseInt(versionStr.substring(1)); // 3 + } + } + } + return -1; + } +} diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java index aa06524393..0740d73471 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java @@ -18,7 +18,6 @@ */ package org.apache.polaris.persistence.relational.jdbc; -import com.google.common.base.Preconditions; import io.smallrye.common.annotation.Identifier; import jakarta.annotation.Nullable; import jakarta.enterprise.context.ApplicationScoped; @@ -30,8 +29,6 @@ import java.util.Map; import java.util.Optional; import java.util.function.Supplier; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import javax.sql.DataSource; import org.apache.polaris.core.PolarisCallContext; import org.apache.polaris.core.PolarisDiagnostics; @@ -72,8 +69,6 @@ public class JdbcMetaStoreManagerFactory implements MetaStoreManagerFactory { final Map metaStoreManagerMap = new HashMap<>(); final Map entityCacheMap = new HashMap<>(); final Map> sessionSupplierMap = new HashMap<>(); - // Define a pattern to find 'v' followed by one or more digits (\d+) - final Pattern pattern = Pattern.compile("(v\\d+)"); @Inject Clock clock; @Inject PolarisDiagnostics diagnostics; @@ -159,25 +154,23 @@ public synchronized Map bootstrapRealms( RealmContext realmContext = () -> realm; if (!metaStoreManagerMap.containsKey(realm)) { DatasourceOperations datasourceOperations = getDatasourceOperations(); - int schemaVersion = + int currentSchemaVersion = JdbcBasePersistenceImpl.loadSchemaVersion( datasourceOperations, configurationStore.getConfiguration( realmContext, BehaviorChangeConfiguration.SCHEMA_VERSION_FALL_BACK_ON_DNE)); - // skip validation if no schema is specified. - int requestedSchemaVersion = getSchemaVersion(bootstrapOptions); - Preconditions.checkState( - (requestedSchemaVersion == schemaVersion) - || (schemaVersion == 0 || requestedSchemaVersion == -1), - "Cannot bootstrap due to schema version mismatch. Current: %s, Requested: %s", - schemaVersion, // "Current" version - requestedSchemaVersion); + int requestedSchemaVersion = JdbcBootstrapUtils.getRequestedSchemaVersion(bootstrapOptions); + int effectiveSchemaVersion = + JdbcBootstrapUtils.getRealmBootstrapSchemaVersion( + currentSchemaVersion, + requestedSchemaVersion, + JdbcBasePersistenceImpl.entityTableExists(datasourceOperations)); try { // Run the set-up script to create the tables. datasourceOperations.executeScript( datasourceOperations .getDatabaseType() - .openInitScriptResource(bootstrapOptions.schemaOptions())); + .openInitScriptResource(effectiveSchemaVersion)); } catch (SQLException e) { throw new RuntimeException( String.format("Error executing sql script: %s", e.getMessage()), e); @@ -299,23 +292,4 @@ private void checkPolarisServiceBootstrappedForRealm(RealmContext realmContext) "Realm is not bootstrapped, please run server in bootstrap mode."); } } - - private int getSchemaVersion(BootstrapOptions bootstrapOptions) { - SchemaOptions schemaOptions = bootstrapOptions.schemaOptions(); - if (schemaOptions != null) { - Integer version = schemaOptions.schemaVersion(); - if (version != null) { - return version; - } - String schemaFile = schemaOptions.schemaFile(); - if (schemaFile != null) { - Matcher matcher = pattern.matcher(schemaFile); - if (matcher.find()) { - String versionStr = matcher.group(1); // "v3" - return Integer.parseInt(versionStr.substring(1)); // 3 - } - } - } - return -1; - } } diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java index 94f6248700..485956ed85 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java @@ -256,6 +256,14 @@ static PreparedQuery generateVersionQuery() { return new PreparedQuery("SELECT version_value FROM POLARIS_SCHEMA.VERSION", List.of()); } + @VisibleForTesting + static PreparedQuery generateEntityTableExistQuery() { + return new PreparedQuery( + String.format( + "SELECT * FROM %s LIMIT 1", getFullyQualifiedTableName(ModelEntity.TABLE_NAME)), + List.of()); + } + /** * Generate a SELECT query to find any entities that have a given realm & parent and that may * overlap with a given location. The check is performed without consideration for the scheme, so diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java new file mode 100644 index 0000000000..b2d072ff83 --- /dev/null +++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java @@ -0,0 +1,173 @@ +/* + * 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.persistence.relational.jdbc; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.when; + +import java.util.Optional; +import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions; +import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +class JdbcBootstrapUtilsTest { + + @Test + void getVersion_whenVersionsMatch() { + // Arrange + int version = 2; + + // Act & Assert + assertEquals( + version, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(version, version, true)); + assertEquals( + version, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(version, version, false)); + } + + @Test + void getVersion_whenFreshDbAndNoRealms() { + // Arrange + int currentVersion = 0; + boolean hasRealms = false; + + // Act & Assert + assertEquals( + 3, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, -1, hasRealms)); + assertEquals( + 2, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, 2, hasRealms)); + assertEquals( + 3, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, 3, hasRealms)); + } + + @Test + void getVersion_whenFreshDbAndRealmsExist() { + // Arrange + int currentVersion = 0; + boolean hasRealms = true; + + // Act & Assert + assertEquals( + 1, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, -1, hasRealms)); + assertEquals( + 1, JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, 1, hasRealms)); + } + + @ParameterizedTest + @CsvSource({"2, true, 2", "3, true, 3", "2, false, 3", "3, false, 3"}) + void getVersion_whenExistingDbAndAutoDetect( + int currentVersion, boolean hasRealms, int expectedVersion) { + // Act & Assert + assertEquals( + expectedVersion, + JdbcBootstrapUtils.getRealmBootstrapSchemaVersion(currentVersion, -1, hasRealms)); + } + + @Test + void throwException_whenFreshDbWithRealmsAndInvalidRequiredVersion() { + // Arrange + int currentVersion = 0; + boolean hasRealms = true; + int invalidRequiredVersion = 2; + + // Act & Assert + assertThrows( + IllegalStateException.class, + () -> + JdbcBootstrapUtils.getRealmBootstrapSchemaVersion( + currentVersion, invalidRequiredVersion, hasRealms)); + } + + @Test + void throwException_whenExistingDbAndInvalidMigrationPath() { + // Arrange + int currentVersion = 2; + int requiredVersion = 3; + + // Act & Assert + assertThrows( + IllegalStateException.class, + () -> + JdbcBootstrapUtils.getRealmBootstrapSchemaVersion( + currentVersion, requiredVersion, true)); + + assertThrows( + IllegalStateException.class, + () -> + JdbcBootstrapUtils.getRealmBootstrapSchemaVersion( + currentVersion, requiredVersion, false)); + } + + @Nested + @ExtendWith(MockitoExtension.class) + class GetRequestedSchemaVersionTests { + + @Mock private BootstrapOptions mockBootstrapOptions; + @Mock private SchemaOptions mockSchemaOptions; + + @BeforeEach + void setUp() { + when(mockBootstrapOptions.schemaOptions()).thenReturn(mockSchemaOptions); + } + + @ParameterizedTest + @CsvSource({"'path/v3.sql', 3", "'v2.sql', 2", "'v12.sql', 12"}) + void whenVersionIsInFileName_shouldParseAndReturnIt(String fileName, int expectedVersion) { + when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); + when(mockSchemaOptions.schemaFile()).thenReturn(Optional.of(fileName)); + + int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); + assertEquals(expectedVersion, result); + } + + @ParameterizedTest + @ValueSource(strings = {"schema.sql", "version_one.sql", "schema-vx.sql"}) + void whenFileNameHasNoValidVersion_shouldReturnDefault(String invalidFileName) { + when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); + when(mockSchemaOptions.schemaFile()).thenReturn(Optional.of(invalidFileName)); + + int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); + assertEquals(-1, result); + } + + @Test + void whenSchemaOptionsIsNull_shouldReturnDefault() { + when(mockBootstrapOptions.schemaOptions()).thenReturn(null); + int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); + assertEquals(-1, result); + } + + @Test + void whenAllOptionsAreEmpty_shouldReturnDefault() { + when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); + when(mockSchemaOptions.schemaFile()).thenReturn(Optional.empty()); + int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); + assertEquals(-1, result); + } + } +} diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java index 6728c380d6..f8e6f6efb3 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java @@ -19,23 +19,19 @@ package org.apache.polaris.core.persistence.bootstrap; -import jakarta.annotation.Nullable; -import java.util.Objects; +import java.util.Optional; import org.apache.polaris.immutables.PolarisImmutable; import org.immutables.value.Value; @PolarisImmutable public interface SchemaOptions { - @Nullable - Integer schemaVersion(); + Optional schemaVersion(); - @Nullable - String schemaFile(); + Optional schemaFile(); @Value.Check default void validate() { - if (schemaVersion() != null - && (schemaFile() != null && !Objects.requireNonNull(schemaFile()).isEmpty())) { + if (schemaVersion().isPresent() && schemaFile().isPresent()) { throw new IllegalStateException("Only one of schemaVersion or schemaFile can be set."); } } diff --git a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java index 88c0ab2c8d..77c556c77e 100644 --- a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java +++ b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java @@ -41,13 +41,13 @@ static class InputOptions { // This ArgGroup enforces the mandatory, exclusive choice. @CommandLine.ArgGroup(multiplicity = "1") - ExclusiveOptions exclusiveOptions; + RootCredentialsOptions rootCredentialsOptions; // This @Mixin provides independent, optional schema flags. @CommandLine.Mixin SchemaInputOptions schemaInputOptions = new SchemaInputOptions(); // This static inner class encapsulates the mutually exclusive choices. - static class ExclusiveOptions { + static class RootCredentialsOptions { @CommandLine.ArgGroup(exclusive = false, heading = "Standard Input Options:%n") StandardInputOptions stdinOptions; @@ -110,21 +110,22 @@ public Integer call() { RootCredentialsSet rootCredentialsSet; List realms; // TODO Iterable - if (inputOptions.exclusiveOptions.fileOptions != null) { + if (inputOptions.rootCredentialsOptions.fileOptions != null) { rootCredentialsSet = - RootCredentialsSet.fromUri(inputOptions.exclusiveOptions.fileOptions.file.toUri()); + RootCredentialsSet.fromUri( + inputOptions.rootCredentialsOptions.fileOptions.file.toUri()); realms = rootCredentialsSet.credentials().keySet().stream().toList(); } else { - realms = inputOptions.exclusiveOptions.stdinOptions.realms; + realms = inputOptions.rootCredentialsOptions.stdinOptions.realms; rootCredentialsSet = - inputOptions.exclusiveOptions.stdinOptions.credentials == null - || inputOptions.exclusiveOptions.stdinOptions.credentials.isEmpty() + inputOptions.rootCredentialsOptions.stdinOptions.credentials == null + || inputOptions.rootCredentialsOptions.stdinOptions.credentials.isEmpty() ? RootCredentialsSet.EMPTY : RootCredentialsSet.fromList( - inputOptions.exclusiveOptions.stdinOptions.credentials); - if (inputOptions.exclusiveOptions.stdinOptions.credentials == null - || inputOptions.exclusiveOptions.stdinOptions.credentials.isEmpty()) { - if (!inputOptions.exclusiveOptions.stdinOptions.printCredentials) { + inputOptions.rootCredentialsOptions.stdinOptions.credentials); + if (inputOptions.rootCredentialsOptions.stdinOptions.credentials == null + || inputOptions.rootCredentialsOptions.stdinOptions.credentials.isEmpty()) { + if (!inputOptions.rootCredentialsOptions.stdinOptions.printCredentials) { spec.commandLine() .getErr() .println( @@ -137,11 +138,17 @@ public Integer call() { final SchemaOptions schemaOptions; if (inputOptions.schemaInputOptions != null) { - schemaOptions = - ImmutableSchemaOptions.builder() - .schemaFile(inputOptions.schemaInputOptions.schemaFile) - .schemaVersion(inputOptions.schemaInputOptions.schemaVersion) - .build(); + ImmutableSchemaOptions.Builder builder = ImmutableSchemaOptions.builder(); + + if (inputOptions.schemaInputOptions.schemaVersion != null) { + builder.schemaVersion(inputOptions.schemaInputOptions.schemaVersion); + } + + if (!inputOptions.schemaInputOptions.schemaFile.isEmpty()) { + builder.schemaFile(inputOptions.schemaInputOptions.schemaFile); + } + + schemaOptions = builder.build(); } else { schemaOptions = ImmutableSchemaOptions.builder().build(); } @@ -163,8 +170,8 @@ public Integer call() { if (result.getValue().isSuccess()) { String realm = result.getKey(); spec.commandLine().getOut().printf("Realm '%s' successfully bootstrapped.%n", realm); - if (inputOptions.exclusiveOptions.stdinOptions != null - && inputOptions.exclusiveOptions.stdinOptions.printCredentials) { + if (inputOptions.rootCredentialsOptions.stdinOptions != null + && inputOptions.rootCredentialsOptions.stdinOptions.printCredentials) { String msg = String.format( "realm: %1s root principal credentials: %2s:%3s", From 0f9f66a614af1d0fb2d5f78b2d582e1ced76bcb3 Mon Sep 17 00:00:00 2001 From: Prashant Date: Tue, 7 Oct 2025 08:27:17 -0700 Subject: [PATCH 5/6] Fix H2 bug and add more logging --- .../relational/jdbc/JdbcBasePersistenceImpl.java | 7 ++----- .../relational/jdbc/JdbcMetaStoreManagerFactory.java | 4 ++++ .../relational-jdbc/src/main/resources/h2/schema-v3.sql | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java index b9070fd192..8d6201453f 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java @@ -748,10 +748,10 @@ static int loadSchemaVersion( } return schemaVersion.getFirst().getValue(); } catch (SQLException e) { - LOGGER.error("Failed to load schema version due to {}", e.getMessage(), e); if (fallbackOnDoesNotExist && datasourceOperations.isRelationDoesNotExist(e)) { return SchemaVersion.MINIMUM.getValue(); } + LOGGER.error("Failed to load schema version due to {}", e.getMessage(), e); throw new IllegalStateException("Failed to retrieve schema version", e); } } @@ -761,10 +761,7 @@ static boolean entityTableExists(DatasourceOperations datasourceOperations) { try { List entities = datasourceOperations.executeSelect(query, new ModelEntity()); - if (entities == null || entities.isEmpty()) { - throw new IllegalStateException("Failed to check if Entities table exist"); - } - return true; + return entities != null && !entities.isEmpty(); } catch (SQLException e) { if (datasourceOperations.isRelationDoesNotExist(e)) { return false; diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java index 0740d73471..e46cc7277e 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java @@ -165,6 +165,10 @@ public synchronized Map bootstrapRealms( currentSchemaVersion, requestedSchemaVersion, JdbcBasePersistenceImpl.entityTableExists(datasourceOperations)); + LOGGER.info( + "Effective schema version: {} for bootstrapping realm: {}", + effectiveSchemaVersion, + realm); try { // Run the set-up script to create the tables. datasourceOperations.executeScript( diff --git a/persistence/relational-jdbc/src/main/resources/h2/schema-v3.sql b/persistence/relational-jdbc/src/main/resources/h2/schema-v3.sql index 6f2aa87b85..3fb7749a3d 100644 --- a/persistence/relational-jdbc/src/main/resources/h2/schema-v3.sql +++ b/persistence/relational-jdbc/src/main/resources/h2/schema-v3.sql @@ -30,7 +30,7 @@ CREATE TABLE IF NOT EXISTS version ( MERGE INTO version (version_key, version_value) KEY (version_key) - VALUES ('version', 2); + VALUES ('version', 3); -- H2 supports COMMENT, but some modes may ignore it COMMENT ON TABLE version IS 'the version of the JDBC schema in use'; From 0af79166d26908b00367584c8b21944244796176 Mon Sep 17 00:00:00 2001 From: Prashant Date: Tue, 7 Oct 2025 11:57:13 -0700 Subject: [PATCH 6/6] Address review feedback --- .../relational/jdbc/DatabaseType.java | 15 ++++++----- .../relational/jdbc/JdbcBootstrapUtils.java | 13 ---------- .../jdbc/JdbcBootstrapUtilsTest.java | 26 +++---------------- .../persistence/bootstrap/SchemaOptions.java | 10 ------- .../polaris/admintool/BootstrapCommand.java | 12 --------- 5 files changed, 11 insertions(+), 65 deletions(-) diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java index 747f7a0401..1ae1b35a55 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatabaseType.java @@ -49,14 +49,15 @@ public static DatabaseType fromDisplayName(String displayName) { * caller. */ public InputStream openInitScriptResource(int schemaVersion) { - final String schemaSuffix; - switch (schemaVersion) { - case 1 -> schemaSuffix = "schema-v1.sql"; - case 2 -> schemaSuffix = "schema-v2.sql"; - case 3 -> schemaSuffix = "schema-v3.sql"; - default -> throw new IllegalArgumentException("Unknown schema version " + schemaVersion); + // Preconditions check is simpler and more direct than a switch default + if (schemaVersion <= 0 || schemaVersion > 3) { + throw new IllegalArgumentException("Unknown or invalid schema version " + schemaVersion); } + + final String resourceName = + String.format("%s/schema-v%d.sql", this.getDisplayName(), schemaVersion); + ClassLoader classLoader = DatasourceOperations.class.getClassLoader(); - return classLoader.getResourceAsStream(this.getDisplayName() + "/" + schemaSuffix); + return classLoader.getResourceAsStream(resourceName); } } diff --git a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java index 21b924ade9..814417d1b8 100644 --- a/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java +++ b/persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtils.java @@ -20,16 +20,11 @@ package org.apache.polaris.persistence.relational.jdbc; import java.util.Optional; -import java.util.regex.Matcher; -import java.util.regex.Pattern; import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions; import org.apache.polaris.core.persistence.bootstrap.SchemaOptions; public class JdbcBootstrapUtils { - // Define a pattern to find 'v' followed by one or more digits (\d+) - private static final Pattern pattern = Pattern.compile("(v\\d+)"); - private JdbcBootstrapUtils() {} /** @@ -90,14 +85,6 @@ public static int getRequestedSchemaVersion(BootstrapOptions bootstrapOptions) { if (version.isPresent()) { return version.get(); } - Optional schemaFile = schemaOptions.schemaFile(); - if (schemaFile.isPresent()) { - Matcher matcher = pattern.matcher(schemaFile.get()); - if (matcher.find()) { - String versionStr = matcher.group(1); // "v3" - return Integer.parseInt(versionStr.substring(1)); // 3 - } - } } return -1; } diff --git a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java index b2d072ff83..6a9eb95524 100644 --- a/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java +++ b/persistence/relational-jdbc/src/test/java/org/apache/polaris/persistence/relational/jdbc/JdbcBootstrapUtilsTest.java @@ -32,7 +32,6 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -136,38 +135,19 @@ void setUp() { } @ParameterizedTest - @CsvSource({"'path/v3.sql', 3", "'v2.sql', 2", "'v12.sql', 12"}) - void whenVersionIsInFileName_shouldParseAndReturnIt(String fileName, int expectedVersion) { - when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); - when(mockSchemaOptions.schemaFile()).thenReturn(Optional.of(fileName)); + @CsvSource({"3", "2", "12"}) + void whenVersionIsInFileName_shouldParseAndReturnIt(int expectedVersion) { + when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.of(expectedVersion)); int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); assertEquals(expectedVersion, result); } - @ParameterizedTest - @ValueSource(strings = {"schema.sql", "version_one.sql", "schema-vx.sql"}) - void whenFileNameHasNoValidVersion_shouldReturnDefault(String invalidFileName) { - when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); - when(mockSchemaOptions.schemaFile()).thenReturn(Optional.of(invalidFileName)); - - int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); - assertEquals(-1, result); - } - @Test void whenSchemaOptionsIsNull_shouldReturnDefault() { when(mockBootstrapOptions.schemaOptions()).thenReturn(null); int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); assertEquals(-1, result); } - - @Test - void whenAllOptionsAreEmpty_shouldReturnDefault() { - when(mockSchemaOptions.schemaVersion()).thenReturn(Optional.empty()); - when(mockSchemaOptions.schemaFile()).thenReturn(Optional.empty()); - int result = JdbcBootstrapUtils.getRequestedSchemaVersion(mockBootstrapOptions); - assertEquals(-1, result); - } } } diff --git a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java index f8e6f6efb3..5cfc20a889 100644 --- a/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java +++ b/polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java @@ -21,18 +21,8 @@ import java.util.Optional; import org.apache.polaris.immutables.PolarisImmutable; -import org.immutables.value.Value; @PolarisImmutable public interface SchemaOptions { Optional schemaVersion(); - - Optional schemaFile(); - - @Value.Check - default void validate() { - if (schemaVersion().isPresent() && schemaFile().isPresent()) { - throw new IllegalStateException("Only one of schemaVersion or schemaFile can be set."); - } - } } diff --git a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java index 77c556c77e..0bdf291b6f 100644 --- a/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java +++ b/runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java @@ -93,14 +93,6 @@ static class SchemaInputOptions { paramLabel = "", description = "The version of the schema to load in [1, 2, 3, LATEST].") Integer schemaVersion; - - @CommandLine.Option( - names = {"--schema-file"}, - paramLabel = "", - description = - "A schema file to bootstrap from. If unset, the bundled files will be used.", - defaultValue = "") - String schemaFile; } } @@ -144,10 +136,6 @@ public Integer call() { builder.schemaVersion(inputOptions.schemaInputOptions.schemaVersion); } - if (!inputOptions.schemaInputOptions.schemaFile.isEmpty()) { - builder.schemaFile(inputOptions.schemaInputOptions.schemaFile); - } - schemaOptions = builder.build(); } else { schemaOptions = ImmutableSchemaOptions.builder().build();