fix(hive-sync): sync column and partition column comments to HMS by rangareddy · Pull Request #19289 · apache/hudi

rangareddy · 2026-07-14T16:59:13Z

Describe the issue this Pull Request addresses

Closes #17359 (HUDI-8843), reported in #11922.

When hoodie.datasource.hive_sync.sync_comment=true, comments of partition columns were never synced to the Hive metastore:

hms sync mode: HMSDDLExecutor.updateTableComments only updated the columns of the storage descriptor and ignored the table's partition keys, so partition column comments were silently dropped.
jdbc/hiveql sync modes: the generated ALTER TABLE ... CHANGE COLUMN statement targeted the partition column, which Hive rejects with Invalid column reference, failing the entire sync.
Additionally, createTable never populated any comments (regular or partition columns), so comments only appeared from the second sync onward.

The equivalent AWS Glue path is also broken (comments are silently dropped since the AWS SDK v2 upgrade) and is tracked separately in #19316; this PR only fixes the Hive paths.

Summary and Changelog

With sync_comment=true, column and partition column comments are now synced to HMS when the table is created, and partition column comments are kept in sync afterwards in hms sync mode.

HMSDDLExecutor.createTable: populates column and partition column comments from the storage schema field docs when sync_comment=true.
HMSDDLExecutor.updateTableComments: also updates comments of the table's partition keys (comment-only changes pass HMS alter-table validation, which compares partition key names/types only).
HiveSchemaUtil.generateCreateDDL (used by jdbc/hiveql sync modes): appends COMMENT '...' clauses to both regular and partition column definitions when sync_comment=true. Comments and TBLPROPERTIES keys/values are backslash-escaped (HiveSchemaUtil.escapeSqlString) so docs containing quotes or backslashes survive as SQL string literals; the same escaping is applied in QueryBasedDDLExecutor.updateTableComments, which previously stripped single quotes.
DDLExecutor.supportsUpdatingPartitionColumnComments(): new capability, false for query based executors since ALTER TABLE ... CHANGE COLUMN fails on partition columns and HiveQL has no other DDL for them. HoodieHiveSyncClient.updateTableComments filters partition columns out of the comment diff when unsupported, so jdbc/hiveql syncs no longer fail and no longer report a schema change when nothing else was applied. Partition column comments are still applied at create time in these modes.
SparkSchemaUtils.convertToSparkSchemaJson / SparkDataSourceTableUtils.getSparkTableProperties: new includeFieldDocs variant that carries field docs as comment in the field metadata of the serialized Spark schema (spark.sql.sources.schema table property). Spark rebuilds the schema of a datasource table from this property and ignores the HMS column comments, so without this Spark DESCRIBE showed no comments even though they were present in HMS. Enabled from HiveSyncTool when sync_comment=true; all other callers keep the previous behavior.
Tests: new testSyncCommentsForPartitionColumns (create path, all sync modes expect comments) and testUpdateCommentsForPartitionColumns (alter path, hms expects the partition comments, query-based modes gracefully skip) in TestHiveSyncTool, with a fixture schema partition-doced-test.avsc using multiple partition columns (with and without docs) and a doc containing a single quote and a trailing backslash to exercise the SQL escaping; TestSparkSchemaUtils#testConvertWithFieldDocs covers flat and nested field docs.

Impact

Users get column and partition column comments in HMS from the first sync when hoodie.datasource.hive_sync.sync_comment=true. No change when the config is disabled (default). Fixes a sync failure in jdbc/hiveql modes when the partition field has a doc in the table schema.

Risk Level

Low. All changes are gated behind hoodie.datasource.hive_sync.sync_comment (default false), except the partition-column filtering (which turns a guaranteed sync failure into a no-op) and the SQL escaping of comments and table properties in the create DDL (which previously broke on quotes). Verified with TestHiveSyncTool comment tests across hms/jdbc/hiveql sync modes, TestHiveSchemaUtil and TestSparkSchemaUtils.

Documentation Update

None. No new configs; behavior now matches the existing description of hoodie.datasource.hive_sync.sync_comment.

Contributor's checklist

Read through contributor's guide
Enough context is provided in the sections above
Adequate tests were added if applicable

hudi-bot · 2026-07-14T17:06:15Z

CI report:

c261b60 UNKNOWN

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

hudi-agent

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for working on this! This PR syncs regular and partition column comments to HMS at table creation and keeps partition column comments in sync afterward for hms mode, while gracefully skipping the unsupported partition-column comment updates in jdbc/hiveql modes. The comment lookups consistently use lowercased field names, the SQL escaping follows the existing replace("'", "") convention already in the codebase, and the changes are additive with backward-compatible overloads. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. A few naming and simplification suggestions below.

cc @yihua

voonhous · 2026-07-15T03:41:45Z

@rangareddy Commit Co-author Validation is failing, can you please edit the commit message to remove the co-author then do a force-push? Thank you.

hudi-agent

⚠️ 🤖 This review was generated by an AI agent and may contain mistakes. Please verify any suggestions before applying.

Thanks for working on this! This PR syncs regular and partition column comments to HMS on table creation and keeps partition column comments in sync afterward in hms mode, while gracefully skipping the unsupported partition-column comment update in jdbc/hiveql modes. I traced the create/update paths (case-insensitive field-doc lookups, partition-key handling in HMSDDLExecutor.updateTableComments, the reOrderedSchema doc preservation for spark table properties, and the recursive spark-schema JSON escaping) and the logic holds up. No new issues flagged from this automated pass beyond the comment-escaping points already raised in prior rounds — a Hudi committer or PMC member can take it from here for a final review. Two naming issues to flag — the rest of the change is clean.

cc @yihua

hudi-agent · 2026-07-15T10:17:00Z

    return columns.toString();
  }

+  private static String escapeSqlComment(String doc) {


🤖 nit: could you rename this to stripSqlComment (or sanitizeSqlComment)? In SQL, "escaping" a single quote conventionally means doubling it (' → ''), so a future reader who sees escapeSqlComment may expect that behavior and be surprised to find the character is silently dropped instead.

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

hudi-agent · 2026-07-15T10:17:00Z

+    }
+  }
+
+  private static void applyFieldDocs(List<FieldSchema> fields, Map<String, String> fieldDocs) {


🤖 nit: applyFieldDocs and setFieldComments (line 291) do structurally the same thing — iterate fields and call fieldSchema.setComment(...) — but they're named with different verbs (apply vs set) and different nouns (Docs vs Comments). Have you considered aligning them, e.g. applyFieldComments for the schema-comment case as well, so the pairing is obvious?

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

voonhous

Left a few comments inline. The fix itself looks solid - main ask is real escaping in escapeSqlComment, the rest are nits.

voonhous · 2026-07-16T09:23:34Z


+  private static String escapeSqlComment(String doc) {
+    // strip single quotes to avoid breaking the SQL string literal
+    return doc.replace("'", "");


If a doc ends with a backslash this produces COMMENT 'foo\' and the backslash eats the closing quote, so the DDL fails to parse - the same failure this PR is fixing. Escaping instead of stripping avoids that and keeps the quotes: doc.replace("\\", "\\\\").replace("'", "\\'"). It would also make hms and jdbc/hiveql store the same comment when a doc has quotes in it.

voonhous · 2026-07-16T09:23:34Z

      String name = field.getKey();
+      // ALTER TABLE ... CHANGE COLUMN fails on partition columns; only HMS sync mode can update their comments
+      if (partitionFields.contains(name.toLowerCase(Locale.ROOT))) {
+        log.warn("Skipping comment update for partition column {} of table {}: not supported in sync mode jdbc/hiveql, use hms sync mode instead", name, tableName);


The diff is built in HoodieHiveSyncClient before this check, so a doced partition column logs this warn on every sync forever, and updateTableComments up there still returns true even when everything got skipped here. Might be cleaner to filter partition fields out of the diff at the client level for the query-based executors.

voonhous · 2026-07-16T09:23:34Z

-      }
+      setFieldComments(sd.getCols(), alterSchema);
      table.setSd(sd);
+      if (table.getPartitionKeys() != null) {


nit: thrift returns an empty list here, not null, so the guard can go.

voonhous · 2026-07-16T09:23:34Z

+  public static Map<String, String> getFieldDocs(HoodieSchema schema) {
+    return schema.getFields().stream()
+        .filter(field -> field.doc().map(doc -> !doc.isEmpty()).orElse(false))
+        .collect(Collectors.toMap(field -> field.name().toLowerCase(Locale.ROOT), field -> field.doc().get(), (existing, duplicate) -> existing));


nit: two fields colliding case-insensitively silently keeps the first doc. Fine in practice since Hive lowercases everything, but maybe say so in the javadoc.

voonhous · 2026-07-16T09:23:34Z

+        "comment of the partition column should be synced on table creation");
+
+    SessionState.start(HiveTestUtil.getHiveConf());
+    Driver hiveDriver = new org.apache.hadoop.hive.ql.Driver(HiveTestUtil.getHiveConf());


nit: Driver is already imported, the fully qualified name can go.

voonhous · 2026-07-16T09:23:34Z

+ "type": "record",
+ "name": "User",
+ "fields": [
+     {"name": "name", "type": "string","doc":"name_comment"},


Can one of these docs get a single quote or a trailing backslash? Nothing exercises escapeSqlComment through the jdbc/hiveql create path right now, and that is exactly where the escaping matters.

voonhous

One note on the description: it says this follows #11922 and that Glue's AWSGlueCatalogSyncClient.updateTableComments already handles partition keys. That's no longer true. setComments (hudi-aws, lines 477-482) does Column.builder().comment(comment).build() and drops the result, so since the SDK v2 upgrade (#9347, blame e9dd73fc08fd replaced the working column.setComment(...)) it applies nothing and updateTableComments returns false -- the Glue comment tests only assert comments they pre-baked into the mock Column, so they don't catch it.

The HMS code in this PR is correct (it mutates FieldSchema in place), so nothing here needs to change. Could you just drop the "Glue already handles this" framing from the description, and file a follow-up issue for the Glue side (rebuild the column list, e.g. columns.set(i, col.toBuilder().comment(...).build()))?

voonhous · 2026-07-16T10:20:57Z

+  @Test
+  public void testConvertWithFieldDocs() {
+    HoodieSchema schema = HoodieSchema.createRecord("root", null, null, false, Arrays.asList(
+        HoodieSchemaField.of("id", HoodieSchema.create(HoodieSchemaType.STRING), "Unique \"id\"\nof the record", null),


These are all flat fields, so the includeFieldDocs threading through the array/map/record recursion never runs in this test. Could you add a nested record with a doc on a sub-field and assert the nested getComment()? That recursion is the easiest part of the change to break.

voonhous · 2026-07-16T10:20:57Z

+    hiveSyncProps.setProperty(HIVE_SYNC_MODE.key(), syncMode);
+    hiveSyncProps.setProperty(HIVE_SYNC_COMMENT.key(), "true");
+    String commitTime = "100";
+    HiveTestUtil.createCOWTableWithSchema(commitTime, "/partition-doced-test.avsc");


The fixture has a single partition column, so the per-key partition loops (and the jdbc/hiveql skip) only ever see one key. A second doced partition field would cover ordering and the multi-key skip path.

voonhous · 2026-07-16T10:20:57Z

+    reInitHiveSyncClient();
+    reSyncHiveTable();
+
+    Map<String, String> commentsByField = hiveClient.getMetastoreFieldSchemas(HiveTestUtil.TABLE_NAME)


nit: this getMetastoreFieldSchemas(...).collect(toMap(getName, getCommentOrEmpty)) block is copy-pasted in both new tests (again at 1082). Worth a small helper like metastoreFieldComments(table).

When hoodie.datasource.hive_sync.sync_comment is enabled, comments of partition columns were never synced to the Hive metastore: - In hms sync mode, HMSDDLExecutor.updateTableComments only updated the columns in the storage descriptor and ignored the table's partition keys, silently dropping partition column comments. - In jdbc/hiveql sync modes, the generated ALTER TABLE ... CHANGE COLUMN statement targeted the partition column, which Hive rejects with "Invalid column reference", failing the whole sync. - createTable never populated any comments, so even regular column comments only appeared from the second sync onwards. - The spark schema serialized into the spark.sql.sources.schema table property never carried the field docs, so spark DESCRIBE showed no comments even when they were present in HMS. Changes: - HMSDDLExecutor.createTable populates column and partition column comments from the storage schema field docs when sync_comment is on. - HMSDDLExecutor.updateTableComments also updates the partition keys, matching AWSGlueCatalogSyncClient.updateTableComments. - HiveSchemaUtil.generateCreateDDL (jdbc/hiveql modes) appends COMMENT clauses to regular and partition column definitions when sync_comment is on. - QueryBasedDDLExecutor.updateTableComments skips partition columns with a warning since HiveQL has no DDL to alter partition column comments, instead of failing the sync. - SparkSchemaUtils.convertToSparkSchemaJson can now include field docs as the comment of the field metadata; HiveSyncTool enables it for the spark.sql.sources.schema table property when sync_comment is on. Fixes apache#17359 (HUDI-8843), follow-up of apache#11922 which fixed the same problem for AWS Glue.

github-actions Bot added the size:M PR with lines of changes in (100, 300] label Jul 14, 2026

hudi-agent reviewed Jul 14, 2026

View reviewed changes

rangareddy force-pushed the HUDI-8843-sync-partition-column-comments branch from c261b60 to f6b88c3 Compare July 15, 2026 09:50

hudi-agent reviewed Jul 15, 2026

View reviewed changes

voonhous reviewed Jul 16, 2026

View reviewed changes

voonhous mentioned this pull request Jul 16, 2026

[TRACKING] Open PR backlog triage: verdicts for open PRs (close / convert-to-issue / keep) #19306

Open

voonhous reviewed Jul 16, 2026

View reviewed changes

rangareddy mentioned this pull request Jul 17, 2026

[SUPPORT] AWSGlueCatalogSyncClient.updateTableComments applies no comments since AWS SDK v2 upgrade #19316

Open

rangareddy force-pushed the HUDI-8843-sync-partition-column-comments branch from f6b88c3 to 86b2bf6 Compare July 17, 2026 16:26

github-actions Bot added size:L PR with lines of changes in (300, 1000] and removed size:M PR with lines of changes in (100, 300] labels Jul 17, 2026

Uh oh!

Conversation

rangareddy commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the issue this Pull Request addresses

Summary and Changelog

Impact

Risk Level

Documentation Update

Contributor's checklist

Uh oh!

hudi-bot commented Jul 14, 2026

CI report:

Uh oh!

hudi-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voonhous commented Jul 15, 2026

Uh oh!

hudi-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voonhous left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voonhous left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rangareddy commented Jul 14, 2026 •

edited

Loading