hudi-agent commented on code in PR #19488:
URL: https://github.com/apache/hudi/pull/19488#discussion_r3818581391
##########
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() {
+ List<Column> columns =
Collections.singletonList(GlueTestUtil.getColumn("name", "string", "person's
name"));
+ Map<String, Option<String>> comments = new HashMap<>();
+ comments.put("name", Option.of("person's name"));
+
+ List<Column> updated = AWSGlueCatalogSyncClient.withComments(columns,
comments);
+
+ assertSame(columns.get(0), updated.get(0), "an unchanged column should be
returned as-is");
+ }
+
+ /**
+ * Why the storage descriptor itself has to be rebuilt, not just the column
list: its {@code columns()} is
+ * unmodifiable, and a descriptor built with new columns is a different
object. Editing a copy of the list
+ * and then sending the original descriptor would silently drop the comments.
+ */
+ @Test
+ void testRebuildingColumnsRequiresRebuildingTheStorageDescriptor() {
+ Column column = GlueTestUtil.getColumn("name", "string", null);
Review Comment:
🤖 nit: this test asserts `UnsupportedOperationException` on a raw AWS SDK
list rather than any code in `AWSGlueCatalogSyncClient` — it's documenting SDK
behaviour more than exercising Hudi logic. Could you fold the immutability
assertion into `testWithCommentsAppliesTheStorageComment` as a comment, and
rename this test to focus on what the Hudi code actually does (e.g.
`testWithCommentsReturnsANewDescriptorWhenColumnsChange`)?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
