diqiu50 commented on code in PR #14565:
URL: https://github.com/apache/gravitino/pull/14565#discussion_r4477766646
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -548,102 +615,250 @@ static TableIdentifier alterTable(
}
}
update.commit();
- LOG.info("Altered Iceberg table {}.{} properties via Iceberg SDK",
dbName, tableName);
}
+
+ transaction.commitTransaction();
+ LOG.info("Altered Iceberg table {}.{} via Iceberg SDK", dbName, tableName);
return TableIdentifier.of(dbName, tableName);
}
+ /**
+ * Refuses column changes that Iceberg cannot apply through this catalog,
before anything is
+ * staged, so that no change of a request is skipped without an error.
+ */
+ private static void checkColumnChange(TableChange.ColumnChange change) {
+ String[] fieldName = change.fieldName();
+ Preconditions.checkArgument(
+ fieldName != null && fieldName.length > 0, "Column change field name
cannot be empty");
+ String name = String.join(DOT, fieldName);
+ Preconditions.checkArgument(
+ fieldName.length == 1,
+ "Iceberg tables in Glue do not support changes to nested field %s",
+ name);
+ if (change instanceof TableChange.AddColumn) {
+ TableChange.AddColumn add = (TableChange.AddColumn) change;
+ Preconditions.checkArgument(
+ !add.isAutoIncrement(), "Iceberg does not support auto-increment
columns: %s", name);
+ Preconditions.checkArgument(
+ add.getDefaultValue() == null
+ || Column.DEFAULT_VALUE_NOT_SET.equals(add.getDefaultValue()),
+ "Iceberg tables in Glue do not support column default values: %s",
+ name);
+ return;
+ }
+ Preconditions.checkArgument(
+ change instanceof TableChange.UpdateColumnPosition
+ || change instanceof TableChange.DeleteColumn
+ || change instanceof TableChange.RenameColumn
+ || change instanceof TableChange.UpdateColumnType
+ || change instanceof TableChange.UpdateColumnComment
+ || change instanceof TableChange.UpdateColumnNullability,
+ "Iceberg tables in Glue do not support %s on column %s",
+ change.getClass().getSimpleName(),
+ name);
+ }
+
+ /**
+ * Moves a column to the requested position. The default position leaves an
added column at the
+ * end and moves an existing column after the last column.
+ */
+ private static void moveColumn(
Review Comment:
`UpdateColumnPosition` rejects the default position, so this case can't
happen. The branch was only reachable by re-adding a column, where it was
wrong, so I removed it.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -436,14 +447,20 @@ public GlueTable loadTable(NameIdentifier ident) throws
NoSuchTableException {
GlueTable table = GlueTable.fromGlueTable(rawGlueTable, typeConverter);
rejectDuplicateColumns(table, dbName);
- // Recover Iceberg-specific partitioning and sort orders from the
Iceberg metadata.
- // AWS Glue Table.partitionKeys() is empty for Iceberg tables, so we
load the Iceberg
- // Table to obtain the accurate partition spec and sort order.
+ // The Iceberg metadata is the only source of the columns, partitioning
and sort order of an
+ // Iceberg table; the Glue columns are a Hive-style copy. A table whose
metadata cannot be
+ // read is reported as an error rather than with the Glue copy.
if (GlueIcebergTableHelper.isIcebergTable(rawGlueTable)) {
try {
GlueIcebergTableHelper.loadTable(icebergGlueCatalog, dbName,
ident.name(), table);
Review Comment:
`dropTable` doesn't use `loadTable`, so these tables can be dropped; added a
test. Property changes need the metadata and fail.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -472,14 +489,28 @@ public GlueTable createTable(
String dbName = schemaName(ident.namespace());
Map<String, String> props = properties != null ? properties :
Collections.emptyMap();
- String tableFormat = props.getOrDefault(GlueConstants.TABLE_FORMAT,
defaultTableFormat);
- boolean isIceberg =
GlueConstants.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase(tableFormat);
- String normalizedFormat =
- isIceberg
- ? GlueConstants.ICEBERG_TABLE_TYPE_VALUE.toLowerCase(Locale.ROOT)
- : props
- .getOrDefault(GlueConstants.TABLE_FORMAT,
GlueConstants.DEFAULT_TABLE_FORMAT_VALUE)
- .toLowerCase(Locale.ROOT);
+ String tableFormat =
+ props.getOrDefault(
+ GlueConstants.TABLE_FORMAT,
+ defaultTableFormat != null
+ ? defaultTableFormat
+ : GlueConstants.DEFAULT_TABLE_FORMAT_VALUE);
+ Preconditions.checkArgument(
+ GlueConstants.TABLE_FORMAT_HIVE.equalsIgnoreCase(tableFormat)
+ ||
GlueConstants.TABLE_FORMAT_ICEBERG.equalsIgnoreCase(tableFormat),
+ "Unsupported value '%s' of '%s': use %s or %s",
Review Comment:
Validated on catalog create and alter; only `iceberg` and `hive` are
accepted.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueExceptionConverter.java:
##########
@@ -413,13 +416,55 @@ context, describe(t.getCause() != null ? t.getCause() :
t)),
// Not wrapped by Iceberg; it may be the table or its database.
return new NotFoundException(e, "%s: %s", context, describe(t));
}
+ if (t instanceof S3Exception) {
+ return toS3Exception((S3Exception) t, context, e);
+ }
+ if (t instanceof SdkClientException) {
+ // The failing client may be the Glue or the S3 client of the Iceberg
catalog.
+ SdkClientException clientException = (SdkClientException) t;
+ if (isCredentialFailure(clientException)) {
+ return toCredentialException(clientException, context);
+ }
+ return new ConnectionFailedException(
+ e,
+ "Failed to reach AWS (Glue or S3) for %s: %s",
+ context,
+ StringUtils.defaultIfBlank(
+ ExceptionMessages.usefulMessage(clientException),
+ clientException.getClass().getSimpleName()));
+ }
if (t instanceof SdkException) {
return toGenericException((SdkException) t, context, null);
}
}
return new RuntimeException(context + ": " + describe(e), e);
}
+ /**
+ * Converts an S3 failure met while reading or writing Iceberg metadata or
data files. The status
+ * decides the result, since S3 reports most failures under a few generic
error codes.
+ */
+ private static RuntimeException toS3Exception(
+ S3Exception s3Exception, String context, RuntimeException e) {
+ String detail = awsErrorDetail(s3Exception);
+ int status = s3Exception.statusCode();
+ if (status == 403) {
+ return new ForbiddenException(e, "S3 denied access for %s: %s", context,
detail);
+ }
+ if (status == 404) {
Review Comment:
The message already includes the table name.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergAwsClientFactory.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 java.util.HashMap;
+import java.util.Map;
+import org.apache.iceberg.aws.AwsClientFactories;
+import org.apache.iceberg.aws.AwsClientFactory;
+import org.apache.iceberg.aws.AwsProperties;
+import org.apache.iceberg.util.PropertyUtil;
+import software.amazon.awssdk.services.dynamodb.DynamoDbClient;
+import software.amazon.awssdk.services.glue.GlueClient;
+import software.amazon.awssdk.services.kms.KmsClient;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.S3Client;
+
+/**
+ * AWS client factory of the Iceberg Glue catalog used for Iceberg tables. The
Glue client is built
+ * the same way as the catalog's own Glue client, so the call timeout, attempt
timeout and maximum
+ * attempts of the catalog also apply to Iceberg tables. The other clients
come from the Iceberg
+ * default factory.
+ *
+ * <p>The catalog properties the Glue client needs are passed under {@link
#GLUE_CONFIG_PREFIX}.
+ */
+public class GlueIcebergAwsClientFactory implements AwsClientFactory {
+
+ /** Prefix of the Iceberg catalog properties that carry the Gravitino Glue
catalog properties. */
+ static final String GLUE_CONFIG_PREFIX = "gravitino.glue.";
+
+ private static final long serialVersionUID = 1L;
+
+ private HashMap<String, String> glueConfig;
Review Comment:
Checked, neither this code nor Iceberg logs the property map.
--
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]
