Copilot commented on code in PR #14561:
URL: https://github.com/apache/gravitino/pull/14561#discussion_r4451925369
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -420,19 +452,27 @@ public GlueTable createTable(
String tableFormat = props.getOrDefault(GlueConstants.TABLE_FORMAT,
defaultTableFormat);
boolean isIceberg =
GlueConstants.ICEBERG_TABLE_TYPE_VALUE.equalsIgnoreCase(tableFormat);
-
- // Non-Iceberg (Hive) tables only support identity partitioning.
- // Iceberg tables delegate to GlueIcebergCatalogHelper which supports all
transforms.
+ String normalizedFormat =
+ isIceberg
+ ? GlueConstants.ICEBERG_TABLE_TYPE_VALUE.toLowerCase(Locale.ROOT)
+ : props
+ .getOrDefault(GlueConstants.TABLE_FORMAT,
GlueConstants.DEFAULT_TABLE_FORMAT_VALUE)
+ .toLowerCase(Locale.ROOT);
+ // A table of an excluded format would be created but stay invisible
through this catalog.
+ Preconditions.checkArgument(
+ isFormatVisible(normalizedFormat),
+ "Cannot create table %s with format '%s': the catalog property '%s'
only exposes %s",
+ ident.name(),
+ normalizedFormat,
+ GlueConstants.TABLE_FORMAT_FILTER,
+ tableFormatFilter);
+
+ validateColumns(columns);
Review Comment:
Create determines the format from `table-format`, but load also recognizes
native `table_type=ICEBERG`. With a `hive`-only filter, supplying only that
native indicator passes this check and creates a table, then the read-back
rejects it as hidden. Reject an Iceberg indicator when the selected creation
mode is not Iceberg before writing to Glue.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -524,75 +550,291 @@ public GlueTable alterTable(NameIdentifier ident,
TableChange... changes)
}
rejectIfView(rawGlueTable, ident, dbName);
+ rejectIfFormatHidden(rawGlueTable, ident, dbName);
if (GlueIcebergTableHelper.isIcebergTable(rawGlueTable)) {
return alterIcebergTable(ident, dbName, rawGlueTable, changes);
}
- GlueTable current = GlueTable.fromGlueTable(rawGlueTable, typeConverter);
- current.initOpsContext(glueClient, catalogId, dbName);
-
- String newName = current.name();
- String newComment = current.comment();
- Map<String, String> newProps = new HashMap<>(current.properties());
+ return alterHiveTable(ident, dbName, rawGlueTable, changes);
+ }
- // Separate data columns from partition columns
- int partCount = current.partitioning().length;
- List<Column> allCols = new ArrayList<>(Arrays.asList(current.columns()));
- List<Column> dataCols = new ArrayList<>(allCols.subList(0, allCols.size()
- partCount));
- List<Column> partCols =
- new ArrayList<>(allCols.subList(allCols.size() - partCount,
allCols.size()));
+ /**
+ * Alters a Hive-format table. Glue UpdateTable replaces the whole table
definition, so the new
+ * definition starts from the table as Glue holds it and only the requested
parts change; fields
+ * Gravitino does not model, and column types that are not changed, are
written back as read. The
+ * version read is sent along, so Glue refuses the update if the table
changed in between.
+ */
+ private GlueTable alterHiveTable(
+ NameIdentifier ident, String dbName, Table raw, TableChange... changes) {
+ StorageDescriptor rawSd = raw.storageDescriptor();
+ HiveColumns cols =
+ new HiveColumns(
+ rawSd != null && rawSd.hasColumns() ? rawSd.columns() :
Collections.emptyList(),
+ raw.hasPartitionKeys() ? raw.partitionKeys() :
Collections.emptyList(),
+ rawSd != null && rawSd.hasBucketColumns()
+ ? rawSd.bucketColumns()
+ : Collections.emptyList(),
+ rawSd != null && rawSd.hasSortColumns()
+ ? rawSd.sortColumns()
Review Comment:
The column editor updates bucket and sort references but leaves
`skewedInfo().skewedColumnNames()` unchanged. Renaming `k` in the new
native-table fixture leaves a stale skew reference, and a skew-only column can
be deleted without removing that reference. Update skew-column names on rename
and reject or safely handle their deletion.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -524,75 +550,291 @@ public GlueTable alterTable(NameIdentifier ident,
TableChange... changes)
}
rejectIfView(rawGlueTable, ident, dbName);
+ rejectIfFormatHidden(rawGlueTable, ident, dbName);
if (GlueIcebergTableHelper.isIcebergTable(rawGlueTable)) {
return alterIcebergTable(ident, dbName, rawGlueTable, changes);
}
- GlueTable current = GlueTable.fromGlueTable(rawGlueTable, typeConverter);
- current.initOpsContext(glueClient, catalogId, dbName);
-
- String newName = current.name();
- String newComment = current.comment();
- Map<String, String> newProps = new HashMap<>(current.properties());
+ return alterHiveTable(ident, dbName, rawGlueTable, changes);
+ }
- // Separate data columns from partition columns
- int partCount = current.partitioning().length;
- List<Column> allCols = new ArrayList<>(Arrays.asList(current.columns()));
- List<Column> dataCols = new ArrayList<>(allCols.subList(0, allCols.size()
- partCount));
- List<Column> partCols =
- new ArrayList<>(allCols.subList(allCols.size() - partCount,
allCols.size()));
+ /**
+ * Alters a Hive-format table. Glue UpdateTable replaces the whole table
definition, so the new
+ * definition starts from the table as Glue holds it and only the requested
parts change; fields
+ * Gravitino does not model, and column types that are not changed, are
written back as read. The
+ * version read is sent along, so Glue refuses the update if the table
changed in between.
+ */
+ private GlueTable alterHiveTable(
+ NameIdentifier ident, String dbName, Table raw, TableChange... changes) {
+ StorageDescriptor rawSd = raw.storageDescriptor();
+ HiveColumns cols =
+ new HiveColumns(
+ rawSd != null && rawSd.hasColumns() ? rawSd.columns() :
Collections.emptyList(),
+ raw.hasPartitionKeys() ? raw.partitionKeys() :
Collections.emptyList(),
+ rawSd != null && rawSd.hasBucketColumns()
+ ? rawSd.bucketColumns()
+ : Collections.emptyList(),
+ rawSd != null && rawSd.hasSortColumns()
+ ? rawSd.sortColumns()
+ : Collections.emptyList());
+ String description = raw.description();
+ Map<String, String> params =
+ raw.hasParameters() ? new HashMap<>(raw.parameters()) : new
HashMap<>();
+ SerDeInfo rawSerde = rawSd != null ? rawSd.serdeInfo() : null;
+ String location = rawSd != null ? rawSd.location() : null;
+ String serdeName = rawSerde != null ? rawSerde.name() : null;
+ Map<String, String> serdeParams =
+ rawSerde != null && rawSerde.hasParameters()
+ ? new HashMap<>(rawSerde.parameters())
+ : new HashMap<>();
+ boolean storageChanged = false;
+ boolean changed = false;
for (TableChange change : changes) {
if (change instanceof TableChange.RenameTable) {
throw new UnsupportedOperationException("Glue does not support table
rename");
} else if (change instanceof TableChange.UpdateComment) {
- newComment = ((TableChange.UpdateComment) change).getNewComment();
+ description = ((TableChange.UpdateComment) change).getNewComment();
+ changed = true;
} else if (change instanceof TableChange.SetProperty) {
+ // Storage properties map to storage descriptor fields, the rest to
table parameters.
TableChange.SetProperty sp = (TableChange.SetProperty) change;
- newProps.put(sp.getProperty(), sp.getValue());
+ String property = sp.getProperty();
+ if (GlueConstants.LOCATION.equals(property)) {
+ location = sp.getValue();
+ storageChanged = true;
+ } else if (GlueConstants.SERDE_NAME.equals(property)) {
+ serdeName = sp.getValue();
+ storageChanged = true;
+ } else if (property.startsWith(GlueConstants.SERDE_PARAMETER_PREFIX)) {
+ serdeParams.put(
+
property.substring(GlueConstants.SERDE_PARAMETER_PREFIX.length()),
sp.getValue());
+ storageChanged = true;
+ } else {
+ checkTableParameterKey(property);
+ params.put(property, sp.getValue());
+ }
+ changed = true;
} else if (change instanceof TableChange.RemoveProperty) {
- newProps.remove(((TableChange.RemoveProperty) change).getProperty());
+ String property = ((TableChange.RemoveProperty) change).getProperty();
+ if (GlueConstants.LOCATION.equals(property)) {
+ storageChanged |= location != null;
+ location = null;
+ } else if (GlueConstants.SERDE_NAME.equals(property)) {
+ storageChanged |= serdeName != null;
+ serdeName = null;
+ } else if (property.startsWith(GlueConstants.SERDE_PARAMETER_PREFIX)) {
+ storageChanged |=
+
serdeParams.remove(property.substring(GlueConstants.SERDE_PARAMETER_PREFIX.length()))
+ != null;
+ } else {
+ checkTableParameterKey(property);
+ changed |= params.remove(property) != null;
+ }
+ changed |= storageChanged;
Review Comment:
Removing a storage property clears its descriptor value but leaves any
same-named entry in `Table.parameters()`, which `GlueTable.fromGlueTable`
exposes as a fallback. The removed value therefore remains visible, and a
parameter-only value can even make the removal a no-op. Remove the parameter
shadow too for `location`, `serde-name`, and `serde.parameter.*`.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -483,27 +527,9 @@ public GlueTable createTable(
executeCreateTable(dbName, ident, req);
LOG.info("Created {} table {}.{}", isIceberg ? "Iceberg" : "Glue", dbName,
ident.name());
- if (isIceberg) {
- // Load from Glue to pick up any server-set parameters (e.g.
current-schema-id).
- return loadTable(ident);
- }
- GlueTable created =
- GlueTable.builder()
- .withName(ident.name())
- .withComment(comment)
- .withColumns(columns)
- .withProperties(props)
- .withPartitioning(partitions)
- .withDistribution(distribution != null ? distribution :
Distributions.NONE)
- .withSortOrders(sortOrders != null ? sortOrders : new SortOrder[0])
- .withAuditInfo(
- AuditInfo.builder()
- .withCreator(PrincipalUtils.getCurrentUserName())
- .withCreateTime(Instant.now())
- .build())
- .build();
- created.initOpsContext(glueClient, catalogId, dbName);
- return created;
+ // Read back what Glue stored, so the response carries the derived
location and storage
+ // classes, as a later load does.
+ return loadTable(ident);
Review Comment:
The table has already been created when this read-back runs. If `GetTable`
is throttled, temporarily unavailable, or denied by IAM, create now returns an
error even though the table exists; the core dispatcher consequently never
records its Gravitino metadata, and retrying gets `TableAlreadyExists`. Return
a response built from the submitted input, or otherwise handle a failed read
without reporting the committed create as failed.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -725,7 +1091,25 @@ private boolean matchesFormatFilter(Table table) {
}
String normalized =
fmt != null ? fmt.toLowerCase(Locale.ROOT) :
GlueConstants.DEFAULT_TABLE_FORMAT_VALUE;
- return tableFormatFilter.contains(normalized);
+ return isFormatVisible(normalized);
Review Comment:
The filter falls back to `hive` for any native table without the
Gravitino-specific `table-format` property, even when its native metadata
identifies another format (for example `spark.sql.sources.provider=delta`).
Consequently a `hive`-only catalog exposes and can now alter or drop an
excluded Delta table, while a `delta`-only catalog hides it. Classify native
format indicators before applying the filter, and use that classification
consistently for all operations.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -872,11 +1250,69 @@ private TableInput buildTableInput(
private software.amazon.awssdk.services.glue.model.Column
toGlueColumn(Column col) {
return software.amazon.awssdk.services.glue.model.Column.builder()
.name(col.name())
- .type(typeConverter.fromGravitino(col.dataType()))
+ .type(toGlueType(col.name(), col.dataType()))
.comment(col.comment())
.build();
}
+ private String toGlueType(String columnName, Type type) {
+ try {
+ return typeConverter.fromGravitino(type);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(
+ String.format("Column %s: %s", columnName, e.getMessage()), e);
+ }
+ }
+
+ /**
+ * Resolves the storage classes of a Hive-format table. A {@code format}
sets all three classes,
+ * and any class given next to it must match. Without a format, either all
three classes are
+ * given, or none and the table is a text table, as in Hive and Athena.
+ *
+ * @return the storage format to apply, or null when all three classes are
given explicitly
+ */
+ private static StorageFormat resolveStorageFormat(
+ String format, String inputFormat, String outputFormat, String serdeLib)
{
+ if (format == null) {
+ boolean anyClass = inputFormat != null || outputFormat != null ||
serdeLib != null;
+ if (!anyClass) {
+ return StorageFormat.TEXTFILE;
+ }
+ Preconditions.checkArgument(
+ inputFormat != null && outputFormat != null && serdeLib != null,
Review Comment:
Checking only for null accepts empty or whitespace-only class names, so
three blank properties pass validation and are written as the table's input
format, output format and SerDe. Reject blank class names as well; they cannot
describe a usable storage format.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -872,11 +1250,69 @@ private TableInput buildTableInput(
private software.amazon.awssdk.services.glue.model.Column
toGlueColumn(Column col) {
return software.amazon.awssdk.services.glue.model.Column.builder()
.name(col.name())
- .type(typeConverter.fromGravitino(col.dataType()))
+ .type(toGlueType(col.name(), col.dataType()))
.comment(col.comment())
.build();
}
+ private String toGlueType(String columnName, Type type) {
+ try {
+ return typeConverter.fromGravitino(type);
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException(
+ String.format("Column %s: %s", columnName, e.getMessage()), e);
+ }
+ }
+
+ /**
+ * Resolves the storage classes of a Hive-format table. A {@code format}
sets all three classes,
+ * and any class given next to it must match. Without a format, either all
three classes are
+ * given, or none and the table is a text table, as in Hive and Athena.
+ *
+ * @return the storage format to apply, or null when all three classes are
given explicitly
+ */
+ private static StorageFormat resolveStorageFormat(
+ String format, String inputFormat, String outputFormat, String serdeLib)
{
+ if (format == null) {
+ boolean anyClass = inputFormat != null || outputFormat != null ||
serdeLib != null;
+ if (!anyClass) {
+ return StorageFormat.TEXTFILE;
+ }
+ Preconditions.checkArgument(
+ inputFormat != null && outputFormat != null && serdeLib != null,
Review Comment:
Without `format`, this accepts incompatible known classes, such as an ORC
input class combined with a Parquet output class and text SerDe. The resulting
table cannot be read consistently. Reject incompatible built-in class
combinations while retaining a defined policy for custom storage classes.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTableOperations.java:
##########
@@ -152,21 +164,54 @@ public Partition addPartition(Partition partition) throws
PartitionAlreadyExists
partition instanceof IdentityPartition, "Glue only supports identity
partitions");
IdentityPartition ip = (IdentityPartition) partition;
Preconditions.checkArgument(
- ip.values().length == partitionColNames.length,
- "Partition values count (%s) must match partition columns count (%s)",
- ip.values().length,
- partitionColNames.length);
+ ip.values().length == partitionColNames.length
+ && ip.fieldNames().length == ip.values().length,
+ "Partition must give one value for each partition column %s",
+ Arrays.toString(partitionColNames));
- List<String> values = new ArrayList<>(ip.values().length);
- for (Literal<?> v : ip.values()) {
- values.add(v.value() != null ? v.value().toString() : null);
+ // Glue stores values by position, so place each value under the column
its field name names.
+ String[] values = new String[partitionColNames.length];
+ for (int i = 0; i < ip.values().length; i++) {
+ String[] fieldName = ip.fieldNames()[i];
+ int column = fieldName.length == 1 ? partitionColumnIndex(fieldName[0])
: -1;
+ Preconditions.checkArgument(
+ column >= 0,
+ "Partition field %s is not a partition column of table %s; partition
columns: %s",
+ String.join(".", fieldName),
+ tableName,
+ Arrays.toString(partitionColNames));
+ Preconditions.checkArgument(
+ values[column] == null, "Partition column %s is given twice",
partitionColNames[column]);
+ Object value = ip.values()[i].value();
+ // Glue limits partition values to 1024 characters. Null and empty
values are rejected because
+ // a Hive-style partition name cannot represent them.
+ Preconditions.checkArgument(
+ value != null && !value.toString().isEmpty(),
+ "Value of partition column %s must not be null or empty",
+ partitionColNames[column]);
+ Preconditions.checkArgument(
+ value.toString().length() <= MAX_PARTITION_VALUE_LENGTH,
+ "Value of partition column %s exceeds the Glue limit of %s
characters",
+ partitionColNames[column],
+ MAX_PARTITION_VALUE_LENGTH);
+ values[column] = value.toString();
}
+ List<String> valueList = Arrays.asList(values);
+ String name = buildPartitionName(valueList);
+ // A given name may be written with or without Hive escaping, but must
match the values.
+ Preconditions.checkArgument(
+ partition.name() == null
+ || partition.name().equals(name)
+ || partition.name().equals(unescapedPartitionName(valueList)),
Review Comment:
This comparison makes partition column names case-sensitive when a name is
supplied: for a `dt` partition, `DT=2024-01-01` is rejected even though `DT` is
accepted as a field name and `getPartition` compares its key
case-insensitively. Compare the key segments without case while keeping value
comparisons case-sensitive, for both escaped and unescaped names.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTableOperations.java:
##########
@@ -180,14 +225,14 @@ public Partition addPartition(Partition partition) throws
PartitionAlreadyExists
glueClient.createPartition(req.build());
} catch (AlreadyExistsException e) {
throw new PartitionAlreadyExistsException(
- e, "Partition %s already exists in table %s", partition.name(),
tableName);
- } catch (GlueException e) {
- throw ExceptionMessages.wrap("Failed to add partition " +
partition.name(), e);
+ e, "Partition %s already exists in table %s", name, tableName);
+ } catch (SdkException e) {
+ throw GlueExceptionConverter.toGenericException(
+ e, "adding partition " + name + " to table " + tableName,
glueClient);
}
- LOG.info("Added partition {} to {}.{}", partition.name(), dbName,
tableName);
- return Partitions.identity(
- partition.name(), ip.fieldNames(), ip.values(),
partition.properties());
+ LOG.info("Added partition {} to {}.{}", name, dbName, tableName);
+ return getPartition(name);
Review Comment:
`CreatePartition` has already committed when this read runs, so a denied or
failed `GetPartition` reports failure for a partition that now exists. The read
also converts supplied integer or date literals into string literals. Return
the canonical name and ordered, typed values from the validated request instead
of requiring another AWS call.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTypeConverter.java:
##########
@@ -243,7 +258,18 @@ public String fromGravitino(Type type) {
if (type instanceof Types.ExternalType) {
return ((Types.ExternalType) type).catalogString();
}
- throw new IllegalArgumentException("Unsupported Gravitino type for Glue: "
+ type);
+ throw new IllegalArgumentException(
+ "Unsupported Gravitino type for Glue: " + type.simpleString());
+ }
+
+ private static String checkStructFieldName(String name) {
+ // Glue type strings have no quoting, so these characters would change how
the type parses.
+ Preconditions.checkArgument(
+ StringUtils.isNotBlank(name) && !StringUtils.containsAny(name, ":,<>"),
+ "Struct field name '%s' is not supported by Glue: it must not be blank
or contain ':', ',',"
+ + " '<' or '>'",
Review Comment:
Parentheses and leading/trailing whitespace also break round trips through
this converter: `splitTopLevel` treats parentheses as nesting delimiters, and
the struct parser trims field names. For example, fields named `a(` and `b`
serialize into one parsed field instead of two. Reject those names before
writing the type string.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -524,75 +550,291 @@ public GlueTable alterTable(NameIdentifier ident,
TableChange... changes)
}
rejectIfView(rawGlueTable, ident, dbName);
+ rejectIfFormatHidden(rawGlueTable, ident, dbName);
if (GlueIcebergTableHelper.isIcebergTable(rawGlueTable)) {
return alterIcebergTable(ident, dbName, rawGlueTable, changes);
}
- GlueTable current = GlueTable.fromGlueTable(rawGlueTable, typeConverter);
- current.initOpsContext(glueClient, catalogId, dbName);
-
- String newName = current.name();
- String newComment = current.comment();
- Map<String, String> newProps = new HashMap<>(current.properties());
+ return alterHiveTable(ident, dbName, rawGlueTable, changes);
+ }
- // Separate data columns from partition columns
- int partCount = current.partitioning().length;
- List<Column> allCols = new ArrayList<>(Arrays.asList(current.columns()));
- List<Column> dataCols = new ArrayList<>(allCols.subList(0, allCols.size()
- partCount));
- List<Column> partCols =
- new ArrayList<>(allCols.subList(allCols.size() - partCount,
allCols.size()));
+ /**
+ * Alters a Hive-format table. Glue UpdateTable replaces the whole table
definition, so the new
+ * definition starts from the table as Glue holds it and only the requested
parts change; fields
+ * Gravitino does not model, and column types that are not changed, are
written back as read. The
+ * version read is sent along, so Glue refuses the update if the table
changed in between.
+ */
+ private GlueTable alterHiveTable(
+ NameIdentifier ident, String dbName, Table raw, TableChange... changes) {
+ StorageDescriptor rawSd = raw.storageDescriptor();
+ HiveColumns cols =
+ new HiveColumns(
+ rawSd != null && rawSd.hasColumns() ? rawSd.columns() :
Collections.emptyList(),
+ raw.hasPartitionKeys() ? raw.partitionKeys() :
Collections.emptyList(),
+ rawSd != null && rawSd.hasBucketColumns()
+ ? rawSd.bucketColumns()
+ : Collections.emptyList(),
+ rawSd != null && rawSd.hasSortColumns()
+ ? rawSd.sortColumns()
+ : Collections.emptyList());
+ String description = raw.description();
+ Map<String, String> params =
+ raw.hasParameters() ? new HashMap<>(raw.parameters()) : new
HashMap<>();
+ SerDeInfo rawSerde = rawSd != null ? rawSd.serdeInfo() : null;
+ String location = rawSd != null ? rawSd.location() : null;
+ String serdeName = rawSerde != null ? rawSerde.name() : null;
+ Map<String, String> serdeParams =
+ rawSerde != null && rawSerde.hasParameters()
+ ? new HashMap<>(rawSerde.parameters())
+ : new HashMap<>();
+ boolean storageChanged = false;
+ boolean changed = false;
for (TableChange change : changes) {
if (change instanceof TableChange.RenameTable) {
throw new UnsupportedOperationException("Glue does not support table
rename");
} else if (change instanceof TableChange.UpdateComment) {
- newComment = ((TableChange.UpdateComment) change).getNewComment();
+ description = ((TableChange.UpdateComment) change).getNewComment();
+ changed = true;
} else if (change instanceof TableChange.SetProperty) {
+ // Storage properties map to storage descriptor fields, the rest to
table parameters.
TableChange.SetProperty sp = (TableChange.SetProperty) change;
- newProps.put(sp.getProperty(), sp.getValue());
+ String property = sp.getProperty();
+ if (GlueConstants.LOCATION.equals(property)) {
+ location = sp.getValue();
+ storageChanged = true;
+ } else if (GlueConstants.SERDE_NAME.equals(property)) {
+ serdeName = sp.getValue();
+ storageChanged = true;
+ } else if (property.startsWith(GlueConstants.SERDE_PARAMETER_PREFIX)) {
+ serdeParams.put(
+
property.substring(GlueConstants.SERDE_PARAMETER_PREFIX.length()),
sp.getValue());
+ storageChanged = true;
+ } else {
+ checkTableParameterKey(property);
+ params.put(property, sp.getValue());
+ }
+ changed = true;
} else if (change instanceof TableChange.RemoveProperty) {
- newProps.remove(((TableChange.RemoveProperty) change).getProperty());
+ String property = ((TableChange.RemoveProperty) change).getProperty();
+ if (GlueConstants.LOCATION.equals(property)) {
+ storageChanged |= location != null;
+ location = null;
+ } else if (GlueConstants.SERDE_NAME.equals(property)) {
+ storageChanged |= serdeName != null;
+ serdeName = null;
+ } else if (property.startsWith(GlueConstants.SERDE_PARAMETER_PREFIX)) {
+ storageChanged |=
+
serdeParams.remove(property.substring(GlueConstants.SERDE_PARAMETER_PREFIX.length()))
+ != null;
+ } else {
+ checkTableParameterKey(property);
+ changed |= params.remove(property) != null;
+ }
+ changed |= storageChanged;
} else if (change instanceof TableChange.ColumnChange) {
- applyColumnChange(dataCols, partCols, (TableChange.ColumnChange)
change);
+ changed |= cols.apply((TableChange.ColumnChange) change);
} else {
throw new IllegalArgumentException(
"Unsupported table change: " + change.getClass().getSimpleName());
}
}
- List<Column> newAllCols = new ArrayList<>(dataCols);
- newAllCols.addAll(partCols);
- Column[] newColumns = newAllCols.toArray(new Column[0]);
+ if (!changed) {
+ return loadTable(ident);
+ }
+
+ StorageDescriptor sd = rawSd;
+ if (rawSd != null || !cols.dataCols.isEmpty() || storageChanged) {
+ StorageDescriptor.Builder sdBuilder =
+ rawSd != null ? rawSd.toBuilder() : StorageDescriptor.builder();
+
sdBuilder.columns(cols.dataCols).bucketColumns(cols.bucketCols).sortColumns(cols.sortCols);
+ if (storageChanged) {
+ SerDeInfo.Builder serdeBuilder =
+ rawSerde != null ? rawSerde.toBuilder() : SerDeInfo.builder();
+ sdBuilder
+ .location(location)
+
.serdeInfo(serdeBuilder.name(serdeName).parameters(serdeParams).build());
+ }
+ sd = sdBuilder.build();
+ }
TableInput input =
- buildTableInput(
- dbName,
- newName,
- newComment,
- newColumns,
- newProps,
- current.partitioning(),
- current.distribution(),
- current.sortOrder(),
- false);
+ TableInput.builder()
+ .name(raw.name())
+ .description(description)
+ .owner(raw.owner())
+ .lastAccessTime(raw.lastAccessTime())
+ .lastAnalyzedTime(raw.lastAnalyzedTime())
+ .retention(raw.retention())
+ .storageDescriptor(sd)
+ .partitionKeys(cols.partCols)
+ .viewOriginalText(raw.viewOriginalText())
+ .viewExpandedText(raw.viewExpandedText())
+ .tableType(raw.tableType())
+ .parameters(params)
+ .targetTable(raw.targetTable())
+ .build();
- executeUpdateTable(ident,
UpdateTableRequest.builder().databaseName(dbName).tableInput(input));
+ executeUpdateTable(
Review Comment:
`table-format` is mutable, so with `table-format-filter=hive` an alter
setting it to `iceberg` is committed, but the subsequent `loadTable` rejects
the now-hidden table. The caller receives a not-found error after a successful
mutation and cannot undo it through this catalog. Validate the resulting format
against the filter before sending `UpdateTable`.
--
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]
