You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "sclee01 (via GitHub)" <gi...@apache.org> on 2025/07/28 10:39:33 UTC
[PR] add the schema-setup for admin tool [polaris]
sclee01 opened a new pull request, #2196:
URL: https://github.com/apache/polaris/pull/2196
### Motivation
This change is related to improving the Polaris Admin Tool.
Currently, the `bootstrap` command in the Admin Tool handles both schema initialization and realm bootstrapping. However, in JDBC persistence, multiple realms can share a single schema. Therefore, schema setup should be treated as a higher-level, one-time operation.
This PR addresses that by introducing a dedicated command.
### Description
- Introduced `SetupCommand` in the Admin Tool for running one-time persistence setup tasks (e.g., schema creation)
- Added a new interface: `SchemaInitializer`, with methods:
- `setupSchema(SchemaOptions)`
- `isSchemaPresent(String schemaName)`
- Refactored existing logic to delegate schema setup outside of `MetaStoreManagerFactory`
- Maintains backward compatibility (bootstrap still performs schema init if needed)
### Context
This change was proposed in the following issue:
- [Support database bootstrap in the admin tool #2010](https://github.com/apache/polaris/issues/2010
)
Based on discussion with @dimas-b.
### Next Steps
- I’m planning to mention this on the dev mailing list for broader visibility, in case there are any concerns or suggestions.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2240346226
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -31,6 +32,17 @@ public interface SchemaOptions {
@Nullable
String schemaFile();
+ @Value.Default
+ default boolean setupSchema() {
+ return true;
+ }
+
+ @NotNull
+ @Value.Default
+ default String schemaName() {
+ return "POLARIS_SCHEMA";
Review Comment:
Isn't this kind of asymmetric now? `SchemaOptions` allow callers to set any schema name, but persistence implementations do not use it for actually creating the schema :thinking: ... or did I miss it?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #2196: add the schema-setup for admin tool
URL: https://github.com/apache/polaris/pull/2196
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2240335637
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -43,6 +44,12 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return metaStoreManagerFactories.select(Identifier.Literal.of(persistenceType)).get();
}
+ @Produces
+ @ApplicationScoped
+ public SchemaInitializer schemaInitializer(MetaStoreManagerFactory metaStoreManagerFactory) {
+ return (SchemaInitializer) metaStoreManagerFactory;
Review Comment:
... but having those runtime errors is good, IMHO :) CDI errors should only happen if the tool setup calls for a `SchemaInitializer` but the configuration does not allow for an implementation to be found. All in all, this would lead to a clear error, whereas, a type case would lead to a very obscure error.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2238599411
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BaseCommand.java:
##########
@@ -29,8 +30,11 @@ public abstract class BaseCommand implements Callable<Integer> {
public static final int EXIT_CODE_USAGE = 2;
public static final int EXIT_CODE_BOOTSTRAP_ERROR = 3;
public static final int EXIT_CODE_PURGE_ERROR = 4;
+ public static final int EXIT_CODE_SCHEMA_ERROR = 5;
@Inject MetaStoreManagerFactory metaStoreManagerFactory;
+ @Inject SchemaInitializer schemaInitializer;
Review Comment:
You're right. I changed it to take SchemaOptions instead for better encapsulation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3139388635
@sclee01 thank you for your commitment!
To clarify what I have in mind: I think a configuration interface like below would be the right way to proceed:
```java
@ConfigMapping(prefix = "polaris.schema")
public interface SchemaSetupConfiguration {
/**
* Whether to set up the schema at startup.
*
* <p>This is useful for development and testing purposes, but should be disabled in production
* environments where the schema is managed externally.
*
* @return true if the schema should be set up at startup, false otherwise.
*/
@WithName("setup-at-startup")
@WithDefault("true")
boolean setupSchemaAtStartup();
}
```
If you really want to give users to choice when to do the setup, then something like below could work:
```java
@ConfigMapping(prefix = "polaris.schema")
public interface SchemaSetupConfiguration {
enum SchemaSetupMode {
AT_STARTUP,
PER_REALM,
NEVER
}
/**
* The mode of schema setup to use.
*
* @return the schema setup mode
*/
@WithDefault("AT_STARTUP")
SchemaSetupMode mode();
}
```
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3144136407
@sclee01 I went ahead and started the thread: https://lists.apache.org/thread/6c7lzgy3675jrrtbrrxsm8l0xd1fo1mj
Please chime in! 🙏
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2238599411
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BaseCommand.java:
##########
@@ -29,8 +30,11 @@ public abstract class BaseCommand implements Callable<Integer> {
public static final int EXIT_CODE_USAGE = 2;
public static final int EXIT_CODE_BOOTSTRAP_ERROR = 3;
public static final int EXIT_CODE_PURGE_ERROR = 4;
+ public static final int EXIT_CODE_SCHEMA_ERROR = 5;
@Inject MetaStoreManagerFactory metaStoreManagerFactory;
+ @Inject SchemaInitializer schemaInitializer;
Review Comment:
Moved it to SetupCommand since it's not used elsewhere.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/SchemaInitializer.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
+
+public interface SchemaInitializer {
+
+ boolean setupSchema(SchemaOptions schemaOptions);
+
+ boolean isSchemaPresent(String schemaName);
Review Comment:
I changed it to take SchemaOptions instead for better encapsulation.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2244873846
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -143,15 +146,16 @@ public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
- try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(bootstrapOptions.schemaOptions()));
- } catch (SQLException e) {
- throw new RuntimeException(
- String.format("Error executing sql script: %s", e.getMessage()), e);
+ if (bootstrapOptions.schemaOptions().setupSchema()) {
+ setupSchema(bootstrapOptions.schemaOptions());
Review Comment:
If we want to support a schemas-per-realm kind of deployment (which is not the case today), then we'd need another config property to indicate which kind of schema we are using, and when to set it up (at startup vs for each realm).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3139417433
@adutra @dimas-b
I’d love to get your thoughts on this.
I initially added this to support explicit one-time schema setup from the Admin Tool side. But given that the scripts are idempotent, I’m now wondering if separating this logic is really necessary — maybe it’s fine to run the setup every time a realm is created.
That said, if the separation is implemented based on the reviews, I’m also thinking it could be beneficial in the long run — for example, it gives us flexibility if we ever want to support different schema strategies (e.g. start up, realm .. ) or control setup behavior more explicitly.
Let me know what you think — happy to drop it if it feels unnecessary, but also open to keeping it if it adds value.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2240322927
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -116,6 +116,28 @@ public void executeScript(InputStream scriptInputStream) throws SQLException {
}
}
+ public boolean checkSchemaExists(@Nonnull String schemaName) {
+ final String sql = "SELECT 1 FROM information_schema.schemata WHERE schema_name = ?";
Review Comment:
PostgreSQL is the only currently supported database. I would not want Polaris to be limited to PostgreSQL forever :)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3243562584
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2240335637
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -43,6 +44,12 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return metaStoreManagerFactories.select(Identifier.Literal.of(persistenceType)).get();
}
+ @Produces
+ @ApplicationScoped
+ public SchemaInitializer schemaInitializer(MetaStoreManagerFactory metaStoreManagerFactory) {
+ return (SchemaInitializer) metaStoreManagerFactory;
Review Comment:
... but having those runtime errors is good, IMHO :) CDI errors should only happen if the tool setup calls for a `SchemaInitializer` but the configuration does not allow for an implementation to be found. All in all this would lead to a clear error, whereas, a type case would lead to a very obscure error.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3140478554
The most user-friendly way IMO is to let Polaris perform the schema initialization on bootstrap automatically. @adutra already provided some useful additional configuration options for "special" scenarios.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2244843534
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -116,6 +116,29 @@ public void executeScript(InputStream scriptInputStream) throws SQLException {
}
}
+ public boolean checkSchemaExists(String schemaName) {
+ final boolean[] exists = {false};
+
+ try {
+ runWithinTransaction(
+ connection -> {
+ try (ResultSet schemas = connection.getMetaData().getSchemas()) {
+ while (schemas.next()) {
+ if (schemaName.equalsIgnoreCase(schemas.getString("TABLE_SCHEM"))) {
Review Comment:
```suggestion
if (schemaName.equalsIgnoreCase(schemas.getString("TABLE_SCHEMA"))) {
```
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -143,15 +146,16 @@ public synchronized Map<String, PrincipalSecretsResult> bootstrapRealms(
RealmContext realmContext = () -> realm;
if (!metaStoreManagerMap.containsKey(realm)) {
DatasourceOperations datasourceOperations = getDatasourceOperations();
- try {
- // Run the set-up script to create the tables.
- datasourceOperations.executeScript(
- datasourceOperations
- .getDatabaseType()
- .openInitScriptResource(bootstrapOptions.schemaOptions()));
- } catch (SQLException e) {
- throw new RuntimeException(
- String.format("Error executing sql script: %s", e.getMessage()), e);
+ if (bootstrapOptions.schemaOptions().setupSchema()) {
+ setupSchema(bootstrapOptions.schemaOptions());
Review Comment:
We are doing this setup per realm, but the schema cannot change from one realm to another. I think this should be done in a startup method. This would be beneficial to both the admin tool and the server imho.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/SchemaOptions.java:
##########
@@ -31,6 +31,11 @@ public interface SchemaOptions {
@Nullable
String schemaFile();
+ @Value.Default
+ default boolean setupSchema() {
Review Comment:
I fell like this should go into application.properties instead. We are creating a second layer of configuration that is only accessible to the admin tool (I don't see how the server runtime can change this value).
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -80,6 +80,12 @@ static class FileInputOptions {
}
static class SchemaInputOptions {
+ @CommandLine.Option(
Review Comment:
The schema init scripts _must_ be idempotent, because the server runtime executes them whenever it loads a new realm. So why do we need to specify this here? Couldn't we just assume that schema should be set up always? If we want to avoid re-running init scripts on an already-initialized database, this is a concern that affects both the admin tool and the server. IOW this should be controlled by a configuration option in application.properties.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2238004882
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -116,6 +116,28 @@ public void executeScript(InputStream scriptInputStream) throws SQLException {
}
}
+ public boolean checkSchemaExists(@Nonnull String schemaName) {
+ final String sql = "SELECT 1 FROM information_schema.schemata WHERE schema_name = ?";
Review Comment:
Is `information_schema.schemata` standard? :thinking: Could it be preferable to use JDBC APIs (`Connection.getMetaData()`) for this and let the driver figure out the right source for this data?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/SchemaInitializer.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
+
+public interface SchemaInitializer {
+
+ boolean setupSchema(SchemaOptions schemaOptions);
+
+ boolean isSchemaPresent(String schemaName);
Review Comment:
`schemaName` looks like a concept leak in this interface. `SchemaOptions` does not define a "name"... How can the caller know what schema name to ask for?
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/BaseCommand.java:
##########
@@ -29,8 +30,11 @@ public abstract class BaseCommand implements Callable<Integer> {
public static final int EXIT_CODE_USAGE = 2;
public static final int EXIT_CODE_BOOTSTRAP_ERROR = 3;
public static final int EXIT_CODE_PURGE_ERROR = 4;
+ public static final int EXIT_CODE_SCHEMA_ERROR = 5;
@Inject MetaStoreManagerFactory metaStoreManagerFactory;
+ @Inject SchemaInitializer schemaInitializer;
Review Comment:
Is this common to all commands? I tend to think it's only for `bootstrap` and `setup` :thinking:
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -43,6 +44,12 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return metaStoreManagerFactories.select(Identifier.Literal.of(persistenceType)).get();
}
+ @Produces
+ @ApplicationScoped
+ public SchemaInitializer schemaInitializer(MetaStoreManagerFactory metaStoreManagerFactory) {
+ return (SchemaInitializer) metaStoreManagerFactory;
Review Comment:
Please use `.select()` as in `metaStoreManagerFactory()` for proper type handling.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3127467990
Fix the code style to pass the unit test.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3141609951
I think it's a good idea for the `bootstrap` command to also setup the schema by default (nicer user experience as @snazy commented).
Still, I believe the `bootstrap` command show have the option to _not_ try to create the schema (it may or may not be idempotent), in which case the admin user can explicitly run the `setup` command.
Subsequently, there is already an existing [mechanism](https://github.com/apache/polaris/blob/ca85339deee9645f0682194141ae8445ab3af768/runtime/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java#L238) for Polaris Servers to perform "bootstrap" automatically on startup.
With all three options combined, I think, the "getting started" experience can remain simple, while more advanced scenarios are still possible for users that need them.
That said, I do _not_ think that JDBC Persistence code should run any DDL on the main call path (i.e. outside of "bootstrap" or "setup" use cases).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2240337180
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/SchemaInitializer.java:
##########
@@ -0,0 +1,29 @@
+/*
+ * 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;
+
+import org.apache.polaris.core.persistence.bootstrap.SchemaOptions;
+
+public interface SchemaInitializer {
+
+ boolean setupSchema(SchemaOptions schemaOptions);
+
+ boolean isSchemaPresent(SchemaOptions schemaName);
Review Comment:
nit: `schemaName` -> `schemaOptions`?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2238603373
##########
runtime/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -43,6 +44,12 @@ public MetaStoreManagerFactory metaStoreManagerFactory(
return metaStoreManagerFactories.select(Identifier.Literal.of(persistenceType)).get();
}
+ @Produces
+ @ApplicationScoped
+ public SchemaInitializer schemaInitializer(MetaStoreManagerFactory metaStoreManagerFactory) {
+ return (SchemaInitializer) metaStoreManagerFactory;
Review Comment:
I didn't use `.select()` because only `JdbcMetaStoreManagerFactory` implements that interface.
If I use `.select()`, it could cause errors at runtime when using other `MetaStoreManagerFactory` implementations that don't implement `SchemaInitializer`. Given that, I kept the cast with a type check to avoid breaking compatibility. Open to suggestions if there's a safer way to handle this.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on code in PR #2196:
URL: https://github.com/apache/polaris/pull/2196#discussion_r2238598998
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -116,6 +116,28 @@ public void executeScript(InputStream scriptInputStream) throws SQLException {
}
}
+ public boolean checkSchemaExists(@Nonnull String schemaName) {
+ final String sql = "SELECT 1 FROM information_schema.schemata WHERE schema_name = ?";
Review Comment:
I thought this would stay PostgreSQL-only, so I used the query directly. But updated to use getMetaData() for better portability.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3141998685
I agree with your suggestion — having it configurable makes a lot of sense.
That said, I think it’s worth hearing a few more thoughts before we settle on the direction.
Maybe we can spin up a DISCUSS thread if needed.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "sclee01 (via GitHub)" <gi...@apache.org>.
sclee01 commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3139285899
Thanks for all the reviews. @adutra @dimas-b
While working on the admin tool side, I did feel some friction around where schema setup really belongs. Your point about handling this at the system level, not just in the admin tool, really clarified that.
I'll continue with the refactor to move schema setup into a shared startup handler and make it config-driven so both the server and admin tool can benefit from it.
Appreciate the insight.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] add the schema-setup for admin tool [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #2196:
URL: https://github.com/apache/polaris/pull/2196#issuecomment-3139569494
Agreed let's gather some consensus. I'd also like to hear from @eric-maynard and @singhpk234.
(Maybe this warrants a DISCUSS thread on the ML btw)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org