Copilot commented on code in PR #12997:
URL: https://github.com/apache/gravitino/pull/12997#discussion_r3957706526
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -824,16 +827,23 @@ private software.amazon.awssdk.services.glue.model.Column
toGlueColumn(Column co
.build();
}
+ /**
+ * Resolves the storage location of a table, in order of precedence: the
explicit {@code location}
+ * property, the {@code LocationUri} the Glue database declares, and finally
the catalog warehouse
+ * path. A database location already identifies the database, so the table
name is appended
+ * directly to it, whereas the warehouse path is shared by all databases and
needs the database
+ * name in between.
+ */
private String resolveTableLocation(String explicitLocation, String dbName,
String tableName) {
if (explicitLocation != null) {
return explicitLocation;
}
+ String databaseLocation = databaseLocationUri(dbName);
+ if (StringUtils.isNotBlank(databaseLocation)) {
+ return trimTrailingSlash(databaseLocation) + "/" + tableName;
+ }
Review Comment:
With this change, creating a table without an explicit `location` will
always issue a Glue `GetDatabase` call to check `LocationUri`, even when many
tables are created in the same database. Consider caching the resolved
`LocationUri` per `dbName` (including a cached \"no location\" result) within
`GlueCatalogOperations` to reduce API calls/latency and avoid throttling risk
during batch table creation.
##########
docs/aws-glue-catalog.md:
##########
@@ -148,7 +148,7 @@ The following table lists predefined properties for Glue
tables. Additional key-
| Property Name | Description
| Default
Value | Required | Reserved |
Immutable |
|---------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------------------|----------|----------|-----------|
-| `location` | The location for table storage, such as
`s3://bucket/prefix/test_table`. Derived from `warehouse/database/table` when
not specified.
| (derived from warehouse) | No
| No | No |
+| `location` | The location for table storage, such as
`s3://bucket/prefix/test_table`. When not specified, it is derived from the
`LocationUri` of the Glue database as `database-location/table`, falling back
to `warehouse/database/table` when the database declares no location.
| (derived from
warehouse) | No | No | No
|
Review Comment:
The docs now describe that `location` may be derived from the Glue database
`LocationUri`, but the Default Value column still says \"(derived from
warehouse)\", which is inaccurate. Also, the Iceberg section wording can be
read as implying `warehouse` is only conditionally required, while the
properties table still marks it as required—please align these statements to
avoid confusing users (e.g., update the default value text to mention DB
location and clarify whether `warehouse` must always be set vs. only used as
fallback).
##########
catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java:
##########
@@ -66,8 +66,9 @@ public final class GlueConstants {
/**
* Base storage path used as a warehouse when no explicit {@code location}
is given at table
- * creation time. The table location is derived as {@code
warehouse/database/table}. Example:
- * {@code s3://my-bucket/gravitino-warehouse}.
+ * creation time and the Glue database declares no {@code LocationUri}. The
table location is then
+ * derived as {@code warehouse/database/table}. Example: {@code
+ * s3://my-bucket/gravitino-warehouse}.
Review Comment:
The inline Javadoc `{@code ...}` tag is split across lines (`Example:
{@code` on one line and the value on the next). This can render oddly in
generated Javadocs; consider keeping the `{@code s3://...}` example on a single
line for cleaner output.
##########
docs/aws-glue-catalog.md:
##########
@@ -214,7 +214,7 @@ The Glue catalog supports creating and managing
Iceberg-format tables through th
Set `table-format=ICEBERG` in the table properties, or configure
`default-table-format=iceberg` on the catalog to make all tables Iceberg by
default.
-The `warehouse` catalog property must be configured. The table location is
derived as `warehouse/database/table` when no explicit `location` is specified.
+When no explicit `location` is specified, the table location is derived from
the `LocationUri` of the Glue database as `database-location/table`. The
`warehouse` catalog property must be configured for databases that declare no
location, in which case the table location is derived as
`warehouse/database/table`.
Review Comment:
The docs now describe that `location` may be derived from the Glue database
`LocationUri`, but the Default Value column still says \"(derived from
warehouse)\", which is inaccurate. Also, the Iceberg section wording can be
read as implying `warehouse` is only conditionally required, while the
properties table still marks it as required—please align these statements to
avoid confusing users (e.g., update the default value text to mention DB
location and clarify whether `warehouse` must always be set vs. only used as
fallback).
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -843,6 +853,21 @@ private String resolveTableLocation(String
explicitLocation, String dbName, Stri
+ "' on the catalog.");
}
+ /** Returns the {@code LocationUri} of the given Glue database, or null when
it declares none. */
+ private String databaseLocationUri(String dbName) {
+ GetDatabaseRequest.Builder req = GetDatabaseRequest.builder().name(dbName);
+ applyCatalogId(catalogId, req::catalogId);
+ try {
+ return glueClient.getDatabase(req.build()).database().locationUri();
+ } catch (GlueException e) {
+ throw GlueExceptionConverter.toSchemaException(e, "schema " + dbName);
+ }
+ }
+
+ private static String trimTrailingSlash(String path) {
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
Review Comment:
`trimTrailingSlash` only removes a single trailing `/`. If a configured
`LocationUri` or `warehouseLocation` ends with multiple slashes (e.g.,
`s3://bucket/prefix//`), the resolved table path can still contain a double
slash. Consider stripping all trailing slashes (e.g., using a utility that
removes a run of `/` at the end) to make location derivation robust against
this input.
--
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]
