rangareddy commented on code in PR #19488:
URL: https://github.com/apache/hudi/pull/19488#discussion_r3819678294
##########
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:
Thanks - the first assertion is fair, but I would rather keep the test as it
is.
Only the `assertThrows` is SDK characterisation. The other four assertions
all run Hudi code: the test calls `AWSGlueCatalogSyncClient.withComments`, then
checks that the original descriptor is untouched, that the rebuilt one carries
the comment, and that `original` and `updated` compare unequal.
That last one is load bearing rather than incidental. `updateTableComments`
decides whether to call `updateTable` purely by comparing the original
descriptor against the rebuilt one, so descriptor equality *is* the Hudi logic
here. It is also the assertion that would have caught the auto-construct-list
bug fixed in this revision, where a rebuilt descriptor compared unequal to
itself for a table whose column list was never set.
Folding the immutability check into
`testWithCommentsAppliesTheStorageComment` would also mix two subjects: that
test covers `withComments` on a plain list, this one covers the descriptor.
Happy to reconsider if a committer prefers the split.
--
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]
