Copilot commented on code in PR #11087: URL: https://github.com/apache/gravitino/pull/11087#discussion_r3240217613
########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java: ########## @@ -0,0 +1,566 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue; + +import com.google.common.base.Preconditions; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.FunctionExpression; +import org.apache.gravitino.rel.expressions.NamedReference; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.sorts.NullOrdering; +import org.apache.gravitino.rel.expressions.sorts.SortDirection; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transform; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.rel.types.Types; +import org.apache.iceberg.NullOrder; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.Schema; +import org.apache.iceberg.Table; +import org.apache.iceberg.UpdateProperties; +import org.apache.iceberg.UpdateSchema; +import org.apache.iceberg.aws.glue.GlueCatalog; +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.expressions.UnboundTerm; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper that delegates all Iceberg table operations to the Iceberg SDK's {@code GlueCatalog}. + * + * <p>Unlike the native AWS Glue SDK {@code OpenTableFormatInput} approach, the Iceberg SDK writes + * the {@code metadata.json} file to S3 and registers the table in Glue with the correct {@code + * metadata_location} parameter, making the table usable by Trino Lakehouse connector and other + * Iceberg-native query engines. + */ +final class GlueIcebergTableHelper { + + private static final Logger LOG = LoggerFactory.getLogger(GlueIcebergTableHelper.class); + + private static final String DOT = "."; + + private GlueIcebergTableHelper() {} + + /** + * Returns true if the Glue table is an Iceberg-format table. + * + * <p>Checks for {@code table_type=ICEBERG} in {@code Table.parameters()}. + */ + static boolean isIcebergTable(software.amazon.awssdk.services.glue.model.Table glueTable) { + if (!glueTable.hasParameters()) return false; + return GlueConstants.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase( + glueTable.parameters().get(GlueConstants.TABLE_TYPE_PARAM)); + } + + /** + * Creates an Iceberg {@link Catalog} backed by AWS Glue. + * + * @param config Gravitino catalog configuration (region, credentials, endpoint, etc.) + * @return an initialized Iceberg Glue catalog + */ + static Catalog createGlueCatalog(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument(region != null, "AWS region is required for Iceberg Glue catalog"); + + Map<String, String> icebergProps = new HashMap<>(); + // Warehouse is required by Iceberg catalog initialization but is not used when each table + // provides an explicit location. + icebergProps.put("warehouse", "/tmp/gravitino-glue-iceberg"); + icebergProps.put("catalog-impl", "GlueCatalog"); + icebergProps.put("client.region", region); + + String catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID); + if (catalogId != null) { + icebergProps.put("glue.id", catalogId); + } + + String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); + String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); + if (accessKey != null && secretKey != null) { + icebergProps.put("client.access-key-id", accessKey); + icebergProps.put("client.secret-access-key", secretKey); + } + + String endpoint = config.get(GlueConstants.AWS_GLUE_ENDPOINT); + if (endpoint != null) { + icebergProps.put("client.endpoint", endpoint); + icebergProps.put("s3.endpoint", endpoint); Review Comment: The Glue endpoint configuration is being reused as the S3 endpoint. `aws-glue-endpoint` can be a Glue VPC endpoint, so passing it to S3FileIO will route metadata.json writes to a non-S3 endpoint and break Iceberg table creation outside LocalStack-style setups. Leave `s3.endpoint` unset or add a separate S3 endpoint property. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java: ########## @@ -497,6 +584,49 @@ public GlueTable alterTable(NameIdentifier ident, TableChange... changes) return altered; } + private GlueTable alterIcebergTable( + NameIdentifier ident, String dbName, Table rawGlueTable, TableChange... changes) { + // Register-mode tables (created with METADATA_LOCATION) are not backed by the Iceberg SDK + // GlueCatalog, so we fall back to the native Glue SDK update path. + if (rawGlueTable.hasParameters() + && rawGlueTable.parameters().containsKey(GlueConstants.METADATA_LOCATION)) { + return alterRegisterModeIcebergTable(ident, dbName, rawGlueTable, changes); + } + GlueIcebergTableHelper.alterTable(icebergGlueCatalog, dbName, ident.name(), changes); + return loadTable(ident); + } + Review Comment: This check treats every Iceberg SDK-created table as register-mode because the Iceberg GlueCatalog also writes `metadata_location` into Glue parameters. As a result, normal Iceberg tables created by this PR will never use `GlueIcebergTableHelper.alterTable`, so schema changes such as add/drop/rename column fail through the register-mode fallback. Use an explicit marker for register-mode tables or another discriminator that is not also set by the Iceberg SDK. ########## catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogPropertiesMetadata.java: ########## @@ -64,6 +65,7 @@ void testAwsGlueCatalogIdIsImmutable() { } @Test + @Disabled("TODO: decide if credentials should be hidden") Review Comment: This disables the regression test that protects Glue credential properties from being exposed. Since these properties contain AWS credentials, keep the test enabled and update the production metadata to mark them hidden instead. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java: ########## @@ -632,6 +811,77 @@ private software.amazon.awssdk.services.glue.model.Column toGlueColumn(Column co .build(); } + /** Translates a format name (e.g., "parquet", "orc") to the Hive input format class. */ + private static String getInputFormatClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_INPUT_FORMAT_CLASS; + case "orc": + return HiveStorageConstants.ORC_INPUT_FORMAT_CLASS; + case "textfile": + case "csv": + return HiveStorageConstants.TEXT_INPUT_FORMAT_CLASS; + case "rcfile": + return HiveStorageConstants.RCFILE_INPUT_FORMAT_CLASS; + case "avro": + return HiveStorageConstants.AVRO_INPUT_FORMAT_CLASS; + case "sequencefile": + return HiveStorageConstants.SEQUENCEFILE_INPUT_FORMAT_CLASS; Review Comment: These `HiveStorageConstants` members are package-private in `org.apache.gravitino.catalog.hive`, so referencing them from the Glue package will not compile. Use public constants or local class-name strings for the rcfile/avro/sequencefile mappings. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java: ########## @@ -0,0 +1,566 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.catalog.glue; + +import com.google.common.base.Preconditions; +import java.util.HashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import org.apache.gravitino.rel.Column; +import org.apache.gravitino.rel.TableChange; +import org.apache.gravitino.rel.expressions.FunctionExpression; +import org.apache.gravitino.rel.expressions.NamedReference; +import org.apache.gravitino.rel.expressions.literals.Literal; +import org.apache.gravitino.rel.expressions.sorts.NullOrdering; +import org.apache.gravitino.rel.expressions.sorts.SortDirection; +import org.apache.gravitino.rel.expressions.sorts.SortOrder; +import org.apache.gravitino.rel.expressions.sorts.SortOrders; +import org.apache.gravitino.rel.expressions.transforms.Transform; +import org.apache.gravitino.rel.expressions.transforms.Transforms; +import org.apache.gravitino.rel.types.Type; +import org.apache.gravitino.rel.types.Types; +import org.apache.iceberg.NullOrder; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.Schema; +import org.apache.iceberg.Table; +import org.apache.iceberg.UpdateProperties; +import org.apache.iceberg.UpdateSchema; +import org.apache.iceberg.aws.glue.GlueCatalog; +import org.apache.iceberg.catalog.Catalog; +import org.apache.iceberg.catalog.TableIdentifier; +import org.apache.iceberg.expressions.Expressions; +import org.apache.iceberg.expressions.UnboundTerm; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Helper that delegates all Iceberg table operations to the Iceberg SDK's {@code GlueCatalog}. + * + * <p>Unlike the native AWS Glue SDK {@code OpenTableFormatInput} approach, the Iceberg SDK writes + * the {@code metadata.json} file to S3 and registers the table in Glue with the correct {@code + * metadata_location} parameter, making the table usable by Trino Lakehouse connector and other + * Iceberg-native query engines. + */ +final class GlueIcebergTableHelper { + + private static final Logger LOG = LoggerFactory.getLogger(GlueIcebergTableHelper.class); + + private static final String DOT = "."; + + private GlueIcebergTableHelper() {} + + /** + * Returns true if the Glue table is an Iceberg-format table. + * + * <p>Checks for {@code table_type=ICEBERG} in {@code Table.parameters()}. + */ + static boolean isIcebergTable(software.amazon.awssdk.services.glue.model.Table glueTable) { + if (!glueTable.hasParameters()) return false; + return GlueConstants.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase( + glueTable.parameters().get(GlueConstants.TABLE_TYPE_PARAM)); + } + + /** + * Creates an Iceberg {@link Catalog} backed by AWS Glue. + * + * @param config Gravitino catalog configuration (region, credentials, endpoint, etc.) + * @return an initialized Iceberg Glue catalog + */ + static Catalog createGlueCatalog(Map<String, String> config) { + String region = config.get(GlueConstants.AWS_REGION); + Preconditions.checkArgument(region != null, "AWS region is required for Iceberg Glue catalog"); + + Map<String, String> icebergProps = new HashMap<>(); + // Warehouse is required by Iceberg catalog initialization but is not used when each table + // provides an explicit location. + icebergProps.put("warehouse", "/tmp/gravitino-glue-iceberg"); + icebergProps.put("catalog-impl", "GlueCatalog"); + icebergProps.put("client.region", region); + + String catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID); + if (catalogId != null) { + icebergProps.put("glue.id", catalogId); + } + + String accessKey = config.get(GlueConstants.AWS_ACCESS_KEY_ID); + String secretKey = config.get(GlueConstants.AWS_SECRET_ACCESS_KEY); + if (accessKey != null && secretKey != null) { + icebergProps.put("client.access-key-id", accessKey); + icebergProps.put("client.secret-access-key", secretKey); + } + + String endpoint = config.get(GlueConstants.AWS_GLUE_ENDPOINT); + if (endpoint != null) { + icebergProps.put("client.endpoint", endpoint); + icebergProps.put("s3.endpoint", endpoint); + } + + icebergProps.put("io-impl", "org.apache.iceberg.aws.s3.S3FileIO"); + + GlueCatalog glueCatalog = new GlueCatalog(); + glueCatalog.initialize("gravitino-glue-iceberg", icebergProps); + LOG.info("Initialized Iceberg GlueCatalog for region {}", region); + return glueCatalog; + } + + /** + * Loads an Iceberg table and recovers partitioning and sort orders. + * + * @param icebergCatalog the Iceberg Glue catalog + * @param dbName the Glue database name + * @param tableName the table name + * @param table the GlueTable to populate with recovered metadata + */ + static void loadTable(Catalog icebergCatalog, String dbName, String tableName, GlueTable table) { + Table icebergTable = icebergCatalog.loadTable(TableIdentifier.of(dbName, tableName)); + if (!icebergTable.spec().fields().isEmpty()) { + table.setPartitioning(convertPartitionSpec(icebergTable.spec(), icebergTable.schema())); + } + if (!icebergTable.sortOrder().fields().isEmpty()) { + table.setSortOrders(convertIcebergSortOrder(icebergTable.sortOrder(), icebergTable.schema())); + } + } + + /** + * Creates an Iceberg table via the Iceberg SDK. + * + * <p>The table is written to S3 (metadata.json) and registered in Glue automatically. + * + * @param icebergCatalog the Iceberg Glue catalog + * @param dbName the Glue database name + * @param tableName the table name + * @param columns the table columns + * @param comment the table comment + * @param properties Gravitino table properties (must contain {@code location}) + * @param partitions partition transforms + * @param sortOrders sort orders + */ + static void createTable( + Catalog icebergCatalog, + String dbName, + String tableName, + Column[] columns, + String comment, + Map<String, String> properties, + Transform[] partitions, + SortOrder[] sortOrders) { + + Schema schema = toIcebergSchema(columns); + PartitionSpec spec = toPartitionSpec(schema, partitions); + org.apache.iceberg.SortOrder icebergSortOrder = toSortOrder(schema, sortOrders); + + Map<String, String> tableProps = new HashMap<>(); + if (comment != null) { + tableProps.put("comment", comment); + } + + String location = properties.get(GlueConstants.LOCATION); + Preconditions.checkArgument( + location != null, "Location is required for Iceberg table creation"); + + String format = properties.get(GlueConstants.FORMAT); + if (format != null) { + tableProps.put("write.format.default", format.toLowerCase(Locale.ROOT)); + } Review Comment: User-provided table properties are dropped for Iceberg SDK creates; only the comment and file format are copied into `tableProps`. This means properties passed to `createTable` are not persisted in Iceberg metadata, unlike the native Glue path. Copy the supported user properties into `tableProps` while filtering connector-only keys such as location/table-format. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java: ########## @@ -432,15 +534,8 @@ public GlueTable alterTable(NameIdentifier ident, TableChange... changes) for (TableChange change : changes) { if (change instanceof TableChange.RenameTable) { - TableChange.RenameTable renameTable = (TableChange.RenameTable) change; - renameTable - .getNewSchemaName() - .ifPresent( - s -> { - throw new UnsupportedOperationException( - "Glue does not support cross-schema table rename"); - }); - newName = renameTable.getNewName(); + // Already handled above; this branch is never reached. + throw new UnsupportedOperationException("Glue does not support table rename"); Review Comment: This change removes the previously supported same-schema rename path for non-Iceberg Glue tables; the old implementation updated `TableInput.name()`, and the PR does not document a breaking API change. Either preserve the native Glue rename behavior for non-Iceberg tables or explicitly gate only unsupported cross-schema/Iceberg renames. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java: ########## @@ -64,7 +64,7 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata + " When omitted the default credential chain is used.", false /* immutable */, null /* defaultValue */, - true /* hidden */)) + false /* hidden */)) Review Comment: These credential properties are sensitive and should remain hidden; making them visible exposes access keys through catalog properties returned to clients. Keep `aws-access-key-id` and `aws-secret-access-key` marked hidden. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java: ########## @@ -73,7 +73,7 @@ public class GlueCatalogPropertiesMetadata extends BaseCatalogPropertiesMetadata + " When omitted the default credential chain is used.", false /* immutable */, null /* defaultValue */, - true /* hidden */)) + false /* hidden */)) Review Comment: These credential properties are sensitive and should remain hidden; making the secret access key visible can expose credentials through catalog metadata APIs. Keep this property marked hidden. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java: ########## @@ -632,6 +811,77 @@ private software.amazon.awssdk.services.glue.model.Column toGlueColumn(Column co .build(); } + /** Translates a format name (e.g., "parquet", "orc") to the Hive input format class. */ + private static String getInputFormatClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_INPUT_FORMAT_CLASS; + case "orc": + return HiveStorageConstants.ORC_INPUT_FORMAT_CLASS; + case "textfile": + case "csv": + return HiveStorageConstants.TEXT_INPUT_FORMAT_CLASS; + case "rcfile": + return HiveStorageConstants.RCFILE_INPUT_FORMAT_CLASS; + case "avro": + return HiveStorageConstants.AVRO_INPUT_FORMAT_CLASS; + case "sequencefile": + return HiveStorageConstants.SEQUENCEFILE_INPUT_FORMAT_CLASS; + case "json": + case "regex": + default: + return HiveStorageConstants.TEXT_INPUT_FORMAT_CLASS; + } + } + + /** Translates a format name to the Hive output format class. */ + private static String getOutputFormatClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_OUTPUT_FORMAT_CLASS; + case "orc": + return HiveStorageConstants.ORC_OUTPUT_FORMAT_CLASS; + case "textfile": + case "csv": + return HiveStorageConstants.IGNORE_KEY_OUTPUT_FORMAT_CLASS; + case "rcfile": + return HiveStorageConstants.RCFILE_OUTPUT_FORMAT_CLASS; + case "avro": + return HiveStorageConstants.AVRO_OUTPUT_FORMAT_CLASS; + case "sequencefile": + return HiveStorageConstants.SEQUENCEFILE_OUTPUT_FORMAT_CLASS; Review Comment: These `HiveStorageConstants` members are package-private in `org.apache.gravitino.catalog.hive`, so referencing them from the Glue package will not compile. Use public constants or local class-name strings for the rcfile/avro/sequencefile mappings. ########## catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java: ########## @@ -632,6 +811,77 @@ private software.amazon.awssdk.services.glue.model.Column toGlueColumn(Column co .build(); } + /** Translates a format name (e.g., "parquet", "orc") to the Hive input format class. */ + private static String getInputFormatClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_INPUT_FORMAT_CLASS; + case "orc": + return HiveStorageConstants.ORC_INPUT_FORMAT_CLASS; + case "textfile": + case "csv": + return HiveStorageConstants.TEXT_INPUT_FORMAT_CLASS; + case "rcfile": + return HiveStorageConstants.RCFILE_INPUT_FORMAT_CLASS; + case "avro": + return HiveStorageConstants.AVRO_INPUT_FORMAT_CLASS; + case "sequencefile": + return HiveStorageConstants.SEQUENCEFILE_INPUT_FORMAT_CLASS; + case "json": + case "regex": + default: + return HiveStorageConstants.TEXT_INPUT_FORMAT_CLASS; + } + } + + /** Translates a format name to the Hive output format class. */ + private static String getOutputFormatClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_OUTPUT_FORMAT_CLASS; + case "orc": + return HiveStorageConstants.ORC_OUTPUT_FORMAT_CLASS; + case "textfile": + case "csv": + return HiveStorageConstants.IGNORE_KEY_OUTPUT_FORMAT_CLASS; + case "rcfile": + return HiveStorageConstants.RCFILE_OUTPUT_FORMAT_CLASS; + case "avro": + return HiveStorageConstants.AVRO_OUTPUT_FORMAT_CLASS; + case "sequencefile": + return HiveStorageConstants.SEQUENCEFILE_OUTPUT_FORMAT_CLASS; + case "json": + case "regex": + default: + return HiveStorageConstants.IGNORE_KEY_OUTPUT_FORMAT_CLASS; + } + } + + /** Translates a format name to the Hive SerDe class. */ + private static String getSerdeClass(String format) { + switch (format) { + case "parquet": + return HiveStorageConstants.PARQUET_SERDE_CLASS; + case "orc": + return HiveStorageConstants.ORC_SERDE_CLASS; + case "textfile": + return HiveStorageConstants.LAZY_SIMPLE_SERDE_CLASS; + case "csv": + return HiveStorageConstants.OPENCSV_SERDE_CLASS; + case "rcfile": + return HiveStorageConstants.COLUMNAR_SERDE_CLASS; + case "avro": + return HiveStorageConstants.AVRO_SERDE_CLASS; + case "json": + return HiveStorageConstants.JSON_SERDE_CLASS; + case "regex": + return HiveStorageConstants.REGEX_SERDE_CLASS; Review Comment: Several constants used here (`COLUMNAR_SERDE_CLASS`, `AVRO_SERDE_CLASS`, `JSON_SERDE_CLASS`, and `REGEX_SERDE_CLASS`) are package-private in `HiveStorageConstants`, so this code will not compile from the Glue package. Use accessible public constants or define the Glue mappings locally. -- 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]
