Copilot commented on code in PR #11186:
URL: https://github.com/apache/gravitino/pull/11186#discussion_r3280165731
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/util/SparkUtilIT.java:
##########
@@ -134,12 +135,45 @@ protected List<String> getTableMetadata(String
getTableMetadataSql) {
}
// Create SparkTableInfo from SparkBaseTable retrieved from LogicalPlan.
+ // In Spark 3.3/3.5: DESC TABLE EXTENDED returns DescribeRelation.
+ // In Spark 3.4: DESC TABLE EXTENDED returns DescribeTableCommand (different
class hierarchy).
+ // Use the v2 Catalog API (CatalogManager + TableCatalog.loadTable) for
cross-version
+ // compatibility.
protected SparkTableInfo getTableInfo(String tableName) {
- Dataset ds = getSparkSession().sql("DESC TABLE EXTENDED " + tableName);
- CommandResult result = (CommandResult) ds.logicalPlan();
- DescribeRelation relation = (DescribeRelation) result.commandLogicalPlan();
- ResolvedTable table = (ResolvedTable) relation.child();
- return SparkTableInfo.create(table.table());
+ CatalogManager catalogManager =
getSparkSession().sessionState().catalogManager();
+
+ // Parse tableName: could be short (tbl), partially-qualified (db.tbl),
+ // or fully-qualified (cat.db.tbl).
+ String[] parts = tableName.split("\\.");
+ Identifier identifier;
+ TableCatalog tableCatalog;
+ if (parts.length == 1) {
+ // Short table name: use current catalog + current V2 namespace.
+ // catalog().currentDatabase() returns the V1 Hive session catalog
database and is NOT
+ // updated when USE <db> is issued against a V2 catalog (e.g. Glue) in
Spark 3.3.
+ // catalogManager.currentNamespace() reflects the V2 namespace correctly.
+ CatalogPlugin currentCatalog = catalogManager.currentCatalog();
+ String[] currentNamespace = catalogManager.currentNamespace();
+ identifier = Identifier.of(currentNamespace, parts[0]);
+ tableCatalog = (TableCatalog) currentCatalog;
+ } else if (parts.length == 2) {
+ // Partially qualified: db.table
+ identifier = Identifier.of(new String[] {parts[0]}, parts[1]);
+ CatalogPlugin currentCatalog = catalogManager.currentCatalog();
+ tableCatalog = (TableCatalog) currentCatalog;
+ } else if (parts.length == 3) {
+ // Fully qualified: cat.db.table
+ identifier = Identifier.of(new String[] {parts[0], parts[1]}, parts[2]);
+ CatalogPlugin catalog = catalogManager.catalog(parts[0]);
+ tableCatalog = (TableCatalog) catalog;
Review Comment:
In the fully-qualified case (cat.db.table), the Identifier namespace should
only contain the database/namespace (db). Including the catalog name in the
namespace (Identifier.of(new String[]{cat, db}, table)) will cause
TableCatalog.loadTable() to look for a nested namespace and can break lookups.
Build the Identifier with namespace [db] and use catalogManager.catalog(cat) as
you already do.
##########
spark-connector/v3.4/spark/src/main/java/org/apache/gravitino/spark/connector/glue/GravitinoGlueCatalogSpark34.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.spark.connector.glue;
+
+import org.apache.gravitino.spark.connector.SparkTableChangeConverter;
+import org.apache.gravitino.spark.connector.SparkTableChangeConverter34;
+import org.apache.gravitino.spark.connector.SparkTypeConverter;
+
+/** Spark 3.4 specific Gravitino Glue catalog implementation. */
+public class GravitinoGlueCatalogSpark34 extends GravitinoGlueCatalog {
+ @Override
+ protected SparkTypeConverter getSparkTypeConverter() {
+ return new
org.apache.gravitino.spark.connector.hive.SparkHiveTypeConverter34();
+ }
Review Comment:
The Spark 3.4 glue catalog uses a fully-qualified class name in code (new
org.apache.gravitino...SparkHiveTypeConverter34()). Per the project's Java
style guidelines, prefer adding an import and using the simple class name to
avoid FQNs in method bodies unless resolving a name collision.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueMetadataAdapter.java:
##########
@@ -0,0 +1,190 @@
+/*
+ * 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.trino.connector.catalog.glue;
+
+import static com.google.common.collect.ImmutableList.toImmutableList;
+import static io.trino.spi.session.PropertyMetadata.integerProperty;
+import static io.trino.spi.session.PropertyMetadata.stringProperty;
+import static io.trino.spi.type.VarcharType.VARCHAR;
+import static java.util.Locale.ENGLISH;
+
+import com.google.common.collect.ImmutableList;
+import io.trino.spi.connector.ConnectorTableMetadata;
+import io.trino.spi.session.PropertyMetadata;
+import io.trino.spi.type.ArrayType;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+import org.apache.gravitino.catalog.property.PropertyConverter;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.trino.connector.catalog.hive.HiveMetadataAdapter;
+import org.apache.gravitino.trino.connector.catalog.hive.SortingColumn;
+import org.apache.gravitino.trino.connector.catalog.iceberg.ExpressionUtil;
+import org.apache.gravitino.trino.connector.metadata.GravitinoTable;
+
+/**
+ * Transforming Apache Gravitino Glue metadata to Trino. This adapter handles
properties that are
+ * specific to the Glue catalog and the lakehouse connector, excluding
properties that conflict with
+ * the lakehouse connector's native properties.
+ */
+public class GlueMetadataAdapter extends HiveMetadataAdapter {
+
+ /** The table type property for lakehouse connector (ICEBERG, HIVE, DELTA).
*/
+ static final String LAKEHOUSE_TABLE_TYPE = "type";
+
+ private static final List<PropertyMetadata<?>> GLUE_TABLE_PROPERTY_META =
+ ImmutableList.of(
+ stringProperty(LAKEHOUSE_TABLE_TYPE, "The type of table (ICEBERG,
HIVE)", null, false),
+ new PropertyMetadata<>(
+ "partitioned_by",
+ "Partition columns",
+ new ArrayType(VARCHAR),
+ List.class,
+ ImmutableList.of(),
+ false,
+ value ->
+ ((List<?>) value)
+ .stream()
+ .map(name -> ((String)
name).toLowerCase(java.util.Locale.ENGLISH))
+ .collect(ImmutableList.toImmutableList()),
Review Comment:
This class already statically imports Locale.ENGLISH, but still uses
java.util.Locale.ENGLISH in the property normalizers. Please use the imported
ENGLISH constant (or add a normal import) instead of a fully-qualified
reference to follow the codebase's 'no FQN in method bodies' convention.
##########
catalogs/catalog-glue/build.gradle.kts:
##########
@@ -96,12 +96,7 @@ tasks {
}
tasks.test {
- val skipITs = project.hasProperty("skipITs")
- if (skipITs) {
- exclude("**/integration/test/**")
- } else {
- dependsOn(tasks.jar)
- }
+ dependsOn(tasks.jar)
Review Comment:
The catalog-glue test task no longer honors the `-PskipITs` convention used
across the repo to exclude integration tests under `**/integration/test/**`.
This can unintentionally run real-AWS integration tests (e.g., AwsGlueCatalogIT
is enabled when AWS_ACCESS_KEY_ID is set) during a normal `test` run,
potentially creating/modifying Glue resources. Consider restoring the skipITs
exclusion (and/or moving AWS tests to a dedicated integrationTest task/source
set).
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/glue/SparkGlueCatalogIT.java:
##########
@@ -0,0 +1,620 @@
+/*
+ * 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.spark.connector.integration.test.glue;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.gravitino.catalog.glue.GlueConstants;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfo;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfo.SparkColumnInfo;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfoChecker;
+import org.apache.spark.sql.types.DataTypes;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Integration test for GravitinoGlueCatalog in Spark connector.
+ *
+ * <p>Tests mixed table type support (Hive format + Iceberg format) in a
single Glue database. Uses
+ * Moto server to mock AWS Glue API, similar to MotoGlueCatalogIT in the
server module.
+ */
+public abstract class SparkGlueCatalogIT extends SparkGlueEnvIT {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(SparkGlueCatalogIT.class);
+
+ private String glueEndpoint;
+ private String awsRegion = "us-east-1";
+ private String awsAccessKeyId = "test";
+ private String awsSecretAccessKey = "test";
+
+ @Override
+ protected String getCatalogName() {
+ return "glue";
+ }
+
+ @Override
+ protected String getProvider() {
+ return "glue";
+ }
+
+ @Override
+ protected Map<String, String> getCatalogConfigs() {
+ Map<String, String> catalogProperties = new java.util.HashMap<>();
+ catalogProperties.put(GlueConstants.AWS_REGION, awsRegion);
+ catalogProperties.put(GlueConstants.AWS_ACCESS_KEY_ID, awsAccessKeyId);
+ catalogProperties.put(GlueConstants.AWS_SECRET_ACCESS_KEY,
awsSecretAccessKey);
+ if (glueEndpoint != null) {
+ catalogProperties.put(GlueConstants.AWS_GLUE_ENDPOINT, glueEndpoint);
+ }
+ return catalogProperties;
+ }
+
+ @Override
+ protected boolean supportsSparkSQLClusteredBy() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsPartition() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsDelete() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsSchemaEvolution() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsReplaceColumns() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsSchemaAndTableProperties() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsComplexType() {
+ // Glue does not support Gravitino complex types (LIST, MAP, STRUCT) in
table columns.
+ return false;
+ }
+
+ @Override
+ protected boolean supportsUpdateColumnPosition() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsFunction() {
+ return false;
+ }
+
+ /**
+ * Sets the Glue endpoint for testing. Called by subclasses after Moto
container is started.
+ *
+ * @param endpoint the Glue API endpoint URL
+ */
+ protected void setGlueEndpoint(String endpoint) {
+ this.glueEndpoint = endpoint;
+ }
+
+ protected void setAwsRegion(String region) {
+ this.awsRegion = region;
+ }
+
+ /**
+ * Sets AWS credentials for testing.
+ *
+ * @param accessKeyId AWS access key ID
+ * @param secretAccessKey AWS secret access key
+ */
+ protected void setAwsCredentials(String accessKeyId, String secretAccessKey)
{
+ this.awsAccessKeyId = accessKeyId;
+ this.awsSecretAccessKey = secretAccessKey;
+ }
+
+ @Override
+ protected String getDefaultAwsRegion() {
+ return awsRegion;
+ }
+
+ @Override
+ protected String getGlueEndpoint() {
+ return glueEndpoint;
+ }
+
+ protected String getAwsRegion() {
+ return awsRegion;
+ }
+
+ /**
+ * Overrides to use CASCADE so that databases with stale tables (e.g., from
prior test runs) can
+ * be cleaned up. Glue tables persist across JVM restarts and may be left
behind after a crash.
+ */
+ @Override
+ protected void dropDatabaseIfExists(String database) {
+ sql("DROP DATABASE IF EXISTS " + database + " CASCADE");
+ }
+
+ /**
+ * Overrides to always use PARQUET format for Glue. Without an explicit
USING clause, Spark may
+ * route CREATE TABLE through the V1 Hive path, bypassing the
location-derivation logic in
+ * GravitinoGlueCatalog.createTable and leaving tables without a stored S3
location.
+ */
+ @Override
+ protected void createSimpleTable(String identifier) {
+ sql(getCreateSimpleTableString(identifier) + " USING PARQUET");
+ }
+
+ /**
+ * Overrides to recreate the database with the correct S3 location. The base
implementation uses
+ * CREATE DATABASE IF NOT EXISTS with a local HDFS path (/user/hive/db),
causing tables to inherit
+ * a local path as their default location. We drop and recreate to ensure
the S3 location is
+ * always set correctly, so stale data cleanup via dropTableIfExists works
reliably.
+ */
+ @Override
+ protected void createDatabaseIfNotExists(String database, String provider) {
+ String dbLocation = warehouse + "/" + database;
+ dropDatabaseIfExists(database);
+ // Delete S3 data directory so stale data files from prior runs don't
cause duplicate rows.
+ deleteDirIfExists(dbLocation);
+ sql(String.format("CREATE DATABASE %s LOCATION '%s'", database,
dbLocation));
+ }
+
+ /**
+ * Overrides to also delete the S3 data directory after dropping the table.
Unlike HDFS managed
+ * tables, Glue external tables do not delete S3 data on DROP TABLE. Stale
data files cause
+ * duplicate rows on the next create+insert cycle.
+ */
+ @Override
+ protected void dropTableIfExists(String tableName) {
+ String location = null;
+ try {
+ location = getTableInfo(tableName).getTableLocation();
+ } catch (Exception e) {
+ // Table may not exist yet — location stays null, nothing to delete.
+ LOG.debug("Could not get location for table {}: {}", tableName,
e.getMessage());
+ }
+ super.dropTableIfExists(tableName);
+ if (location != null) {
+ deleteDirIfExists(location);
+ }
+ }
+
+ /**
+ * Overrides base class: use USING PARQUET to ensure the table goes through
the Gravitino Glue
+ * catalog (V2 path).
+ */
+ @Test
+ @Override
+ protected void testDropAndWriteTable() {
+ String tableName = "drop_then_create_write_table";
+ dropTableIfExists(tableName);
+ sql(getCreateSimpleTableString(tableName) + " USING PARQUET");
+ SparkTableInfo info = getTableInfo(tableName);
+ checkTableReadWrite(info);
+
+ // External tables on S3 do not delete data on DROP TABLE; clean up
explicitly.
+ String location = info.getTableLocation();
+ dropTableIfExists(tableName);
+ if (location != null) {
+ deleteDirIfExists(location);
+ }
+
+ sql(getCreateSimpleTableString(tableName) + " USING PARQUET");
+ checkTableReadWrite(getTableInfo(tableName));
+ }
+
+ /**
+ * Overrides base class: skip this test due to a known issue where ALTER
TABLE RENAME fails for
+ * non-Iceberg (PARQUET) tables via the Glue catalog path. The rename
operation triggers
+ * tableExists() which calls tableCatalog.loadTable() →
GravitinoGlueCatalog.loadTable() →
+ * loadSparkTable() → HiveTableCatalog.loadTable(ident), but the table
lookup may fail because
+ * Derby and Glue are out of sync for renamed tables. This requires further
investigation into the
+ * HiveTableCatalog.loadTable() behavior and the renameTable dispatch chain
in
+ * RenameTableExec.apply().
+ */
+ @Test
+ @Override
+ protected void testRenameTable() {
+ // Skipped pending investigation. Rename logic in Glue backend works at
the REST API level
+ // (GlueCatalogOperations.alterTable), but the Spark catalog path for
non-Iceberg tables needs
+ // verification of how Derby/Hive metastore tracks renamed tables.
+ }
+
+ // -------------------------------------------------------------------------
+ // Test mixed table types (Hive format + Iceberg format)
+ // -------------------------------------------------------------------------
+
+ @Test
+ void testCreateHiveFormatTable() {
+ String tableName = "test_hive_format_table";
+ dropTableIfExists(tableName);
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += " USING PARQUET";
+ sql(createTableSql);
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ SparkTableInfoChecker checker =
+
SparkTableInfoChecker.create().withName(tableName).withColumns(getSimpleTableColumn());
+ checker.check(tableInfo);
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ void testCreateIcebergFormatTable() {
+ String tableName = "test_iceberg_format_table";
+ dropTableIfExists(tableName);
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += " USING iceberg";
+ sql(createTableSql);
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ SparkTableInfoChecker checker =
+ SparkTableInfoChecker.create()
+ .withName(tableName)
+ .withColumns(getSimpleIcebergTableColumn());
+ checker.check(tableInfo);
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ void testMixedTableTypesInSameDatabase() {
+ String hiveTable = "mixed_hive_table";
+ String icebergTable = "mixed_iceberg_table";
+
+ // Create non-partitioned Hive format table
+ dropTableIfExists(hiveTable);
+ sql(getCreateSimpleTableString(hiveTable) + " USING PARQUET");
+
+ // Create non-partitioned Iceberg format table
+ dropTableIfExists(icebergTable);
+ sql(getCreateSimpleTableString(icebergTable) + " USING iceberg");
+
+ // Both tables should be accessible
+ SparkTableInfo hiveTableInfo = getTableInfo(hiveTable);
+ SparkTableInfoChecker hiveChecker =
+
SparkTableInfoChecker.create().withName(hiveTable).withColumns(getSimpleTableColumn());
+ hiveChecker.check(hiveTableInfo);
+ checkTableReadWrite(hiveTableInfo);
+
+ SparkTableInfo icebergTableInfo = getTableInfo(icebergTable);
+ SparkTableInfoChecker icebergChecker =
+ SparkTableInfoChecker.create()
+ .withName(icebergTable)
+ .withColumns(getSimpleIcebergTableColumn());
+ icebergChecker.check(icebergTableInfo);
+ checkTableReadWrite(icebergTableInfo);
+ }
+
+ @Test
+ void testHivePartitionedTable() {
+ String tableName = "test_hive_partitioned_table";
+ dropTableIfExists(tableName);
+ // Use existing columns as partition keys (datasource-style) so partition
columns stay in
+ // schema.
+ // Spark places partition columns last; columns = [id, age, name] with
name as partition.
+ sql(
+ "CREATE TABLE "
+ + tableName
+ + " (id INT COMMENT 'id comment', age INT, name STRING COMMENT '')
USING PARQUET"
+ + " PARTITIONED BY (name)");
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ SparkTableInfoChecker checker =
+ SparkTableInfoChecker.create()
+ .withName(tableName)
+ .withColumns(
+ Arrays.asList(
+ SparkColumnInfo.of("id", DataTypes.IntegerType, "id
comment"),
+ SparkColumnInfo.of("age", DataTypes.IntegerType, null),
+ SparkColumnInfo.of("name", DataTypes.StringType, "")))
+ .withIdentifyPartition(Arrays.asList("name"));
+ checker.check(tableInfo);
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ void testIcebergPartitionedTable() {
+ String tableName = "test_iceberg_partitioned_table";
+ dropTableIfExists(tableName);
+ // Partition by an existing column (id). Iceberg stores partition columns
separately from
+ // schema.
+ sql(getCreateSimpleTableString(tableName) + " USING iceberg PARTITIONED BY
(id)");
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ SparkTableInfoChecker checker =
+ SparkTableInfoChecker.create()
+ .withName(tableName)
+ .withColumns(getSimpleIcebergTableColumn());
+ checker.check(tableInfo);
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ void testInsertHiveTable() {
+ String tableName = "test_insert_hive_table";
+ dropTableIfExists(tableName);
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += " USING PARQUET";
+ sql(createTableSql);
+
+ sql(String.format("INSERT INTO %s VALUES (1, 'name1', 25)", tableName));
+ List<String> tableData = getTableData(tableName);
+ Assertions.assertFalse(tableData.isEmpty());
+ Assertions.assertEquals("1,name1,25", tableData.get(0));
+ }
+
+ @Test
+ void testInsertIcebergTable() {
+ String tableName = "test_insert_iceberg_table";
+ dropTableIfExists(tableName);
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += " USING iceberg";
+ sql(createTableSql);
+
+ sql(String.format("INSERT INTO %s VALUES (1, 'name1', 25)", tableName));
+ List<String> tableData = getTableData(tableName);
+ Assertions.assertFalse(tableData.isEmpty());
+ Assertions.assertEquals("1,name1,25", tableData.get(0));
+ }
+
+ @Test
+ void testCreateTableWithComment() {
+ String tableName = "test_table_with_comment";
+ dropTableIfExists(tableName);
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += " USING PARQUET COMMENT 'Test table comment'";
+ sql(createTableSql);
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ Assertions.assertEquals("Test table comment", tableInfo.getComment());
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ void testExternalTableLocation() {
+ String tableName = "test_external_table";
+ dropTableIfExists(tableName);
+ String externalLocation = warehouse + "/external_glue_db/external_table";
+ deleteDirIfExists(externalLocation);
+
+ String createTableSql = getCreateSimpleTableString(tableName);
+ createTableSql += String.format(" USING PARQUET LOCATION '%s'",
externalLocation);
+ sql(createTableSql);
+
+ SparkTableInfo tableInfo = getTableInfo(tableName);
+ Assertions.assertEquals(externalLocation, tableInfo.getTableLocation());
+ checkTableReadWrite(tableInfo);
+ }
+
+ @Test
+ @Override
+ protected void testLoadCatalogs() {
+ // Glue catalog is not shown in SHOW CATALOGS output (Gravitino registers
it lazily via Spark
+ // plugin). Verify accessibility by listing databases instead.
+ Assertions.assertDoesNotThrow(() -> sql("SHOW DATABASES IN " +
getCatalogName()));
+ }
+
+ /**
+ * Overrides base: ensures the table is cleaned up before creation to handle
stale state from
+ * prior test runs (Glue tables persist across JVM restarts unlike in-memory
Derby).
+ */
+ @Test
+ void testDropTable() {
+ String tableName = "drop_table";
+ dropTableIfExists(tableName);
+ createSimpleTable(tableName);
+ Assertions.assertTrue(tableExists(tableName));
+
+ dropTableIfExists(tableName);
+ Assertions.assertFalse(tableExists(tableName));
+
+ Assertions.assertThrows(Exception.class, () -> sql("DROP TABLE
not_exists"));
+ }
+
+ /**
+ * Overrides base: skips S3 directory verification when no explicit LOCATION
was given. Glue does
+ * not store the auto-assigned warehouse location in table properties, so we
cannot reconstruct
+ * the exact S3 path. Data read/write correctness is already validated by
{@link
+ * #checkTableReadWrite}.
+ */
+ @Override
+ protected void checkPartitionDirExists(SparkTableInfo table) {
+ if (table.getTableLocation() == null) {
+ return;
+ }
+ super.checkPartitionDirExists(table);
+ }
+
+ // -------------------------------------------------------------------------
+ // Override unsupported operation tests (Glue doesn't support these)
+ // -------------------------------------------------------------------------
+
+ /** Glue doesn't support DROP COLUMNS, so skip this test. */
+ @Test
+ void testAlterTableAddAndDeleteColumn() {
+ // Glue doesn't support DROP COLUMNS — skip
+ }
+
+ /** Glue doesn't support CHANGE COLUMN, so skip this test. */
+ @Test
+ void testAlterTableUpdateColumnType() {
+ // Glue doesn't support ALTER TABLE CHANGE COLUMN — skip
+ }
+
+ /** Glue doesn't support RENAME COLUMN, so skip this test. */
+ @Test
+ void testAlterTableRenameColumn() {
+ // Glue doesn't support RENAME COLUMN — skip
+ }
Review Comment:
These no-op methods are intended to override SparkCommonIT's ALTER TABLE
tests, but SparkCommonIT defines them with package-private access (see
SparkCommonIT around
testAlterTableAddAndDeleteColumn/testAlterTableUpdateColumnType/testAlterTableRenameColumn).
Because this class is in a different package, these methods do not override
the superclass tests, so the original failing tests will still execute. Make
the superclass test methods protected and add `@Override` here (and ideally
`@Disabled` with a reason) so the unsupported-operation tests are actually
skipped for Glue.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java:
##########
@@ -73,7 +74,7 @@ public class GlueCatalogPropertiesMetadata extends
BaseCatalogPropertiesMetadata
+ " When omitted the default credential chain is used.",
false /* immutable */,
null /* defaultValue */,
- false /* hidden */))
+ true /* hidden */))
Review Comment:
Now that aws-secret-access-key is marked hidden, consider also marking
aws-access-key-id as hidden. Access key IDs are still part of static
credentials and can be sensitive; hiding both reduces the chance of credential
material being exposed via Catalog.properties()/APIs/UI/logs while still
allowing the backend to read the values internally.
##########
gradle/libs.versions.toml:
##########
@@ -18,6 +18,7 @@
#
[versions]
awssdk = "2.31.73"
+aws-glue-datacatalog = "3.4.0"
Review Comment:
The new version/catalog entries for aws-glue-datacatalog are not referenced
anywhere in the repo (no Gradle usage found). Keeping unused version-catalog
entries increases maintenance overhead and can confuse future dependency
management; either wire this library into a build where it's needed or remove
the unused entries.
##########
gradle/libs.versions.toml:
##########
@@ -149,12 +150,14 @@ ql-expression = "4.0.3"
[libraries]
aspectj-aspectjrt = { group = "org.aspectj", name = "aspectjrt", version.ref =
"aspectj" }
+aws-dynamodb = { group = "software.amazon.awssdk", name = "dynamodb",
version.ref = "awssdk" }
aws-glue = { group = "software.amazon.awssdk", name = "glue", version.ref =
"awssdk" }
aws-iam = { group = "software.amazon.awssdk", name = "iam", version.ref =
"awssdk" }
aws-policy = { group = "software.amazon.awssdk", name = "iam-policy-builder",
version.ref = "awssdk" }
aws-s3 = { group = "software.amazon.awssdk", name = "s3", version.ref =
"awssdk" }
aws-sts = { group = "software.amazon.awssdk", name = "sts", version.ref =
"awssdk" }
aws-kms = { group = "software.amazon.awssdk", name = "kms", version.ref =
"awssdk" }
+aws-glue-datacatalog-hive3 = { group = "com.amazonaws.glue", name =
"aws-glue-datacatalog-hive3-client", version.ref = "aws-glue-datacatalog" }
Review Comment:
The aws-glue-datacatalog-hive3 library entry appears unused (no references
in build.gradle(.kts)). If it’s not needed for this PR, please remove it;
otherwise add the corresponding dependency usage so the version catalog doesn’t
accumulate dead entries.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/glue/SparkGlueEnvIT.java:
##########
@@ -0,0 +1,360 @@
+/*
+ * 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.spark.connector.integration.test.glue;
+
+import java.io.IOException;
+import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.spark.connector.GravitinoSparkConfig;
+import org.apache.gravitino.spark.connector.integration.test.SparkCommonIT;
+import org.apache.gravitino.spark.connector.plugin.GravitinoSparkPlugin;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileStatus;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.spark.SparkConf;
+import org.apache.spark.sql.SparkSession;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Base environment setup for Spark Glue connector integration tests.
+ *
+ * <p>Replaces the Hive/HDFS initialization chain from SparkEnvIT with
Glue-appropriate setup. Glue
+ * does not require Hive metastore or HDFS — it uses AWS Glue API (mocked by
LocalStack) and S3
+ * storage.
+ *
+ * <p>Subclasses must:
+ *
+ * <ul>
+ * <li>Start LocalStack container in their {@link #startUp()} and call {@link
+ * SparkGlueCatalogIT#setGlueEndpoint(String)}
+ * <li>Set AWS credentials via {@link
SparkGlueCatalogIT#setAwsCredentials(String, String)}
+ * <li>Set S3 credentials via {@link #setS3Credentials(String, String,
String)}
+ * <li>Call {@code super.startUp()} after configuring Glue
+ * </ul>
+ */
+public abstract class SparkGlueEnvIT extends SparkCommonIT {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(SparkGlueEnvIT.class);
+
+ private SparkSession sparkSession;
+
+ private String s3AccessKey;
+ private String s3SecretKey;
+ private String s3Endpoint;
+ private String s3BucketName = S3_BUCKET_NAME;
+
+ protected static final String S3_BUCKET_NAME = "ice-glue-test-01";
+ protected static final int DEFAULT_GRAVITINO_PORT = 8090;
+
+ @Override
+ protected SparkSession getSparkSession() {
+ return sparkSession;
+ }
+
+ /**
+ * Starts the test environment. Subclasses must call super.startUp() after
configuring the Glue
+ * endpoint and credentials.
+ */
+ @BeforeAll
+ protected void startUp() throws Exception {
+ warehouse = "s3a://" + s3BucketName + "/warehouse";
+ hiveMetastoreUri = null;
+ hdfs = null;
+
+ int gravitinoPort;
+ boolean serverWasStartedByThisClass = false;
+ if (serverConfig != null) {
+ // Gravitino server already started by an external process (e.g., real
AWS integration
+ // test environment). Use existing config.
+ gravitinoPort = getGravitinoServerPort();
+ } else {
+ // Start the embedded Gravitino server.
SparkEnvIT.startIntegrationTest() is an empty
+ // @BeforeAll override that prevents JUnit from auto-invoking
BaseIT.startIntegrationTest().
+ // We call startServer() directly (a non-@BeforeAll method) to avoid the
virtual-dispatch
+ // problem that would occur with reflection + Method.invoke().
+ try {
+ startServer();
+ gravitinoPort = getGravitinoServerPort();
+ serverWasStartedByThisClass = true;
+ } catch (Exception e) {
+ LOG.warn(
+ "Failed to start embedded Gravitino, assuming externally-provided
server on port {}. Reason: {}",
+ DEFAULT_GRAVITINO_PORT,
+ e.getMessage());
+ gravitinoPort = DEFAULT_GRAVITINO_PORT;
+ }
+ }
+ String gravitinoUri = String.format("http://127.0.0.1:%d";, gravitinoPort);
+ // Only initialize metalake and catalog if this class started Gravitino
itself.
+ // For externally-provided Gravitino (e.g., real AWS tests), metalake and
catalog
+ // must already exist.
+ if (serverWasStartedByThisClass) {
+ initMetalakeAndCatalogs();
+ }
+ initSparkEnv(gravitinoUri);
+ // Create the default database in the Glue catalog so that tests can USE
it.
+ // Note: SparkCommonIT.initDefaultDatabase() is package-private and cannot
be @Override'd
+ // from this package, so we call this class's own implementation directly.
+ initDefaultDatabase();
+
+ LOG.info(
+ "Startup Spark env for Glue successfully, Gravitino uri: {},
Warehouse: {}",
+ gravitinoUri,
+ warehouse);
+ }
+
+ @BeforeEach
+ protected void init() {
+ // Spark's Hive-style catalog integration treats USE <name> as switching
databases,
+ // not catalogs. We must use SET to set the default catalog instead.
+ sql("SET spark.sql.defaultCatalog=" + getCatalogName());
+ sql("USE " + getDefaultDatabase());
+ }
+
+ protected String getDefaultAwsRegion() {
+ return "us-east-1";
+ }
+
+ /**
+ * Skips the Hive container initialization. Glue uses AWS Glue API (catalog)
and S3 (storage), not
+ * a Hive metastore or HDFS. This method shadows the private {@link
SparkEnvIT#initHiveEnv()} from
+ * the grandparent class — no @Override since the parent method is private
(not inheritable).
+ */
+ protected void initHiveEnv() {
+ // Skip Hive container startup. Glue uses AWS Glue API + S3, not HMS.
+ // warehouse and hiveMetastoreUri were already set in startUp():
+ // - warehouse = "s3a://bucket/warehouse" (already set)
+ // - hiveMetastoreUri = null (already set)
+ }
+
+ /**
+ * Overrides HDFS filesystem initialization. Glue uses S3, not HDFS, so this
is a no-op. Note:
+ * no @Override because the parent method is private (not inheritable).
+ */
+ protected void initHdfsFileSystem() {
+ // Glue uses S3, not HDFS. hdfs is already set to null in startUp().
+ }
+
+ /**
+ * Shadows parent's HDFS-based database initialization. The parent {@link
+ * org.apache.gravitino.spark.connector.integration.test.SparkCommonIT}
creates a database with an
+ * HDFS location ('/user/hive/{db}') which is wrong for Glue. This
implementation drops and
+ * recreates the database with the correct S3 location to remove any stale
tables left by prior
+ * test runs (e.g., when multiple Spark versions share the same Glue
catalog).
+ *
+ * <p>Called explicitly from {@link #startUp()}, not via JUnit lifecycle
inheritance. JUnit 5
+ * treats this protected method as hiding the package-private {@code
@BeforeAll
+ * initDefaultDatabase()} from {@code SparkCommonIT}, so the parent's
drop-and-recreate logic
+ * would not run — this override must perform the same cleanup itself.
+ */
+ protected void initDefaultDatabase() {
+ String defaultDbName = getDefaultDatabase();
+ String dbLocation = warehouse + "/" + defaultDbName;
+ sql("SET spark.sql.defaultCatalog=" + getCatalogName());
+ sql("DROP DATABASE IF EXISTS " + defaultDbName + " CASCADE");
+ deleteDirIfExists(dbLocation);
+ sql(String.format("CREATE DATABASE %s LOCATION '%s'", defaultDbName,
dbLocation));
+ sql("USE " + defaultDbName);
+ }
+
+ /**
+ * Overrides HDFS-based directory check. Glue uses S3 storage, not HDFS.
Uses S3A filesystem to
+ * verify directory existence on S3.
+ */
+ @Override
+ protected void checkDirExists(Path dir) {
+ try {
+ Configuration conf = newS3Config();
+ FileSystem fs = FileSystem.get(dir.toUri(), conf);
+ boolean exists = fs.exists(dir);
+ fs.close();
+ org.junit.jupiter.api.Assertions.assertTrue(exists, "S3 directory not
exists: " + dir);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Overrides HDFS-based file existence check. Glue uses S3 storage, not
HDFS. Uses S3A filesystem
+ * to verify that at least one data file exists in the partition directory.
+ */
+ @Override
+ protected void checkDataFileExists(Path dir) {
+ try {
+ Configuration conf = newS3Config();
+ FileSystem fs = FileSystem.get(dir.toUri(), conf);
+ FileStatus[] files = fs.listStatus(dir);
+ boolean hasFile = false;
+ for (FileStatus file : files) {
+ if (file.isFile()) {
+ hasFile = true;
+ break;
+ }
+ }
+ fs.close();
+ org.junit.jupiter.api.Assertions.assertTrue(hasFile, "No data file found
in: " + dir);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @AfterAll
+ protected void stop() throws IOException, InterruptedException {
+ if (sparkSession != null) {
+ sparkSession.close();
+ }
+ super.stopIntegrationTest();
+ }
+
+ /**
+ * Deletes a directory (file or directory) from S3 if it exists. Overrides
the HDFS-based
+ * implementation in SparkCommonIT since Glue uses S3 storage instead of
HDFS.
+ */
+ @Override
+ protected void deleteDirIfExists(String path) {
+ try {
+ Configuration conf = newS3Config();
+ Path dir = new Path(path);
+ FileSystem fs = FileSystem.get(dir.toUri(), conf);
+ if (fs.exists(dir)) {
+ fs.delete(dir, true);
+ }
+ fs.close();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /** Creates a Hadoop Configuration configured for S3A access with the test
credentials. */
+ private Configuration newS3Config() {
+ Configuration conf = new Configuration();
+ conf.set("fs.s3a.impl", "org.apache.hadoop.fs.s3a.S3AFileSystem");
+ conf.set("fs.s3a.access.key", s3AccessKey);
+ conf.set("fs.s3a.secret.key", s3SecretKey);
+ conf.set(
+ "fs.s3a.aws.credentials.provider",
"org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider");
+ if (s3Endpoint != null) {
+ conf.set("fs.s3a.endpoint", s3Endpoint);
+ conf.set("fs.s3a.path.style.access", "true");
+ conf.set("fs.s3a.connection.ssl.enabled", "false");
+ }
+ return conf;
+ }
+
+ private void initMetalakeAndCatalogs() {
+ client.createMetalake(metalakeName, "", java.util.Collections.emptyMap());
+ GravitinoMetalake metalake = client.loadMetalake(metalakeName);
+ metalake.createCatalog(
+ getCatalogName(),
+ org.apache.gravitino.Catalog.Type.RELATIONAL,
+ getProvider(),
+ "",
+ getCatalogConfigs());
Review Comment:
This method uses fully-qualified names (java.util.Collections.emptyMap and
org.apache.gravitino.Catalog.Type.RELATIONAL) even though there are no apparent
naming conflicts. Please add the corresponding imports and use simple names to
match the project's Java style rule of avoiding FQNs in method bodies.
##########
spark-connector/spark-common/src/main/java/org/apache/gravitino/spark/connector/catalog/BaseCatalog.java:
##########
@@ -328,11 +328,9 @@ public void renameTable(Identifier oldIdent, Identifier
newIdent)
newDatabase.equals(oldDatabase), "Doesn't support rename table to
different database");
org.apache.gravitino.rel.TableChange rename =
org.apache.gravitino.rel.TableChange.rename(newIdent.name());
+ NameIdentifier ident = NameIdentifier.of(getDatabase(oldIdent),
oldIdent.name());
Review Comment:
renameTable() no longer invalidates the underlying sparkCatalog cache
(invalidateTable(oldIdent)/invalidateTable(newIdent)), while
alterTable/dropTable/purgeTable all do. Without invalidation, Spark may keep
stale metadata for the old name (and/or miss the new name), causing subsequent
loadTable/tableExists behavior to be inconsistent after a rename. Consider
invalidating at least oldIdent (and newIdent if applicable) around the
Gravitino alterTable call.
##########
spark-connector/spark-common/src/test/java/org/apache/gravitino/spark/connector/integration/test/glue/SparkGlueCatalogIT.java:
##########
@@ -0,0 +1,620 @@
+/*
+ * 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.spark.connector.integration.test.glue;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.gravitino.catalog.glue.GlueConstants;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfo;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfo.SparkColumnInfo;
+import
org.apache.gravitino.spark.connector.integration.test.util.SparkTableInfoChecker;
+import org.apache.spark.sql.types.DataTypes;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Integration test for GravitinoGlueCatalog in Spark connector.
+ *
+ * <p>Tests mixed table type support (Hive format + Iceberg format) in a
single Glue database. Uses
+ * Moto server to mock AWS Glue API, similar to MotoGlueCatalogIT in the
server module.
+ */
+public abstract class SparkGlueCatalogIT extends SparkGlueEnvIT {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(SparkGlueCatalogIT.class);
+
+ private String glueEndpoint;
+ private String awsRegion = "us-east-1";
+ private String awsAccessKeyId = "test";
+ private String awsSecretAccessKey = "test";
+
+ @Override
+ protected String getCatalogName() {
+ return "glue";
+ }
+
+ @Override
+ protected String getProvider() {
+ return "glue";
+ }
+
+ @Override
+ protected Map<String, String> getCatalogConfigs() {
+ Map<String, String> catalogProperties = new java.util.HashMap<>();
+ catalogProperties.put(GlueConstants.AWS_REGION, awsRegion);
+ catalogProperties.put(GlueConstants.AWS_ACCESS_KEY_ID, awsAccessKeyId);
+ catalogProperties.put(GlueConstants.AWS_SECRET_ACCESS_KEY,
awsSecretAccessKey);
+ if (glueEndpoint != null) {
+ catalogProperties.put(GlueConstants.AWS_GLUE_ENDPOINT, glueEndpoint);
+ }
+ return catalogProperties;
+ }
+
+ @Override
+ protected boolean supportsSparkSQLClusteredBy() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsPartition() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsDelete() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsSchemaEvolution() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsReplaceColumns() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsSchemaAndTableProperties() {
+ return true;
+ }
+
+ @Override
+ protected boolean supportsComplexType() {
+ // Glue does not support Gravitino complex types (LIST, MAP, STRUCT) in
table columns.
+ return false;
+ }
+
+ @Override
+ protected boolean supportsUpdateColumnPosition() {
+ return false;
+ }
+
+ @Override
+ protected boolean supportsFunction() {
+ return false;
+ }
+
+ /**
+ * Sets the Glue endpoint for testing. Called by subclasses after Moto
container is started.
+ *
+ * @param endpoint the Glue API endpoint URL
+ */
+ protected void setGlueEndpoint(String endpoint) {
+ this.glueEndpoint = endpoint;
+ }
+
+ protected void setAwsRegion(String region) {
+ this.awsRegion = region;
+ }
+
+ /**
+ * Sets AWS credentials for testing.
+ *
+ * @param accessKeyId AWS access key ID
+ * @param secretAccessKey AWS secret access key
+ */
+ protected void setAwsCredentials(String accessKeyId, String secretAccessKey)
{
+ this.awsAccessKeyId = accessKeyId;
+ this.awsSecretAccessKey = secretAccessKey;
+ }
+
+ @Override
+ protected String getDefaultAwsRegion() {
+ return awsRegion;
+ }
+
+ @Override
+ protected String getGlueEndpoint() {
+ return glueEndpoint;
+ }
+
+ protected String getAwsRegion() {
+ return awsRegion;
+ }
+
+ /**
+ * Overrides to use CASCADE so that databases with stale tables (e.g., from
prior test runs) can
+ * be cleaned up. Glue tables persist across JVM restarts and may be left
behind after a crash.
+ */
+ @Override
+ protected void dropDatabaseIfExists(String database) {
+ sql("DROP DATABASE IF EXISTS " + database + " CASCADE");
+ }
+
+ /**
+ * Overrides to always use PARQUET format for Glue. Without an explicit
USING clause, Spark may
+ * route CREATE TABLE through the V1 Hive path, bypassing the
location-derivation logic in
+ * GravitinoGlueCatalog.createTable and leaving tables without a stored S3
location.
+ */
+ @Override
+ protected void createSimpleTable(String identifier) {
+ sql(getCreateSimpleTableString(identifier) + " USING PARQUET");
+ }
+
+ /**
+ * Overrides to recreate the database with the correct S3 location. The base
implementation uses
+ * CREATE DATABASE IF NOT EXISTS with a local HDFS path (/user/hive/db),
causing tables to inherit
+ * a local path as their default location. We drop and recreate to ensure
the S3 location is
+ * always set correctly, so stale data cleanup via dropTableIfExists works
reliably.
+ */
+ @Override
+ protected void createDatabaseIfNotExists(String database, String provider) {
+ String dbLocation = warehouse + "/" + database;
+ dropDatabaseIfExists(database);
+ // Delete S3 data directory so stale data files from prior runs don't
cause duplicate rows.
+ deleteDirIfExists(dbLocation);
+ sql(String.format("CREATE DATABASE %s LOCATION '%s'", database,
dbLocation));
+ }
+
+ /**
+ * Overrides to also delete the S3 data directory after dropping the table.
Unlike HDFS managed
+ * tables, Glue external tables do not delete S3 data on DROP TABLE. Stale
data files cause
+ * duplicate rows on the next create+insert cycle.
+ */
+ @Override
+ protected void dropTableIfExists(String tableName) {
+ String location = null;
+ try {
+ location = getTableInfo(tableName).getTableLocation();
+ } catch (Exception e) {
+ // Table may not exist yet — location stays null, nothing to delete.
+ LOG.debug("Could not get location for table {}: {}", tableName,
e.getMessage());
+ }
+ super.dropTableIfExists(tableName);
+ if (location != null) {
+ deleteDirIfExists(location);
+ }
+ }
+
+ /**
+ * Overrides base class: use USING PARQUET to ensure the table goes through
the Gravitino Glue
+ * catalog (V2 path).
+ */
+ @Test
+ @Override
+ protected void testDropAndWriteTable() {
+ String tableName = "drop_then_create_write_table";
+ dropTableIfExists(tableName);
+ sql(getCreateSimpleTableString(tableName) + " USING PARQUET");
+ SparkTableInfo info = getTableInfo(tableName);
+ checkTableReadWrite(info);
+
+ // External tables on S3 do not delete data on DROP TABLE; clean up
explicitly.
+ String location = info.getTableLocation();
+ dropTableIfExists(tableName);
+ if (location != null) {
+ deleteDirIfExists(location);
+ }
+
+ sql(getCreateSimpleTableString(tableName) + " USING PARQUET");
+ checkTableReadWrite(getTableInfo(tableName));
+ }
+
+ /**
+ * Overrides base class: skip this test due to a known issue where ALTER
TABLE RENAME fails for
+ * non-Iceberg (PARQUET) tables via the Glue catalog path. The rename
operation triggers
+ * tableExists() which calls tableCatalog.loadTable() →
GravitinoGlueCatalog.loadTable() →
+ * loadSparkTable() → HiveTableCatalog.loadTable(ident), but the table
lookup may fail because
+ * Derby and Glue are out of sync for renamed tables. This requires further
investigation into the
+ * HiveTableCatalog.loadTable() behavior and the renameTable dispatch chain
in
+ * RenameTableExec.apply().
+ */
+ @Test
+ @Override
+ protected void testRenameTable() {
+ // Skipped pending investigation. Rename logic in Glue backend works at
the REST API level
+ // (GlueCatalogOperations.alterTable), but the Spark catalog path for
non-Iceberg tables needs
+ // verification of how Derby/Hive metastore tracks renamed tables.
+ }
Review Comment:
This test is intentionally being skipped but will still report as a passing
test. Please use `@Disabled` (with a reason/link to an issue) or an explicit
Assumptions.assumeTrue(false, ...) so test reports clearly show it was skipped
rather than silently doing nothing.
--
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]
