Copilot commented on code in PR #11073:
URL: https://github.com/apache/gravitino/pull/11073#discussion_r3286730773
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -123,6 +121,9 @@ public class GlueCatalogOperations implements
CatalogOperations, SupportsSchemas
@VisibleForTesting String defaultTableFormat;
+ /** Optional S3 warehouse prefix. Table location is derived as {@code
warehouse/db/table}. */
+ @VisibleForTesting String warehouseLocation;
+
Review Comment:
The field comment says the warehouse prefix is "Optional", but
`initialize()` always calls `GlueIcebergTableHelper.createGlueCatalog(config)`
which now requires `GlueConstants.WAREHOUSE` to be set. Either update the
comment to reflect that the warehouse is required, or make Iceberg catalog
initialization conditional so the warehouse can truly be optional.
##########
trino-connector/integration-test/src/test/java/org/apache/gravitino/trino/connector/integration/test/TrinoQueryIT.java:
##########
@@ -480,18 +490,9 @@ void executeSqlFileWithGenOutput(
}
firstLine = false;
- String sql = sqlMatcher.group(1);
+ String sql = resolveParameters(sqlMatcher.group(1));
String result = queryRunner.runQuery(sql).trim();
LOG.info("Execute sql:\n{}\nResult:\n{}", sql, result);
- if (isQueryFailed(result)) {
- throw new RuntimeException(
- "Failed to execute sql in the test set. "
- + simpleTesterName(path)
- + ":\n"
- + sql
- + "\nresult:\n"
- + result);
- }
outputStream.write(result.getBytes(StandardCharsets.UTF_8));
outputStream.write("\n".getBytes(StandardCharsets.UTF_8));
Review Comment:
`executeSqlFileWithGenOutput` no longer fails when a query returns a "Query
... failed:" result; it will still write the failure text into the generated
golden output file. This makes it easy to accidentally generate/commit outputs
that represent failures rather than correct results. Consider restoring the
failure check (or gating it behind an explicit flag) so `--gen_output` stops on
query failures.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -199,6 +209,23 @@ static void loadTable(Catalog icebergCatalog, String
dbName, String tableName, G
mergedProps.putAll(icebergTable.properties());
table.setProperties(mergedProps);
+ // Overwrite Glue's Hive-style column types with the accurate types from
the Iceberg schema.
+ // Glue stores Iceberg TIME as "string", REAL as "float", etc., so the
Iceberg schema is the
+ // authoritative source for column types on Iceberg tables.
+ Schema icebergSchema = icebergTable.schema();
+ Column[] columns =
Review Comment:
`loadTable` now overwrites Glue-derived column types with types converted
from the Iceberg schema. This is a behavior change that can impact clients
relying on Glue-reported types, but there are no unit tests asserting the new
column/type reconciliation behavior (e.g., TIME mapped correctly). Adding a
focused test around this overwrite would reduce regression risk.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueConnectorAdapter.java:
##########
@@ -0,0 +1,127 @@
+/*
+ * 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 io.trino.spi.session.PropertyMetadata;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.trino.connector.catalog.CatalogConnectorAdapter;
+import
org.apache.gravitino.trino.connector.catalog.CatalogConnectorMetadataAdapter;
+import org.apache.gravitino.trino.connector.catalog.HasPropertyMeta;
+import org.apache.gravitino.trino.connector.catalog.hive.HivePropertyMeta;
+import org.apache.gravitino.trino.connector.metadata.GravitinoCatalog;
+
+/**
+ * Transforming Apache Gravitino Glue catalog configuration and components
into Apache Gravitino
+ * connector.
+ */
+public class GlueConnectorAdapter implements CatalogConnectorAdapter {
+
+ private static final String CONNECTOR_LAKEHOUSE = "lakehouse";
+
+ // Gravitino catalog property keys for AWS Glue
+ private static final String PROP_AWS_REGION = "aws-region";
+ private static final String PROP_AWS_GLUE_CATALOG_ID = "aws-glue-catalog-id";
+ private static final String PROP_AWS_ACCESS_KEY_ID = "aws-access-key-id";
+ private static final String PROP_AWS_SECRET_ACCESS_KEY =
"aws-secret-access-key";
+ private static final String PROP_AWS_GLUE_ENDPOINT = "aws-glue-endpoint";
+
+ // Trino Hive connector configuration keys for Glue
+ private static final String HIVE_METASTORE = "hive.metastore";
+ private static final String HIVE_METASTORE_GLUE_REGION =
"hive.metastore.glue.region";
+ private static final String HIVE_METASTORE_GLUE_CATALOG_ID =
"hive.metastore.glue.catalogid";
+ private static final String HIVE_METASTORE_GLUE_ACCESS_KEY =
"hive.metastore.glue.aws-access-key";
+ private static final String HIVE_METASTORE_GLUE_SECRET_KEY =
"hive.metastore.glue.aws-secret-key";
+ private static final String HIVE_METASTORE_GLUE_ENDPOINT =
"hive.metastore.glue.endpoint-url";
+ private static final String HIVE_S3_ACCESS_KEY = "hive.s3.aws-access-key";
+ private static final String HIVE_S3_SECRET_KEY = "hive.s3.aws-secret-key";
+
+ private final HasPropertyMeta propertyMetadata;
+
+ /** Constructs a new GlueConnectorAdapter. */
+ public GlueConnectorAdapter() {
+ this.propertyMetadata = new HivePropertyMeta();
+ }
+
+ @Override
+ public Map<String, String> buildInternalConnectorConfig(GravitinoCatalog
catalog)
+ throws Exception {
+ Map<String, String> config = new HashMap<>();
+
+ // Glue-specific metastore configuration.
+ config.put(HIVE_METASTORE, "glue");
+ config.put(HIVE_METASTORE_GLUE_REGION,
catalog.getRequiredProperty(PROP_AWS_REGION));
+
+ // Passed through to the underlying Trino Hive/Iceberg connector.
Review Comment:
`GlueConnectorAdapter` adds new config mapping behavior, but there are no
unit tests validating the produced connector config (key names,
required/optional properties, endpoint/catalog ID wiring, credential
propagation). Other connector adapters in this module have
`buildInternalConnectorConfig` tests; adding similar coverage here would help
prevent regressions.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -207,6 +234,74 @@ static void loadTable(Catalog icebergCatalog, String
dbName, String tableName, G
}
}
+ /**
+ * Converts an Iceberg type to the equivalent Gravitino type.
+ *
+ * <p>TIME and TIMESTAMP types are always returned with microsecond (6)
precision, matching
+ * Iceberg's internal representation.
+ */
+ // TODO: the Iceberg-to-Gravitino conversions in this class (type mapping,
partition spec,
+ // sort order, etc.) duplicate logic in catalog-lakehouse-iceberg. Consider
extracting them
+ // to a shared layer (e.g. catalog-common) so both catalogs can reuse the
code.
+ // The parameter uses FQN because org.apache.iceberg.types.Type and
+ // org.apache.gravitino.rel.types.Type share the same simple name.
+ static Type fromIcebergType(org.apache.iceberg.types.Type icebergType) {
+ switch (icebergType.typeId()) {
+ case BOOLEAN:
+ return BooleanType.get();
+ case INTEGER:
Review Comment:
`fromIcebergType(...)` introduces a comprehensive Iceberg→Gravitino type
mapping (including precision rules and nested LIST/MAP/STRUCT). There is
currently no unit test coverage validating these mappings for key Iceberg types
(especially TIME, TIMESTAMP/TZ, DECIMAL, nested types), which makes regressions
hard to catch. Consider adding targeted tests for representative primitive +
nested types.
##########
catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/integration/test/AwsGlueCatalogIT.java:
##########
@@ -62,15 +62,13 @@ class AwsGlueCatalogIT extends AbstractGlueCatalogIT {
@BeforeAll
void checkAwsEnvironment() {
- Assumptions.assumeTrue(
- System.getenv("AWS_SECRET_ACCESS_KEY") != null,
- "Skipped: AWS_SECRET_ACCESS_KEY is not set");
+ Preconditions.checkState(
+ System.getenv("AWS_SECRET_ACCESS_KEY") != null, "AWS_SECRET_ACCESS_KEY
must be set");
+ Preconditions.checkState(
+ System.getenv("AWS_S3_TEST_BUCKET") != null, "AWS_S3_TEST_BUCKET must
be set");
Review Comment:
This integration test is documented as "skipped by default", but
`checkAwsEnvironment()` now uses `Preconditions.checkState(...)` which
hard-fails whenever `AWS_ACCESS_KEY_ID` is set but other env vars are missing.
To avoid unexpected failures in partially-configured environments, consider
gating the test on all required env vars (e.g., additional
`@EnabledIfEnvironmentVariable`s) or using JUnit assumptions to skip when
missing.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/glue/GlueDataTypeTransformer.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * 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 io.trino.spi.type.TimeType;
+import io.trino.spi.type.TimestampType;
+import io.trino.spi.type.TimestampWithTimeZoneType;
+import org.apache.gravitino.rel.types.Type;
+import org.apache.gravitino.rel.types.Type.Name;
+import org.apache.gravitino.rel.types.Types;
+import
org.apache.gravitino.trino.connector.catalog.hive.HiveDataTypeTransformer;
+
+/**
+ * Type transformer for the Glue catalog. Normalizes Trino TIME and TIMESTAMP
types to microsecond
+ * precision (6) before sending to Gravitino, matching Iceberg's type system
requirements.
+ */
+public class GlueDataTypeTransformer extends HiveDataTypeTransformer {
+
+ @Override
+ public Type getGravitinoType(io.trino.spi.type.Type type) {
+ Class<? extends io.trino.spi.type.Type> typeClass = type.getClass();
+ if (TimeType.class.isAssignableFrom(typeClass)) {
+ // Iceberg only supports microsecond (6) precision for time.
Review Comment:
`GlueDataTypeTransformer` changes type mapping semantics (TIME/TIMESTAMP
precision normalization and TIMESTAMP WITH TIME ZONE handling), but there are
no focused unit tests covering these conversions. There are existing tests for
`HiveDataTypeTransformer`/`IcebergDataTypeTransformer`; adding analogous tests
for Glue would make these mappings safer to evolve.
--
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]
