diff --git a/catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java b/catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java index 76de4483d58..da78dfe24cf 100644 --- a/catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java +++ b/catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java @@ -66,8 +66,10 @@ 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 + * derived as {@code warehouse/database/table}. + * + *

Example warehouse: {@code s3://my-bucket/gravitino-warehouse}. */ public static final String WAREHOUSE = "warehouse"; diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java index e7ce0ea8b06..67a8a807384 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java @@ -122,7 +122,10 @@ public class GlueCatalogOperations implements CatalogOperations, SupportsSchemas @VisibleForTesting String defaultTableFormat; - /** Warehouse storage path. Table location is derived as {@code warehouse/db/table}. */ + /** + * Warehouse storage path. Table location is derived as {@code warehouse/db/table} for databases + * that declare no {@code LocationUri}. + */ @VisibleForTesting String warehouseLocation; /** Iceberg SDK Glue catalog used for creating Iceberg-format tables. */ @@ -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 StringUtils.stripEnd(databaseLocation, "/") + "/" + tableName; + } if (StringUtils.isNotBlank(warehouseLocation)) { - String base = - warehouseLocation.endsWith("/") - ? warehouseLocation.substring(0, warehouseLocation.length() - 1) - : warehouseLocation; - return base + "/" + dbName + "/" + tableName; + return StringUtils.stripEnd(warehouseLocation, "/") + "/" + dbName + "/" + tableName; } throw new IllegalArgumentException( "Table location is required: either set the '" @@ -843,6 +853,17 @@ 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 void applyColumnChange( List dataCols, List partCols, TableChange.ColumnChange change) { diff --git a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java index 47e5bf31c62..7275861ab74 100644 --- a/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java +++ b/catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java @@ -105,8 +105,9 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata stringRequiredPropertyEntry( WAREHOUSE, "Base storage path used as warehouse when no explicit location is set" - + " at table creation time (e.g. s3://my-bucket/warehouse)." - + " Table location is derived as warehouse/database/table.", + + " at table creation time and the database declares no LocationUri" + + " (e.g. s3://my-bucket/warehouse)." + + " Table location is then derived as warehouse/database/table.", false /* immutable */, false /* hidden */)) .build(); diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsForIceberg.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsForIceberg.java index 8970b74a4af..3fe0f11d96c 100644 --- a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsForIceberg.java +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsForIceberg.java @@ -47,6 +47,9 @@ import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import software.amazon.awssdk.services.glue.GlueClient; +import software.amazon.awssdk.services.glue.model.Database; +import software.amazon.awssdk.services.glue.model.GetDatabaseRequest; +import software.amazon.awssdk.services.glue.model.GetDatabaseResponse; import software.amazon.awssdk.services.glue.model.GetPartitionsRequest; import software.amazon.awssdk.services.glue.model.GetPartitionsResponse; import software.amazon.awssdk.services.glue.model.GetTableRequest; @@ -193,6 +196,10 @@ void testCreateTable_icebergMissingLocationThrows() { GlueColumn[] cols = { GlueColumn.builder().withName("id").withType(Types.LongType.get()).withNullable(false).build() }; + // The database declares no location either, so there is nothing to derive the location from. + when(mockClient.getDatabase(any(GetDatabaseRequest.class))) + .thenReturn( + GetDatabaseResponse.builder().database(Database.builder().name(DB).build()).build()); assertThrows( IllegalArgumentException.class, diff --git a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogTableOperations.java b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogTableOperations.java index b8ed3206e8f..d2da93aafc9 100644 --- a/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogTableOperations.java +++ b/catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogTableOperations.java @@ -24,6 +24,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -52,8 +53,11 @@ import software.amazon.awssdk.services.glue.GlueClient; import software.amazon.awssdk.services.glue.model.AlreadyExistsException; import software.amazon.awssdk.services.glue.model.CreateTableRequest; +import software.amazon.awssdk.services.glue.model.Database; import software.amazon.awssdk.services.glue.model.DeleteTableRequest; import software.amazon.awssdk.services.glue.model.EntityNotFoundException; +import software.amazon.awssdk.services.glue.model.GetDatabaseRequest; +import software.amazon.awssdk.services.glue.model.GetDatabaseResponse; import software.amazon.awssdk.services.glue.model.GetTableRequest; import software.amazon.awssdk.services.glue.model.GetTableResponse; import software.amazon.awssdk.services.glue.model.GetTablesRequest; @@ -74,6 +78,14 @@ void setup() { ops = new GlueCatalogOperations(); ops.glueClient = mockClient; ops.warehouseLocation = "s3://test-bucket/warehouse"; + stubDatabaseLocation(null); + } + + /** Stubs the database lookup used to resolve table locations. */ + private void stubDatabaseLocation(String locationUri) { + Database database = Database.builder().name("mydb").locationUri(locationUri).build(); + when(mockClient.getDatabase(any(GetDatabaseRequest.class))) + .thenReturn(GetDatabaseResponse.builder().database(database).build()); } // ------------------------------------------------------------------------- @@ -274,6 +286,163 @@ void testCreateTableStorageDescriptorProperties() { assertFalse(req.tableInput().parameters().containsKey(GlueConstants.LOCATION)); } + @Test + void testCreateTableLocationFromDatabaseLocationUri() { + stubDatabaseLocation("s3://test-bucket/gravprobe"); + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://test-bucket/gravprobe/mytable", + captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableLocationFromDatabaseLocationUriWithTrailingSlashes() { + stubDatabaseLocation("s3://test-bucket/gravprobe///"); + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://test-bucket/gravprobe/mytable", + captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableLocationWhenWarehouseEqualsDatabaseLocation() { + ops.warehouseLocation = "s3://test-bucket/gravprobe"; + stubDatabaseLocation("s3://test-bucket/gravprobe"); + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://test-bucket/gravprobe/mytable", + captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableLocationFallsBackToWarehouse() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://test-bucket/warehouse/mydb/mytable", + captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableLocationFromWarehouseWithTrailingSlashes() { + ops.warehouseLocation = "s3://test-bucket/warehouse///"; + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://test-bucket/warehouse/mydb/mytable", + captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableExplicitLocationWinsOverDatabaseLocationUri() { + stubDatabaseLocation("s3://test-bucket/gravprobe"); + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "mytable"); + + ArgumentCaptor captor = ArgumentCaptor.forClass(CreateTableRequest.class); + + ops.createTable( + ident, + new Column[0], + "comment", + Map.of(GlueConstants.LOCATION, "s3://my-bucket/path"), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES); + + verify(mockClient).createTable(captor.capture()); + assertEquals( + "s3://my-bucket/path", captor.getValue().tableInput().storageDescriptor().location()); + } + + @Test + void testCreateTableSchemaNotFoundWhileResolvingLocation() { + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "missing", "mytable"); + when(mockClient.getDatabase(any(GetDatabaseRequest.class))) + .thenThrow(EntityNotFoundException.builder().message("not found").build()); + + assertThrows( + NoSuchSchemaException.class, + () -> + ops.createTable( + ident, + new Column[0], + "comment", + Collections.emptyMap(), + Transforms.EMPTY_TRANSFORM, + Distributions.NONE, + SortOrders.NONE, + Indexes.EMPTY_INDEXES)); + } + // ------------------------------------------------------------------------- // alterTable // ------------------------------------------------------------------------- @@ -299,6 +468,32 @@ void testAlterTableRenameAndComment() { assertEquals("new comment", result.comment()); } + @Test + void testAlterTableKeepsExistingLocation() { + stubDatabaseLocation("s3://test-bucket/gravprobe"); + NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "t"); + Table glueTable = + Table.builder() + .name("t") + .storageDescriptor( + StorageDescriptor.builder().location("s3://other-bucket/existing/t").build()) + .createTime(Instant.now()) + .build(); + when(mockClient.getTable(any(GetTableRequest.class))) + .thenReturn(GetTableResponse.builder().table(glueTable).build()); + when(mockClient.updateTable(any(UpdateTableRequest.class))) + .thenReturn(UpdateTableResponse.builder().build()); + + ArgumentCaptor captor = ArgumentCaptor.forClass(UpdateTableRequest.class); + ops.alterTable(ident, TableChange.updateComment("new comment")); + + verify(mockClient).updateTable(captor.capture()); + assertEquals( + "s3://other-bucket/existing/t", + captor.getValue().tableInput().storageDescriptor().location()); + verify(mockClient, never()).getDatabase(any(GetDatabaseRequest.class)); + } + @Test void testAlterTableSetProperty() { NameIdentifier ident = NameIdentifier.of("metalake", "catalog", "mydb", "t"); diff --git a/docs/aws-glue-catalog.md b/docs/aws-glue-catalog.md index 658e733f512..9ee2cdcf121 100644 --- a/docs/aws-glue-catalog.md +++ b/docs/aws-glue-catalog.md @@ -42,7 +42,7 @@ Besides the [common catalog properties](./gravitino-server-config.md#catalog-pro | `aws-access-key-id` | AWS access key ID for static credential authentication. When omitted, the default credential chain is used. | (none) | No | No | | `aws-secret-access-key` | AWS secret access key paired with `aws-access-key-id`. When omitted, the default credential chain is used. | (none) | No | No | | `aws-glue-endpoint` | Custom Glue endpoint URL for VPC endpoints or LocalStack testing (e.g. `http://localhost:4566`). | (none) | No | No | -| `warehouse` | Base storage path used as the warehouse when no explicit `location` is specified at table creation time (e.g. `s3://my-bucket/warehouse`). Table location is derived as `warehouse/database/table`. | (none) | Yes | No | +| `warehouse` | Base storage path used as the warehouse when no explicit `location` is specified at table creation time and the Glue database declares no `LocationUri` (e.g. `s3://my-bucket/warehouse`). Table location is then derived as `warehouse/database/table`. | (none) | Yes | No | | `default-table-format` | Default format for tables created via Gravitino's `createTable()` API. Accepted values: `iceberg`, `hive`. | `hive` | No | No | | `table-format-filter` | Comma-separated list of table formats exposed by `listTables()` and `loadTable()`. Accepted values: `all`, `hive`, `iceberg`, `delta`, `parquet`. Use to restrict visible table types. | `all` | No | No | @@ -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 database location or warehouse) | No | No | No | | `format` | The table file format (`parquet`, `orc`, `textfile`, etc.). When set, `input-format`, `output-format`, and `serde-lib` are derived automatically. Used primarily when creating Hive-format tables via Trino. | (none) | No | No | Yes | | `input-format` | The input format class for the table, such as `org.apache.hadoop.hive.ql.io.orc.OrcInputFormat`. | `org.apache.hadoop.mapred.TextInputFormat` | No | No | Yes | | `output-format` | The output format class for the table, such as `org.apache.hadoop.hive.ql.io.orc.OrcOutputFormat`. | `org.apache.hadoop.hive.ql.io.HiveIgnoreKeyTextOutputFormat` | No | No | Yes | @@ -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. +The `warehouse` catalog property is required, but it is used only as a fallback when no explicit `location` is specified and the Glue database declares no `LocationUri`. Without an explicit `location`, the table location is derived as `database-location/table` when the database declares a `LocationUri`, or as `warehouse/database/table` otherwise. ### Register an Existing Iceberg Table