cobed95 commented on code in PR #4945:
URL: https://github.com/apache/polaris/pull/4945#discussion_r3549679837
##########
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:
Implemented in
[1ec77a3fc](https://github.com/apache/polaris/pull/4945/commits/1ec77a3fc):
`QueryGenerator` is a pure static utility again, all generated SQL uses
unqualified table names, and `DatasourceOperations` selects the session schema
on every connection it borrows (`SET search_path TO <name>` for
PostgreSQL/CockroachDB, `SET SCHEMA <name>` for H2). As you predicted, the bulk
of the previous diff disappeared.
One deliberate choice worth flagging: the `SET` runs on every borrow rather
than once per physical connection. It's a single cheap statement, and it keeps
correctness independent of pool behavior; if it ever shows up in profiles it
could move to the pool's new-connection SQL, but I didn't want correctness to
depend on deployment-level Agroal configuration. Happy to iterate on this.
##########
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:
Following up now that the code is pushed: with
[1ec77a3fc](https://github.com/apache/polaris/pull/4945/commits/1ec77a3fc) this
is the case — the `queryGenerator` field is gone from
`JdbcBasePersistenceImpl`, so nothing ties generated queries to a particular
`DatasourceOperations` instance anymore.
##########
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've gone ahead and implemented the container-element form described above
in [1ed89f430](https://github.com/apache/polaris/pull/4945/commits/1ed89f430),
keeping the programmatic check alongside it for now per the reasoning in my
previous comment. If you'd rather go annotation-only (with the validator added
to the admin tool), it's an easy switch — just let me know.
--
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]
