Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
*/
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 org.apache.polaris.core.persistence.bootstrap.SchemaOptions;

public enum DatabaseType {
POSTGRES("postgres"),
Expand All @@ -43,7 +48,24 @@ public static DatabaseType fromDisplayName(String displayName) {
};
}

public String getInitScriptResource() {
return String.format("%s/schema-v1.sql", this.getDisplayName());
public InputStream getInitScriptResource(@Nonnull SchemaOptions schemaOptions) {
Comment thread
eric-maynard marked this conversation as resolved.
Outdated
if (schemaOptions.schemaFile() != null) {
try {
return new FileInputStream(schemaOptions.schemaFile());
} catch (IOException e) {
throw new IllegalArgumentException("Unable to load file " + schemaOptions.schemaFile(), e);
}
} else {
final String schemaSuffix;
switch (schemaOptions.schemaVersion()) {
case SchemaOptions.LATEST -> schemaSuffix = "schema-v1.sql";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: make schemaVersion() optional and default to latest.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In SchemaOptions it is:

  @Value.Default
  default String schemaVersion() {
    return LATEST;
  }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean default in bootstrap code to whatever version happens to be latest when no explicit version is requested, but avoid having the LATEST constant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively: use actual current schema version in the LATEST constant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait... that does not carry over to NoSQL 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the LATEST constant now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it's nice to have so that users can be explicit about taking the LATEST schema -- the alternative is just that you get LATEST by not setting the version, right?

Besides the ability to be explicit about wanting the LATEST, I think the constant is potentially useful in the contrived scenario where a persistence wants to default to a non-LATEST schema

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough... but in this case I'd prefer to go back to the non-null schemaVersion() with LATEST as default in the Immutable class... Sorry about back-and-forth 🤦

@eric-maynard eric-maynard Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in that case we actually lose this:

the contrived scenario where a persistence wants to default to a non-LATEST schema

Since there is no way for the value to be unset, it's either LATEST or some number. But I don't feel too strongly about this, as I think that scenario is pretty unlikely. I'm happy to either leave the default or remove LATEST; I'll try getting rid of LATEST for now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right 🤦

case "1" -> schemaSuffix = "schema-v1.sql";
default ->
throw new IllegalArgumentException(
"Unknown schema version " + schemaOptions.schemaVersion());
}
ClassLoader classLoader = DatasourceOperations.class.getClassLoader();
return classLoader.getResourceAsStream(this.getDisplayName() + "/" + schemaSuffix);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import jakarta.annotation.Nonnull;
import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.sql.Connection;
import java.sql.PreparedStatement;
Expand Down Expand Up @@ -80,16 +81,13 @@ DatabaseType getDatabaseType() {
* @param scriptFilePath : Path of SQL script.
* @throws SQLException : Exception while executing the script.
*/
public void executeScript(String scriptFilePath) throws SQLException {
ClassLoader classLoader = DatasourceOperations.class.getClassLoader();
public void executeScript(InputStream scriptInputStream) throws SQLException {
runWithinTransaction(
connection -> {
try (Statement statement = connection.createStatement()) {
BufferedReader reader =
new BufferedReader(
new InputStreamReader(
Objects.requireNonNull(classLoader.getResourceAsStream(scriptFilePath)),
UTF_8));
new InputStreamReader(Objects.requireNonNull(scriptInputStream), UTF_8));
Comment thread
eric-maynard marked this conversation as resolved.
Outdated
StringBuilder sqlBuffer = new StringBuilder();
String line;
while ((line = reader.readLine()) != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
import org.apache.polaris.core.persistence.PrincipalSecretsGenerator;
import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions;
import org.apache.polaris.core.persistence.bootstrap.ImmutableBootstrapOptions;
import org.apache.polaris.core.persistence.bootstrap.ImmutableSchemaOptions;
import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet;
import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.cache.InMemoryEntityCache;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
Expand Down Expand Up @@ -93,13 +97,14 @@ protected PolarisMetaStoreManager createNewMetaStoreManager() {
}

private void initializeForRealm(
RealmContext realmContext, RootCredentialsSet rootCredentialsSet, boolean isBootstrap) {
DatasourceOperations databaseOperations = getDatasourceOperations(isBootstrap);
DatasourceOperations datasourceOperations,
RealmContext realmContext,
RootCredentialsSet rootCredentialsSet) {
sessionSupplierMap.put(
realmContext.getRealmIdentifier(),
() ->
new JdbcBasePersistenceImpl(
databaseOperations,
datasourceOperations,
secretsGenerator(realmContext, rootCredentialsSet),
storageIntegrationProvider,
realmContext.getRealmIdentifier()));
Expand All @@ -108,35 +113,52 @@ private void initializeForRealm(
metaStoreManagerMap.put(realmContext.getRealmIdentifier(), metaStoreManager);
}

private DatasourceOperations getDatasourceOperations(boolean isBootstrap) {
public DatasourceOperations getDatasourceOperations() {
DatasourceOperations databaseOperations;
try {
databaseOperations = new DatasourceOperations(dataSource.get(), relationalJdbcConfiguration);
} catch (SQLException sqlException) {
throw new RuntimeException(sqlException);
}
if (isBootstrap) {
try {
// Run the set-up script to create the tables.
databaseOperations.executeScript(
databaseOperations.getDatabaseType().getInitScriptResource());
} catch (SQLException e) {
throw new RuntimeException(
String.format("Error executing sql script: %s", e.getMessage()), e);
}
}
return databaseOperations;
}

@Override
public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(
Iterable<String> realms, RootCredentialsSet rootCredentialsSet) {
SchemaOptions schemaOptions = ImmutableSchemaOptions.builder().build();

BootstrapOptions bootstrapOptions =
ImmutableBootstrapOptions.builder()
.realms(realms)
.rootCredentialsSet(rootCredentialsSet)
.schemaOptions(schemaOptions)
.build();

return bootstrapRealms(bootstrapOptions);
}

@Override
public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(
BootstrapOptions bootstrapOptions) {
Map<String, PrincipalSecretsResult> results = new HashMap<>();

for (String realm : realms) {
for (String realm : bootstrapOptions.realms()) {
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
initializeForRealm(realmContext, rootCredentialsSet, true);
DatasourceOperations datasourceOperations = getDatasourceOperations();
try {
// Run the set-up script to create the tables.
datasourceOperations.executeScript(
datasourceOperations
.getDatabaseType()
.getInitScriptResource(bootstrapOptions.schemaOptions()));
} catch (SQLException e) {
throw new RuntimeException(
String.format("Error executing sql script: %s", e.getMessage()), e);
}
initializeForRealm(
datasourceOperations, realmContext, bootstrapOptions.rootCredentialsSet());
PrincipalSecretsResult secretsResult =
bootstrapServiceAndCreatePolarisPrincipalForRealm(
realmContext, metaStoreManagerMap.get(realm));
Expand Down Expand Up @@ -172,7 +194,8 @@ public Map<String, BaseResult> purgeRealms(Iterable<String> realms) {
public synchronized PolarisMetaStoreManager getOrCreateMetaStoreManager(
RealmContext realmContext) {
if (!metaStoreManagerMap.containsKey(realmContext.getRealmIdentifier())) {
initializeForRealm(realmContext, null, false);
DatasourceOperations datasourceOperations = getDatasourceOperations();
initializeForRealm(datasourceOperations, realmContext, null);
checkPolarisServiceBootstrappedForRealm(
realmContext, metaStoreManagerMap.get(realmContext.getRealmIdentifier()));
}
Expand All @@ -183,7 +206,8 @@ public synchronized PolarisMetaStoreManager getOrCreateMetaStoreManager(
public synchronized Supplier<BasePersistence> getOrCreateSessionSupplier(
RealmContext realmContext) {
if (!sessionSupplierMap.containsKey(realmContext.getRealmIdentifier())) {
initializeForRealm(realmContext, null, false);
DatasourceOperations datasourceOperations = getDatasourceOperations();
initializeForRealm(datasourceOperations, realmContext, null);
checkPolarisServiceBootstrappedForRealm(
realmContext, metaStoreManagerMap.get(realmContext.getRealmIdentifier()));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.apache.polaris.core.persistence.PrincipalSecretsGenerator.RANDOM_SECRETS;

import java.io.InputStream;
import java.sql.SQLException;
import java.time.ZoneId;
import java.util.Optional;
Expand Down Expand Up @@ -49,8 +50,11 @@ protected PolarisTestMetaStoreManager createPolarisTestMetaStoreManager() {
try {
datasourceOperations =
new DatasourceOperations(createH2DataSource(), new H2JdbcConfiguration());
datasourceOperations.executeScript(
String.format("%s/schema-v1.sql", DatabaseType.H2.getDisplayName()));
ClassLoader classLoader = DatasourceOperations.class.getClassLoader();
InputStream scriptStream =
classLoader.getResourceAsStream(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this stream normally needs to be closed, AFAIK.

@eric-maynard eric-maynard Jun 28, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto the above -- I think we just weren't closing it before, but added an explicit close into executeScript to fix this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a try-with-resources block?

@eric-maynard eric-maynard Jun 29, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the javadoc, I think it's better to have this method take on the responsibility for closing the stream after it's used rather than scatter try-with-resources amongst the callers. The loan pattern would be better, but we're already getting pretty far away from the intent of this change.

String.format("%s/schema-v1.sql", DatabaseType.H2.getDisplayName()));
datasourceOperations.executeScript(scriptStream);
} catch (SQLException e) {
throw new RuntimeException(
String.format(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.Map;
import java.util.function.Supplier;
import org.apache.polaris.core.context.RealmContext;
import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions;
import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet;
import org.apache.polaris.core.persistence.cache.EntityCache;
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
Expand All @@ -41,6 +42,10 @@ public interface MetaStoreManagerFactory {
Map<String, PrincipalSecretsResult> bootstrapRealms(
Iterable<String> realms, RootCredentialsSet rootCredentialsSet);

default Map<String, PrincipalSecretsResult> bootstrapRealms(BootstrapOptions bootstrapOptions) {
return bootstrapRealms(bootstrapOptions.realms(), bootstrapOptions.rootCredentialsSet());
}

/** Purge all metadata for the realms provided */
Map<String, BaseResult> purgeRealms(Iterable<String> realms);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/*
* 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.core.persistence.bootstrap;

import org.apache.polaris.immutables.PolarisImmutable;

@PolarisImmutable
public interface BootstrapOptions {
Iterable<String> realms();

RootCredentialsSet rootCredentialsSet();

SchemaOptions schemaOptions();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* 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.core.persistence.bootstrap;

import jakarta.annotation.Nullable;
import org.apache.polaris.immutables.PolarisImmutable;
import org.immutables.value.Value;

@PolarisImmutable
public interface SchemaOptions {
public static final String LATEST = "LATEST";

@Value.Default
default String schemaVersion() {
return LATEST;
}

@Nullable
String schemaFile();
Comment thread
eric-maynard marked this conversation as resolved.
Outdated
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
import java.nio.file.Path;
import java.util.List;
import java.util.Map;
import org.apache.polaris.core.persistence.bootstrap.BootstrapOptions;
import org.apache.polaris.core.persistence.bootstrap.ImmutableBootstrapOptions;
import org.apache.polaris.core.persistence.bootstrap.ImmutableSchemaOptions;
import org.apache.polaris.core.persistence.bootstrap.RootCredentialsSet;
import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
import org.apache.polaris.core.persistence.dao.entity.PrincipalSecretsResult;
import picocli.CommandLine;

Expand All @@ -42,6 +46,9 @@ static class InputOptions {
@CommandLine.ArgGroup(multiplicity = "1")
FileInputOptions fileOptions;

@CommandLine.ArgGroup(multiplicity = "1")
SchemaInputOptions schemaInputOptions;

static class StandardInputOptions {

@CommandLine.Option(
Expand Down Expand Up @@ -71,6 +78,23 @@ static class FileInputOptions {
description = "A file containing root principal credentials to bootstrap.")
Path file;
}

static class SchemaInputOptions {
@CommandLine.Option(
names = {"-v", "--schema-version"},
paramLabel = "<schema version>",
description = "The version of the schema to load in [1, 2, LATEST].",
defaultValue = SchemaOptions.LATEST)
String schemaVersion;

@CommandLine.Option(
names = {"--schema-file"},
paramLabel = "<schema file>",
description =
"A schema file to bootstrap from. If unset, the bundled files will be used.",
Comment thread
eric-maynard marked this conversation as resolved.
defaultValue = "")
String schemaFile;
}
}

@Override
Expand Down Expand Up @@ -103,9 +127,27 @@ public Integer call() {
}
}

final SchemaOptions schemaOptions;
if (inputOptions.schemaInputOptions != null) {
schemaOptions =
ImmutableSchemaOptions.builder()
.schemaFile(inputOptions.schemaInputOptions.schemaFile)
.schemaVersion(inputOptions.schemaInputOptions.schemaVersion)
.build();
} else {
schemaOptions = ImmutableSchemaOptions.builder().build();
}

BootstrapOptions bootstrapOptions =
ImmutableBootstrapOptions.builder()
.realms(realms)
.rootCredentialsSet(rootCredentialsSet)
.schemaOptions(schemaOptions)
.build();

// Execute the bootstrap
Map<String, PrincipalSecretsResult> results =
metaStoreManagerFactory.bootstrapRealms(realms, rootCredentialsSet);
metaStoreManagerFactory.bootstrapRealms(bootstrapOptions);

// Log any errors:
boolean success = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,9 @@ public void testBootstrapInvalidCredentials(LaunchResult result) {
public void testBootstrapInvalidArguments(LaunchResult result) {
assertThat(result.getErrorOutput())
.contains(
"Error: (-r=<realm> [-r=<realm>]... [-c=<realm,clientId,clientSecret>]... [-p]) "
+ "and -f=<file> are mutually exclusive (specify only one)");
"(-r=<realm> [-r=<realm>]... [-c=<realm,clientId,clientSecret>]... [-p]) and -f=<file> "
+ "and ([-v=<schema version>] | [--schema-file=<schema file>]) are mutually exclusive "
+ "(specify only one)");
}

@Test
Expand Down
Loading