hudi-agent commented on code in PR #19488:
URL: https://github.com/apache/hudi/pull/19488#discussion_r3946734969
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -682,12 +729,21 @@ public void createTable(String tableName,
try {
Map<String, String> mapSchema = hoodieSchemaToMapSchema(storageSchema,
config.getBoolean(HIVE_SUPPORT_TIMESTAMP_TYPE), false);
- List<Column> schemaWithoutPartitionKeys =
getColumnsFromSchema(mapSchema);
+ // Populate comments at create time rather than leaving them to the next
updateTableComments pass:
+ // HiveSyncTool.syncHoodieTable runs syncFirstTime without syncSchema,
so a table created with
+ // empty comments would only pick them up on the second sync. Mirrors
HiveSchemaUtil.generateCreateDDL
+ // on the HMS side, which gates the same lookup on HIVE_SYNC_COMMENT
(#19289).
+ Map<String, String> fieldDocs = config.getBoolean(HIVE_SYNC_COMMENT)
Review Comment:
🤖 nit: this `HIVE_SYNC_COMMENT ? getFieldDocs(schema) : emptyMap()` block
(and its explanatory comment) is duplicated in `updateTableSchema` — could you
pull it into a small `getFieldDocsIfEnabled(HoodieSchema)` helper so the gating
lives in one place?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -506,19 +541,25 @@ public List<FieldSchema> getStorageFieldSchemas() {
public boolean updateTableComments(String tableName, List<FieldSchema>
fromMetastore, List<FieldSchema> fromStorage) {
Table table = getTable(awsGlue, databaseName, tableName);
- Map<String, Option<String>> commentsMap =
fromStorage.stream().collect(Collectors.toMap(FieldSchema::getName,
FieldSchema::getComment));
+ Map<String, Option<String>> commentsMap = fromStorage.stream()
+ .collect(Collectors.toMap(f -> f.getName().toLowerCase(Locale.ROOT),
FieldSchema::getComment, (existing, duplicate) -> existing));
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);
Review Comment:
🤖 nit: could you rename this to `updatedPartitionKeys` to match
`updatedColumns`/`updatedStorageDescriptor`? Right now `partitionKeys` reads as
the original list, but it's the rebuilt one being compared against
`table.partitionKeys()` below.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-aws/src/main/java/org/apache/hudi/aws/sync/AWSGlueCatalogSyncClient.java:
##########
@@ -474,11 +478,42 @@ 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) {
Review Comment:
🤖 nit: it might be worth trimming this Javadoc to the current contract
(known columns get the schema's comment, unknown ones are left alone, returns a
new list) and moving the history of the pre-SDK-v2 bug to the PR description —
the same goes for the `#19488`/thread references in the test Javadocs.
<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]
