voonhous commented on code in PR #19488:
URL: https://github.com/apache/hudi/pull/19488#discussion_r3916660683
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -529,7 +555,7 @@ && getTable(awsGlue, databaseName,
tableName).partitionKeys().equals(partitionKe
.tableType(table.tableType())
.parameters(table.parameters())
.partitionKeys(partitionKeys)
- .storageDescriptor(storageDescriptor)
+ .storageDescriptor(updatedStorageDescriptor)
Review Comment:
**major:** The `UpdateTableRequest` built just below (lines 563-566) does
not pass `.skipArchive(skipTableArchive)`, while `updateTableSchema` (line 615)
and `updateTableParameters` (line 1208) do, and
`hoodie.datasource.meta.sync.glue.skip_table_archive` defaults to `true`. This
branch was unreachable before (the old `setComments` never changed anything),
so every comment sync will now archive a Glue table version regardless of the
config. Could we add `.skipArchive(skipTableArchive)` to this request?
(`updateSerdeProperties` at line 1121 has the same pre-existing omission.)
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -509,16 +530,21 @@ public boolean updateTableComments(String tableName,
List<FieldSchema> fromMetas
Map<String, Option<String>> commentsMap =
fromStorage.stream().collect(Collectors.toMap(FieldSchema::getName,
FieldSchema::getComment));
StorageDescriptor storageDescriptor = table.storageDescriptor();
- List<Column> columns = storageDescriptor.columns();
- setComments(columns, commentsMap);
-
- List<Column> partitionKeys = table.partitionKeys();
- setComments(partitionKeys, commentsMap);
+ List<Column> partitionKeys = withComments(table.partitionKeys(),
commentsMap);
+ List<Column> updatedColumns = withComments(storageDescriptor.columns(),
commentsMap);
Review Comment:
**major:** This restores the update path, but `createTable` still writes
`comment("")` for every column and partition key (`getColumnsFromSchema` line
1149, line 716), and `HiveSyncTool.syncHoodieTable` runs `syncFirstTime`
without `syncSchema`, so with `sync_comment=true` a new Glue table gets its
comments only on the second sync. #19289 closed exactly this gap on the HMS
side (`HMSDDLExecutor.createTable` via `HiveSchemaUtil.getFieldDocs`). Could
`getColumnsFromSchema` and the partition-key builder populate the docs when
`HIVE_SYNC_COMMENT` is on, with an assertion on the captured
`CreateTableRequest`? If that is deliberately out of scope, could the
description say comments appear from the second sync onwards?
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -474,11 +475,31 @@ public boolean updateTableProperties(String tableName,
Map<String, String> table
}
}
- private void setComments(List<Column> columns, Map<String, Option<String>>
commentsMap) {
- columns.forEach(column -> {
- String comment = commentsMap.getOrDefault(column.name(),
Option.empty()).orElse(null);
- Column.builder().comment(comment).build();
- });
+ /**
+ * Returns {@code columns} with the comment of every column the storage
schema knows about replaced by the
+ * one the schema carries, clearing it when the schema has none.
+ *
+ * <p>Columns the schema says nothing about are left untouched rather than
cleared. The pre-SDK-v2 code
+ * cleared them, but only nominally: it built a {@code Column} and discarded
it, so no comment was ever
+ * applied and nothing can depend on that behaviour. Clearing is also the
more dangerous reading - the
+ * storage field names keep the Avro schema's case while a catalog may hold
them lowercased, and a name
+ * that fails to match would silently wipe a comment. This matches
+ * {@code HMSDDLExecutor.applyFieldComments} on the Hive side, which only
touches known columns.
+ *
+ * <p>SDK v2 model classes are immutable and their getters return
unmodifiable lists, so the columns cannot
+ * be edited in place; a new list of rebuilt columns is returned instead.
+ */
+ @VisibleForTesting
+ static List<Column> withComments(List<Column> columns, Map<String,
Option<String>> commentsMap) {
+ return columns.stream()
+ .map(column -> {
+ if (!commentsMap.containsKey(column.name())) {
+ return column;
+ }
+ String comment = commentsMap.get(column.name()).orElse(null);
+ return Objects.equals(comment, column.comment()) ? column :
column.toBuilder().comment(comment).build();
Review Comment:
**major:** Hudi's own `createTable`/`getColumnsFromSchema` write
`comment("")`, a storage field without a doc yields `null` here, and
`Objects.equals(null, "")` is false, so every doc-less column is rebuilt.
Checked on a merge with master: Glue columns carrying `""` plus a doc-less
`fromStorage` make `updateTableComments` return `true` and send an
`UpdateTable` whose only effect is stripping the empty comments, i.e. one
redundant write per table after every create and schema evolution (per sync if
Glue echoes `""` back). HMS normalises this via
`FieldSchema.getCommentOrEmpty`. Could we treat `""` and `null` as equal, and
add the `""` case to `testUpdateTableCommentsIsANoOpWhenNothingChanges`?
```suggestion
String comment = commentsMap.get(column.name()).orElse(null);
String current = column.comment() == null ? "" : column.comment();
return current.equals(comment == null ? "" : comment) ? column :
column.toBuilder().comment(comment).build();
```
##########
hudi-aws/src/test/java/org/apache/hudi/aws/testutils/GlueTestUtil.java:
##########
@@ -110,9 +111,18 @@ public static void createHoodieTable() throws IOException {
.setPayloadClass(HoodieAvroPayload.class)
.initTable(HadoopFSUtils.getStorageConf(new Configuration()),
basePath);
- String instantTime = "101";
+ // Write the commit through HoodieTestTable rather than by hand. A
table-version-8+ timeline lives under
+ // .hoodie/timeline and is read through CommitMetadataSerDe, so a
hand-written JSON file in .hoodie is not
+ // on the timeline at all and TableSchemaResolver finds no schema - which
left every sync path that reads
+ // the table schema failing before it reached what it was meant to
exercise.
HoodieCommitMetadata commitMetadata = new HoodieCommitMetadata(false);
- createMetaFile(basePath, new
DefaultInstantFileNameGenerator().makeCommitFileName(instantTime),
commitMetadata);
+ commitMetadata.addMetadata(HoodieCommitMetadata.SCHEMA_KEY,
getSimpleSchema().toAvroSchema().toString());
+ try {
+ HoodieTestTable.of(metaClient).addCommit("101",
Option.of(commitMetadata));
+ } catch (Exception e) {
+ throw new IOException("Failed to seed the test table with a commit", e);
+ }
+ metaClient.reloadActiveTimeline();
}
public static HoodieSchema getSimpleSchema() {
Review Comment:
**minor:** Now that the fixture puts a schema on the timeline,
`getStorageFieldSchemas()` (`AWSGlueCatalogSyncClient` line 514, the `f.doc()`
that actually sources the comments) is still never exercised with a real Avro
doc: this schema has none and every test hand-builds `FieldSchema`. Not
blocking: could one field here carry a doc and a test assert that
`getStorageFieldSchemas()` surfaces it?
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -474,11 +475,31 @@ public boolean updateTableProperties(String tableName,
Map<String, String> table
}
}
- private void setComments(List<Column> columns, Map<String, Option<String>>
commentsMap) {
- columns.forEach(column -> {
- String comment = commentsMap.getOrDefault(column.name(),
Option.empty()).orElse(null);
- Column.builder().comment(comment).build();
- });
+ /**
+ * Returns {@code columns} with the comment of every column the storage
schema knows about replaced by the
+ * one the schema carries, clearing it when the schema has none.
+ *
+ * <p>Columns the schema says nothing about are left untouched rather than
cleared. The pre-SDK-v2 code
+ * cleared them, but only nominally: it built a {@code Column} and discarded
it, so no comment was ever
+ * applied and nothing can depend on that behaviour. Clearing is also the
more dangerous reading - the
+ * storage field names keep the Avro schema's case while a catalog may hold
them lowercased, and a name
+ * that fails to match would silently wipe a comment. This matches
+ * {@code HMSDDLExecutor.applyFieldComments} on the Hive side, which only
touches known columns.
+ *
+ * <p>SDK v2 model classes are immutable and their getters return
unmodifiable lists, so the columns cannot
+ * be edited in place; a new list of rebuilt columns is returned instead.
+ */
+ @VisibleForTesting
+ static List<Column> withComments(List<Column> columns, Map<String,
Option<String>> commentsMap) {
+ return columns.stream()
+ .map(column -> {
Review Comment:
**minor:** Matching is case-exact here, while
`HoodieHiveSyncClient.updateTableComments` lowercases both sides. Hudi-created
Glue tables preserve the Avro case (`hoodieSchemaToMapSchema` never lowercases
names), so this is consistent for Hudi's own tables; a table created out of
band with lowercased names silently never gets comments, which is the scenario
the javadoc cites. Not blocking: could we lowercase both keys as the HMS path
does, or pin the exact-match choice with a mixed-case test?
##########
hudi-aws/src/test/java/org/apache/hudi/aws/testutils/GlueTestUtil.java:
##########
@@ -110,9 +111,18 @@ public static void createHoodieTable() throws IOException {
.setPayloadClass(HoodieAvroPayload.class)
.initTable(HadoopFSUtils.getStorageConf(new Configuration()),
basePath);
- String instantTime = "101";
+ // Write the commit through HoodieTestTable rather than by hand. A
table-version-8+ timeline lives under
Review Comment:
**nit:** The description says `createMetaFile` is removed, but it is still
declared at lines 136-143, unreferenced, along with three imports only it uses
(`FSDataOutputStream`, `StandardCharsets`, `METAFOLDER_NAME`). Checkstyle has
`UnusedImports` but no unused-private-method rule, which is why it builds
clean. Could we delete the method and those imports, or drop the claim from the
description?
##########
hudi-aws/src/test/java/org/apache/hudi/aws/sync/TestAWSGlueSyncClient.java:
##########
@@ -253,6 +259,176 @@ void testMetastoreFieldSchemas_EmptyPartitions() {
assertEquals("person's age", fields.get(1).getComment().get(), "glue table
second column comment should person's age");
}
+ /**
+ * End to end through {@code updateTableComments}: the Glue table holds no
comments, the storage schema has
+ * them, so it must apply them and report that it changed something. This is
the path the bug actually broke
+ * - {@code setComments} discarded its rebuilt {@code Column}, so nothing
was applied and the method always
+ * returned false. It also pins the second half of the fix: {@code
StorageDescriptor} is immutable too, so
+ * rebuilding only the column list would still have sent a descriptor
carrying no comments.
+ */
+ @Test
+ void testUpdateTableCommentsAppliesThemToColumnsAndPartitionKeys() throws
Exception {
+ String tableName = "testTable";
+ List<Column> columns = Arrays.asList(GlueTestUtil.getColumn("name",
"string", null),
+ GlueTestUtil.getColumn("age", "int", null));
+ List<Column> partitionKeys =
Collections.singletonList(GlueTestUtil.getColumn("city", "string", null));
+ Mockito.when(mockAwsGlue.getTable(any(GetTableRequest.class)))
+ .thenReturn(getTableWithDefaultProps(tableName, columns,
partitionKeys));
+ Mockito.when(mockAwsGlue.updateTable(any(UpdateTableRequest.class)))
+
.thenReturn(CompletableFuture.completedFuture(UpdateTableResponse.builder().build()));
+
+ List<FieldSchema> fromStorage = Arrays.asList(
+ new FieldSchema("name", "string", "person's name"),
+ new FieldSchema("age", "int", "person's age"),
+ new FieldSchema("city", "string", "person's city"));
+
+ assertTrue(awsGlueSyncClient.updateTableComments(tableName,
Collections.emptyList(), fromStorage),
+ "applying comments the table does not have should report a change");
+
+ ArgumentCaptor<UpdateTableRequest> captor =
ArgumentCaptor.forClass(UpdateTableRequest.class);
+ verify(mockAwsGlue, times(1)).updateTable(captor.capture());
+ TableInput sent = captor.getValue().tableInput();
+ assertEquals("person's name",
sent.storageDescriptor().columns().get(0).comment(),
+ "the rebuilt storage descriptor must be the one sent, carrying the
column comment");
+ assertEquals("person's age",
sent.storageDescriptor().columns().get(1).comment());
+ assertEquals("person's city", sent.partitionKeys().get(0).comment(),
+ "partition column comments must be sent too");
+ }
+
+ /** The other direction: comments already matching the storage schema must
not trigger an update call. */
+ @Test
+ void testUpdateTableCommentsIsANoOpWhenNothingChanges() throws Exception {
+ String tableName = "testTable";
+ List<Column> columns = Arrays.asList(GlueTestUtil.getColumn("name",
"string", "person's name"),
+ GlueTestUtil.getColumn("age", "int", "person's age"));
+ List<Column> partitionKeys =
Collections.singletonList(GlueTestUtil.getColumn("city", "string", "person's
city"));
+ Mockito.when(mockAwsGlue.getTable(any(GetTableRequest.class)))
+ .thenReturn(getTableWithDefaultProps(tableName, columns,
partitionKeys));
+
+ List<FieldSchema> fromStorage = Arrays.asList(
+ new FieldSchema("name", "string", "person's name"),
+ new FieldSchema("age", "int", "person's age"),
+ new FieldSchema("city", "string", "person's city"));
+
+ assertFalse(awsGlueSyncClient.updateTableComments(tableName,
Collections.emptyList(), fromStorage),
+ "comments already matching the storage schema should not report a
change");
+ verify(mockAwsGlue, never()).updateTable(any(UpdateTableRequest.class));
+ }
+
+ /**
+ * A Glue table can carry a storage descriptor whose column list was never
set. SDK v2 returns an
+ * auto-construct list for that, and {@code hasColumns()} is false.
Rebuilding the descriptor
+ * unconditionally would set an explicit empty list, flip {@code
hasColumns()} to true and make the
+ * descriptor compare unequal to itself, reporting a change and sending an
{@code updateTable} that
+ * changes nothing - on every sync, since the fetched table comes back the
same way each time.
+ */
+ @Test
+ void testUpdateTableCommentsIsANoOpWhenTheTableHasNoColumns() {
+ String tableName = "testTable";
+ StorageDescriptor noColumns = StorageDescriptor.builder()
+
.serdeInfo(SerDeInfo.builder().serializationLibrary("serde").parameters(new
HashMap<>()).build())
+ .inputFormat("inputFormat")
+ .location(glueSyncProps.getString(META_SYNC_BASE_PATH.key()))
+ .outputFormat("outputFormat")
+ .build();
+ assertFalse(noColumns.hasColumns(), "precondition: the column list must be
unset, not empty");
+ Table table = Table.builder()
+ .name(tableName)
+ .tableType("COPY_ON_WRITE")
+ .parameters(new HashMap<>())
+ .storageDescriptor(noColumns)
+ .build();
+ Mockito.when(mockAwsGlue.getTable(any(GetTableRequest.class)))
+
.thenReturn(CompletableFuture.completedFuture(GetTableResponse.builder().table(table).build()));
+
+ assertFalse(awsGlueSyncClient.updateTableComments(tableName,
Collections.emptyList(), Collections.emptyList()),
+ "a table with no columns has nothing to update, so it must not report
a change");
+ verify(mockAwsGlue, never()).updateTable(any(UpdateTableRequest.class));
+ }
+
+ /**
+ * The bug this covers: {@code setComments} built a {@code Column} and
dropped the result, so no comment
+ * was ever applied and {@code updateTableComments} always reported no
change. AWS SDK v2 model classes are
+ * immutable, so the column has to be rebuilt and put back.
+ */
+ @Test
+ void testWithCommentsAppliesTheStorageComment() {
+ List<Column> columns = Arrays.asList(GlueTestUtil.getColumn("name",
"string", null),
+ GlueTestUtil.getColumn("age", "int", "stale comment"));
+ Map<String, Option<String>> comments = new HashMap<>();
+ comments.put("name", Option.of("person's name"));
+ comments.put("age", Option.of("person's age"));
+
+ List<Column> updated = AWSGlueCatalogSyncClient.withComments(columns,
comments);
+
+ assertEquals("person's name", updated.get(0).comment(), "a missing comment
should be applied");
+ assertEquals("person's age", updated.get(1).comment(), "an out-of-date
comment should be replaced");
+ assertNull(columns.get(0).comment(), "the input columns must not be
mutated");
+ assertEquals("stale comment", columns.get(1).comment(), "the input columns
must not be mutated");
+ }
+
+ @Test
+ void testWithCommentsClearsTheCommentOfAKnownColumnWithoutADoc() {
+ List<Column> columns =
Collections.singletonList(GlueTestUtil.getColumn("name", "string", "old
comment"));
+ Map<String, Option<String>> comments = new HashMap<>();
+ comments.put("name", Option.empty());
+
+ List<Column> updated = AWSGlueCatalogSyncClient.withComments(columns,
comments);
+
+ assertNull(updated.get(0).comment(),
+ "the storage schema is authoritative for a column it knows, so its
comment should be cleared");
+ }
+
+ /**
+ * A column the storage schema says nothing about is left alone rather than
cleared - the storage field
+ * names keep the Avro schema's case while a catalog may hold them
lowercased, so a name that fails to
+ * match must not silently wipe a comment. Matches {@code
HMSDDLExecutor.applyFieldComments}.
+ */
+ @Test
+ void testWithCommentsLeavesColumnsTheStorageSchemaDoesNotKnowAlone() {
+ List<Column> columns =
Collections.singletonList(GlueTestUtil.getColumn("myCol", "string", "keep me"));
+
+ List<Column> updated = AWSGlueCatalogSyncClient.withComments(columns,
Collections.emptyMap());
+
+ assertEquals("keep me", updated.get(0).comment(), "an unknown column's
comment must be preserved");
+ assertSame(columns.get(0), updated.get(0), "and the column should be
returned as-is");
+ }
+
+ @Test
+ void testWithCommentsLeavesAnUpToDateColumnAlone() {
Review Comment:
**nit:** This only pins `assertSame`, i.e. that no new `Column` is
allocated, which nothing downstream depends on (`updatedColumns.equals(...)`
holds either way). Feel free to ignore: could it fold into
`testWithCommentsAppliesTheStorageComment` as a third, already-correct column?
##########
hudi-aws/src/test/java/org/apache/hudi/aws/sync/TestAWSGlueSyncClient.java:
##########
@@ -253,6 +259,176 @@ void testMetastoreFieldSchemas_EmptyPartitions() {
assertEquals("person's age", fields.get(1).getComment().get(), "glue table
second column comment should person's age");
}
+ /**
+ * End to end through {@code updateTableComments}: the Glue table holds no
comments, the storage schema has
+ * them, so it must apply them and report that it changed something. This is
the path the bug actually broke
+ * - {@code setComments} discarded its rebuilt {@code Column}, so nothing
was applied and the method always
+ * returned false. It also pins the second half of the fix: {@code
StorageDescriptor} is immutable too, so
+ * rebuilding only the column list would still have sent a descriptor
carrying no comments.
+ */
+ @Test
+ void testUpdateTableCommentsAppliesThemToColumnsAndPartitionKeys() throws
Exception {
+ String tableName = "testTable";
+ List<Column> columns = Arrays.asList(GlueTestUtil.getColumn("name",
"string", null),
+ GlueTestUtil.getColumn("age", "int", null));
+ List<Column> partitionKeys =
Collections.singletonList(GlueTestUtil.getColumn("city", "string", null));
+ Mockito.when(mockAwsGlue.getTable(any(GetTableRequest.class)))
+ .thenReturn(getTableWithDefaultProps(tableName, columns,
partitionKeys));
+ Mockito.when(mockAwsGlue.updateTable(any(UpdateTableRequest.class)))
+
.thenReturn(CompletableFuture.completedFuture(UpdateTableResponse.builder().build()));
+
+ List<FieldSchema> fromStorage = Arrays.asList(
+ new FieldSchema("name", "string", "person's name"),
+ new FieldSchema("age", "int", "person's age"),
+ new FieldSchema("city", "string", "person's city"));
+
+ assertTrue(awsGlueSyncClient.updateTableComments(tableName,
Collections.emptyList(), fromStorage),
+ "applying comments the table does not have should report a change");
+
+ ArgumentCaptor<UpdateTableRequest> captor =
ArgumentCaptor.forClass(UpdateTableRequest.class);
+ verify(mockAwsGlue, times(1)).updateTable(captor.capture());
+ TableInput sent = captor.getValue().tableInput();
+ assertEquals("person's name",
sent.storageDescriptor().columns().get(0).comment(),
+ "the rebuilt storage descriptor must be the one sent, carrying the
column comment");
+ assertEquals("person's age",
sent.storageDescriptor().columns().get(1).comment());
+ assertEquals("person's city", sent.partitionKeys().get(0).comment(),
+ "partition column comments must be sent too");
+ }
+
+ /** The other direction: comments already matching the storage schema must
not trigger an update call. */
+ @Test
+ void testUpdateTableCommentsIsANoOpWhenNothingChanges() throws Exception {
+ String tableName = "testTable";
+ List<Column> columns = Arrays.asList(GlueTestUtil.getColumn("name",
"string", "person's name"),
Review Comment:
**nit:** This `name/age/city` triple and the matching `fromStorage` list now
appear six times in the class (lines 223, 246, 302, 488, 1014, 1042), most
predating this PR. Feel free to ignore: would a small `personColumns(...)` /
`personFields()` helper be worth it while touching the file?
--
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]
