adutra commented on code in PR #4945:
URL: https://github.com/apache/polaris/pull/4945#discussion_r3506060638
##########
persistence/relational-jdbc/src/main/resources/cockroachdb/schema-v4.sql:
##########
@@ -28,8 +28,8 @@
-- policy_mapping_record, events, idempotency_records, scan_metrics_report,
commit_metrics_report
-- * Compatible with PostgreSQL wire protocol
-CREATE SCHEMA IF NOT EXISTS POLARIS_SCHEMA;
-SET search_path TO POLARIS_SCHEMA;
+CREATE SCHEMA IF NOT EXISTS ${schema};
Review Comment:
Using placeholders looks like a code smell. If the schema name is dynamic,
I'd suggest that the scripts should not include a `CREATE SCHEMA` statement at
all.
Here is a better workflow imho:
1. Java creates the schema programmatically before running the script - a
single `CREATE SCHEMA IF NOT EXISTS <name>` statement executed via JDBC (the
schema name is already validated as a safe identifier).
2. Java sets the session search path on the connection before executing the
script `SET search_path TO <name>` for Postgres/CockroachDB, `SET SCHEMA
<name> for H2`. This would required customization per-database type, but seems
doable.
3. The SQL scripts use only unqualified table names, no schema references at
all. They become truly database-agnostic with respect to schema placement.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -422,8 +438,7 @@ public static PreparedQuery generateOverlapQuery(
return new PreparedQuery(query.sql(), where.parameters());
}
- static String getFullyQualifiedTableName(String tableName) {
- // TODO: make schema name configurable.
- return "POLARIS_SCHEMA." + tableName;
+ String getFullyQualifiedTableName(String tableName) {
+ return schemaName + "." + tableName;
Review Comment:
I think that quoting and casing will become a potential issue here. If a
user inputs the schema name as `MySchema`, without quoting, Postgres will
treat it as `myschema` – this may not be what users intended. I wonder if we
should quote the schema name here so that it becomes case-sensitive, and change
the default schema name to `polaris_schema` to match the current behavior.
But then we need to be careful when introducing support for MySQL in #3960:
MySQL has a whole different way of handling case sensitivity and quoting.
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/JdbcBasePersistenceImpl.java:
##########
@@ -1365,7 +1367,7 @@ private void writeScanMetricsReport(@NonNull
ModelScanMetricsReport report) {
DatasourceOperations metricsOps = getMetricsDatasource();
try {
PreparedQuery pq =
- QueryGenerator.generateInsertQuery(
+ queryGenerator.generateInsertQuery(
Review Comment:
Here we have an inconsistency: `queryGenerator` is being applied to a
(potentially) distinct `DatasourceOperations` instance (`metricsOps`).
If we ever allow different datasources for metrics, then we may also wonder
if these datasources would all have the same schema name, or different ones.
Just thinking out loud.
##########
helm/polaris/values.yaml:
##########
@@ -922,6 +922,11 @@ persistence:
type: in-memory # relational-jdbc, nosql
# The configuration for the relational-jdbc persistence manager.
relationalJdbc:
+ # -- The database schema (namespace) holding the Polaris tables. When
empty, Polaris uses the
+ # default schema `POLARIS_SCHEMA`. Must be a valid SQL identifier: it must
start with a letter or
+ # underscore and contain only letters, digits, and underscores.
+ # @section -- Persistence
+ schemaName: ""
Review Comment:
Why not use the default?
```suggestion
schemaName: "POLARIS_SCHEMA"
```
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/QueryGenerator.java:
##########
@@ -41,6 +41,21 @@
*/
public class QueryGenerator {
+ /** Default schema (namespace) used for Polaris tables when none is
configured. */
+ public static final String DEFAULT_SCHEMA_NAME = "POLARIS_SCHEMA";
+
+ /** The database schema (namespace) that qualifies every generated table
reference. */
+ private final String schemaName;
Review Comment:
I am wondering if this is the right approach.
Expanding on my idea stated previously that SQL scripts should be agnostic
of the schema, I think the same is valid for JDBC connections in general. If
`DatasourceOperations` sets the session search path on every connection it
acquires, then:
- `QueryGenerator` goes back to being a pure static utility
- All table references in generated SQL are unqualified (e.g. `ENTITIES`,
not `POLARIS_SCHEMA.ENTITIES`)
- We avoid problems with quoting and casing of schema names in this class
(the problem is transferred to `DatabaseOperations` where we can apply
different logics per database type)
- The bulk of this PR's diff simply disappears
- The `${schema}` substitution in SQL scripts disappears too.
WDYT?
##########
persistence/relational-jdbc/src/main/java/org/apache/polaris/persistence/relational/jdbc/DatasourceOperations.java:
##########
@@ -94,10 +104,37 @@ public DatasourceOperations(
}
}
+ private static String resolveSchemaName(RelationalJdbcConfiguration
configuration) {
Review Comment:
I think you can replace this method with an annotation on the config class:
```java
@Pattern(
regexp = "[A-Za-z_][A-Za-z0-9_]*",
message = "must start with a letter or underscore and contain only
letters, digits, and underscores")
Optional<String> schemaName();
```
`Pattern` would be `jakarta.validation.constraints.Pattern`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
