diqiu50 commented on code in PR #14561:
URL: https://github.com/apache/gravitino/pull/14561#discussion_r4465693441
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogCapability.java:
##########
@@ -65,7 +65,8 @@ public CapabilityResult caseSensitiveOnName(Scope scope) {
switch (scope) {
case SCHEMA:
case TABLE:
- // Glue folds database/table names to lowercase on storage.
+ case COLUMN:
Review Comment:
Agreed, fixed in f6685bc5d and 51d7332f9. `COLUMN` is case-sensitive again,
so Iceberg column names stay as given. Hive-format column names are now folded
in the Glue create and alter paths, including bucket and sort references.
Folding in the catalog would leave the stored column entity with the
requested name, so a rename to a mixed-case name would lose the column id and
its tags. To avoid that, core now stores the name the catalog reports on create
and points rename targets at it (51d7332f9).
One refinement after checking on real AWS: Glue does lowercase its copy of
Iceberg columns (`eventTime` becomes `eventtime` in
`StorageDescriptor.Columns`), while the Iceberg metadata keeps `eventTime`.
Gravitino loads Iceberg columns from the metadata, so your conclusion holds,
and `docs/aws-glue-catalog.md` now describes both.
Columns that differ only by case stay refused, since Glue would hold them as
duplicates: `id` and `ID` are stored as `id`, `id`.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueIcebergTableHelper.java:
##########
@@ -477,7 +477,9 @@ static TableIdentifier alterTable(
}
if (hasSchemaChange) {
- UpdateSchema update = table.updateSchema();
+ // Gravitino folds column names in requests to lower case, as Glue does,
so look up existing
Review Comment:
Fixed in f6685bc5d: this is back to `table.updateSchema()`, so Iceberg
columns are matched exactly.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTableOperations.java:
##########
@@ -235,20 +271,22 @@ public boolean dropPartition(String partitionName) {
/**
* Builds a Hive-style partition name (e.g. {@code
dt=2024-01-01/country=us}) from an ordered list
- * of Glue partition values.
+ * of Glue partition values. Values are escaped as Hive does, so the name
parses back to them.
*/
private String buildPartitionName(List<String> values) {
StringBuilder sb = new StringBuilder();
for (int i = 0; i < partitionColNames.length && i < values.size(); i++) {
if (i > 0) sb.append('/');
- sb.append(partitionColNames[i]).append('=').append(values.get(i));
+ sb.append(escapePathName(partitionColNames[i]))
Review Comment:
The key is lowercased before escaping now (f6685bc5d), and the comment at
the add path is corrected.
I kept the default partition mapping out. As far as I know, Hive and Spark
store such a partition with the literal value `__HIVE_DEFAULT_PARTITION__`, and
that name already round-trips. Mapping it back to an empty value on read would
make get and drop look up `""` and miss those partitions. I have not checked
this against partitions written by those engines on Glue, so please tell me if
you see otherwise. A default partition can still be added by passing the
literal value.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueTableOperations.java:
##########
@@ -235,20 +271,22 @@ public boolean dropPartition(String partitionName) {
/**
* Builds a Hive-style partition name (e.g. {@code
dt=2024-01-01/country=us}) from an ordered list
- * of Glue partition values.
+ * of Glue partition values. Values are escaped as Hive does, so the name
parses back to them.
*/
private String buildPartitionName(List<String> values) {
StringBuilder sb = new StringBuilder();
for (int i = 0; i < partitionColNames.length && i < values.size(); i++) {
if (i > 0) sb.append('/');
- sb.append(partitionColNames[i]).append('=').append(values.get(i));
+ sb.append(escapePathName(partitionColNames[i]))
+ .append('=')
+ .append(escapePathName(values.get(i)));
}
return sb.toString();
}
/**
* Parses a Hive-style partition name (e.g. {@code
dt=2024-01-01/country=us}) into an ordered list
- * of values, validating that the keys match the table's partition columns
in order.
+ * of unescaped values, validating that the keys match the table's partition
columns in order.
*/
private List<String> parsePartitionName(String partitionName) {
String[] parts = partitionName.split("/");
Review Comment:
I checked this and left `split("/")` as is: `"dt=2024-01-01/hr=".split("/")`
keeps the last segment, because `hr=` is not an empty string. Only a name
ending in `/` loses a trailing segment.
`testPartitionWithEmptyValueIsAddressableByItsListedName` in f6685bc5d covers
listing such a partition and getting it by the listed name.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -550,70 +591,311 @@ public GlueTable alterTable(NameIdentifier ident,
TableChange... changes)
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(),
+ rawSd != null && rawSd.skewedInfo() != null &&
rawSd.skewedInfo().hasSkewedColumnNames()
+ ? rawSd.skewedInfo().skewedColumnNames()
+ : 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();
+ // A table parameter of the same name is shown when the storage
descriptor has no value, so
+ // it is removed as well.
+ if (isStorageProperty(property)) {
Review Comment:
Fixed in f6685bc5d: setting a storage property now also removes the table
parameter of the same name, so a later remove does not bring the old value back.
--
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]
