Copilot commented on code in PR #12939:
URL: https://github.com/apache/gravitino/pull/12939#discussion_r3946191961
##########
docs/jdbc-postgresql-catalog.md:
##########
@@ -36,14 +36,14 @@ Check the relevant data source configuration in [data
source properties](https:/
When using Gravitino with Trino, pass the Trino PostgreSQL connector
configuration using the `trino.bypass.` prefix. For example, using
`trino.bypass.join-pushdown.strategy` to pass the `join-pushdown.strategy` to
the Gravitino PostgreSQL catalog in Trino runtime.
-If you use JDBC catalog, you must provide `jdbc-url`, `jdbc-driver`,
`jdbc-database`, `jdbc-user` and `jdbc-password` to catalog properties.
+If you use JDBC catalog, you must provide `jdbc-url`, `jdbc-driver`,
`jdbc-user` and `jdbc-password` to catalog properties.
Besides the [common catalog
properties](./gravitino-server-config.md#catalog-properties-configuration), the
PostgreSQL catalog has the following properties:
| Configuration item | Description
| Default value | Required |
|-------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|----------|
| `jdbc-url` | JDBC URL for connecting to the database. You need
to specify the database in the URL. For example
`jdbc:postgresql://localhost:3306/pg_database?sslmode=require`. | (none)
| Yes |
| `jdbc-driver` | The driver of the JDBC connection. For example
`org.postgresql.Driver`.
| (none) | Yes |
-| `jdbc-database` | The database of the JDBC connection. Configure it
with the same value as the database in the `jdbc-url`. For example
`pg_database`. | (none) | Yes |
+| `jdbc-database` | The database of the JDBC connection. Derived from
`jdbc-url` when omitted. An explicit value must be nonblank and match the URL
database. | (none) | Only if absent from `jdbc-url` |
Review Comment:
The `jdbc-url` row still says users must specify the database in the URL and
the example uses port `3306` (MySQL default). This contradicts the new behavior
that allows supplying the database via `jdbc-database` when the URL omits it,
and can confuse users.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/PostgreSqlCatalog.java:
##########
@@ -32,9 +33,22 @@
import
org.apache.gravitino.catalog.postgresql.operation.PostgreSqlTableOperations;
import org.apache.gravitino.connector.CatalogOperations;
import org.apache.gravitino.connector.capability.Capability;
+import org.postgresql.Driver;
public class PostgreSqlCatalog extends JdbcCatalog {
+ /** {@inheritDoc} */
+ @Override
+ public JdbcCatalog withCatalogConf(Map<String, String> conf) {
+ return super.withCatalogConf(
+ resolveJdbcDatabase(
+ conf,
+ url -> {
+ Properties parsed = Driver.parseURL(url, new Properties());
+ return parsed == null ? null : parsed.getProperty("PGDBNAME");
+ }));
Review Comment:
`Driver.parseURL(...)` returns `null` when the JDBC URL is not a valid
PostgreSQL URL. Returning `null` here causes invalid URLs to be treated as
"database absent" (and can even allow an invalid URL when `jdbc-database` is
explicitly set). It’s better to fail fast with a clear invalid-URL 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
