rangareddy commented on code in PR #19488:
URL: https://github.com/apache/hudi/pull/19488#discussion_r3924202020
##########
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:
Fixed, added `.skipArchive(skipTableArchive)` and pinned it with an
assertion on the captured request. I left the same omission in
`updateSerdeProperties` alone since it is pre-existing and unrelated to
comments; happy to fold it in if you would rather it went with this.
##########
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:
Fixed, and thanks for tracing it against master. `withComments` now
normalises via a small `emptyIfNull`, so `""` and null count as the same absent
comment. Added `testWithCommentsTreatsAnEmptyCommentAndNoCommentAsTheSame`
covering both directions.
##########
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:
Fixed rather than scoped out. `createTable` and `updateTableSchema` now
derive the docs via `HiveSchemaUtil.getFieldDocs`, gated on `HIVE_SYNC_COMMENT`
exactly as `generateCreateDDL` does, so comments land on the first sync.
`testCreateTableCarriesColumnCommentsOnTheFirstSync` asserts them on the
captured `CreateTableRequest`.
##########
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:
Changed to lowercase both sides with `Locale.ROOT`, matching
`HoodieHiveSyncClient.updateTableComments`, and added
`testWithCommentsMatchesNamesCaseInsensitively`. Consistency with the HMS path
seemed better than pinning an exact-match choice that the javadoc argues
against.
##########
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:
Done, the `name` field in the fixture now carries a doc
(`GlueTestUtil.NAME_FIELD_DOC`) and
`testGetStorageFieldSchemasSurfacesTheAvroDoc` asserts it reaches the sync,
with the doc-less `id` field as the negative case.
##########
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:
Removed, along with the three imports only it used. Thanks, the description
claim was correct about the intent and wrong about the code.
##########
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:
You are right and I have dropped the test. The descriptor is built with
explicit columns, so `hasColumns()` is true and the branch I was pointing at is
never reached; `testUpdateTableCommentsIsANoOpWhenTheTableHasNoColumns` is the
real coverage for it. The SDK-immutability note it carried already lives in the
`withComments` javadoc.
--
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]
