dimas-b commented on code in PR #1371:
URL: https://github.com/apache/polaris/pull/1371#discussion_r2060764381
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -106,12 +111,21 @@ private void initializeForRealm(
}
private DatasourceOperations getDatasourceOperations(boolean isBootstrap) {
- DatasourceOperations databaseOperations = new
DatasourceOperations(dataSource);
+ // TODO: remove when multiple dataSources are supported.
+ if (dataSources.size() > 1) {
+ throw new IllegalStateException("More than one dataSources configured");
+ }
+ DatasourceOperations databaseOperations = new
DatasourceOperations(dataSources.getFirst());
if (isBootstrap) {
- // TODO: see if we need to take script from Quarkus or can we just
- // use the script committed in the repo.
try {
-
databaseOperations.executeScript("scripts/postgres/schema-v1-postgres.sql");
+ DatabaseType databaseType;
+ try (Connection connection = dataSources.getFirst().getConnection()) {
+ String productName =
+
connection.getMetaData().getDatabaseProductName().toLowerCase(Locale.ROOT);
+ databaseType = DatabaseType.fromDisplayName(productName);
+ }
+ databaseOperations.executeScript(
+ String.format("%s/schema-v1.sql", databaseType.getDisplayName()));
Review Comment:
nit: It'd recommend to make `databaseType.getInitScriptResource()` (or
something like that) and return the full path.
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -106,12 +111,21 @@ private void initializeForRealm(
}
private DatasourceOperations getDatasourceOperations(boolean isBootstrap) {
- DatasourceOperations databaseOperations = new
DatasourceOperations(dataSource);
+ // TODO: remove when multiple dataSources are supported.
+ if (dataSources.size() > 1) {
+ throw new IllegalStateException("More than one dataSources configured");
+ }
+ DatasourceOperations databaseOperations = new
DatasourceOperations(dataSources.getFirst());
if (isBootstrap) {
Review Comment:
Just to recap: I believe it is preferable to run DDL from the dedicated
`bootstrap` command or higher-level class and avoid conditional DDL yes/no
logic on every call to `getDatasourceOperations` (for follow-up).
##########
extension/persistence/relational-jdbc/src/main/java/org/apache/polaris/extension/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java:
##########
@@ -106,12 +111,21 @@ private void initializeForRealm(
}
private DatasourceOperations getDatasourceOperations(boolean isBootstrap) {
- DatasourceOperations databaseOperations = new
DatasourceOperations(dataSource);
+ // TODO: remove when multiple dataSources are supported.
+ if (dataSources.size() > 1) {
+ throw new IllegalStateException("More than one dataSources configured");
+ }
+ DatasourceOperations databaseOperations = new
DatasourceOperations(dataSources.getFirst());
if (isBootstrap) {
- // TODO: see if we need to take script from Quarkus or can we just
- // use the script committed in the repo.
try {
-
databaseOperations.executeScript("scripts/postgres/schema-v1-postgres.sql");
+ DatabaseType databaseType;
+ try (Connection connection = dataSources.getFirst().getConnection()) {
+ String productName =
+
connection.getMetaData().getDatabaseProductName().toLowerCase(Locale.ROOT);
+ databaseType = DatabaseType.fromDisplayName(productName);
Review Comment:
The `toLowerCase` operation should probably be done inside
`fromDisplayName`. Why should the caller care about the comparison method used
in `DatabaseType`?
##########
quarkus/service/build.gradle.kts:
##########
@@ -122,6 +123,18 @@ dependencies {
testImplementation(libs.threeten.extra)
testImplementation(libs.hawkular.agent.prometheus.scraper)
+
+ testImplementation(project(":polaris-quarkus-test-commons"))
+ testFixturesApi(enforcedPlatform(libs.quarkus.bom)) {
+ exclude(group = "org.antlr", module = "antlr4-runtime")
+ exclude(group = "org.scala-lang", module = "scala-library")
Review Comment:
Why do we need to to `exclude` in an `enforcedPlatform` statement?
##########
extension/persistence/relational-jdbc/src/main/resources/postgres/schema-v1.sql:
##########
@@ -15,16 +15,12 @@
-- KIND, either express or implied. See the License for the
-- specific language governing permissions and limitations
-- under the License.
---
--- Note: Database and schema creation is not included in this script. Please
create the database and
--- schema before running this script. for example in psql:
--- CREATE DATABASE polaris_db;
--- \c polaris_db
--- CREATE SCHEMA polaris_schema;
--- set search_path to polaris_schema;
+CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA;
+SET search_path TO POLARIS_SCHEMA;
CREATE TABLE IF NOT EXISTS entities (
+ realm_id TEXT NOT NULL,
Review Comment:
Let's do collation settings in a follow-up PR, but we must not forget it :)
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisRestCatalogViewIntegrationBase.java:
##########
@@ -80,7 +84,7 @@ static void close() throws Exception {
@BeforeEach
public void before(TestInfo testInfo) {
- Assumptions.assumeFalse(shouldSkip());
+ org.junit.jupiter.api.Assumptions.assumeFalse(shouldSkip());
Review Comment:
Why not `org.assertj.core.api.Assumptions`?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/config/QuarkusProducers.java:
##########
@@ -76,4 +81,13 @@ public PolarisConfigurationStore configurationStore() {
// A configuration store is not required when running the admin tool.
return new PolarisConfigurationStore() {};
}
+
+ @Produces
+ public List<DataSource> dataSources(@All List<InstanceHandle<DataSource>>
dataSources) {
Review Comment:
Is this really needed? I'd expect Quarkus to be able to do this
automatically :thinking:
--
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-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
