rangareddy opened a new pull request, #19488:
URL: https://github.com/apache/hudi/pull/19488
### Describe the issue this Pull Request addresses
Closes #19316.
`AWSGlueCatalogSyncClient.updateTableComments` has applied no column or
partition column comments since the
AWS SDK v2 upgrade (#9347). Its helper built a `Column` carrying the comment
and threw the result away:
```java
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(); // result dropped, column
unchanged
});
}
```
Before the upgrade this called `column.setComment(...)` on the mutable v1
model, which worked. SDK v2 model
classes are immutable, so nothing was applied: `updateTableComments` never
detected a change, always
returned `false`, and with `hoodie.datasource.hive_sync.sync_comment=true`
no comment ever reached Glue.
Found while reviewing #19289, which fixed the equivalent Hive metastore
paths.
### Summary and Changelog
- `setComments` becomes `withComments`, which returns a rebuilt list instead
of mutating in place.
- **The storage descriptor is rebuilt too.** Rebuilding only the column list
is not enough and is the part
worth reviewing: `StorageDescriptor` is immutable as well, and the
`UpdateTableRequest` was sending the
*original* descriptor. Editing a copy of `storageDescriptor.columns()` —
which is what the issue text
originally suggested — would still have shipped columns with no comments.
The request now sends the
descriptor rebuilt from the updated columns.
- **A column the storage schema says nothing about is left untouched rather
than cleared.** The pre-SDK-v2
code cleared it, but since that code has been a no-op for three years
nothing depends on it, and clearing
is the riskier reading: `getStorageFieldSchemas` keeps the Avro schema's
case while a catalog may hold
column names lowercased, so a name that failed to match would silently
wipe a user's comment. Columns the
schema *does* know are still authoritative — a known column with no doc
has its comment cleared. This
matches `HMSDDLExecutor.applyFieldComments`, added for the Hive side in
#19289, so the two catalogs now
agree.
- **Change detection now uses the table already fetched.** It compared a
freshly fetched table against local
objects it had not modified — trivially equal, and two extra Glue
`GetTable` calls per sync. It now
compares the fetched table against the rebuilt values, so one `GetTable`
call does the job.
### Verification
`withComments` is `@VisibleForTesting` and covered by four new tests in
`TestAWSGlueSyncClient`:
| test | what it pins |
| --- | --- |
| `testWithCommentsAppliesTheStorageComment` | a missing comment is applied,
a stale one replaced, and the input list is not mutated |
| `testWithCommentsClearsTheCommentOfAKnownColumnWithoutADoc` | the schema
is authoritative for columns it knows |
| `testWithCommentsLeavesColumnsTheStorageSchemaDoesNotKnowAlone` | an
unknown column's comment is preserved |
| `testRebuildingColumnsRequiresRebuildingTheStorageDescriptor` |
`storageDescriptor.columns()` is unmodifiable, and a descriptor rebuilt with
new columns is a different object — the trap the original bug fell into |
Restoring the build-and-drop behaviour inside `withComments` turns three of
them red, so they are not
passing vacuously:
```
[ERROR] testWithCommentsAppliesTheStorageComment
AssertionFailedError: a missing comment should be applied ==> expected:
<person's name> but was: <null>
[ERROR] testWithCommentsClearsTheCommentOfAKnownColumnWithoutADoc
AssertionFailedError: ... ==> expected: <null> but was: <old comment>
[ERROR] testRebuildingColumnsRequiresRebuildingTheStorageDescriptor
AssertionFailedError: the rebuilt descriptor carries the comment ==>
expected: <person's name> but was: <null>
```
Whole `hudi-aws` module: `Tests run: 99, Failures: 0, Errors: 0, Skipped:
16` (skips pre-existing).
`checkstyle:check` and `apache-rat:check` clean.
**What is not covered, and why:** an end-to-end test driving
`updateTableComments` itself and asserting the
captured `UpdateTableRequest`. I wrote one, but it cannot run against the
current Glue test fixture:
`updateTableComments` calls `getTableDoc()`, which resolves the table
schema, and `GlueTestUtil`'s table has
none — it writes its commit to `.hoodie/` while a table-version-8 timeline
lives under `.hoodie/timeline/`,
so the instant is not on the timeline and `TableSchemaResolver` throws
`HoodieSchemaNotFoundException`. That
is a pre-existing gap in the fixture rather than something this change
introduces, and fixing it properly
needs either a correctly written commit or a test-jar dependency that
`hudi-aws` does not currently have.
I did not want to bundle that into a bug fix, so it is called out here as a
follow-up. Happy to do it
separately if you would prefer the end-to-end coverage in this PR.
### Impact
With `hoodie.datasource.hive_sync.sync_comment=true`, Glue column and
partition column comments start being
applied on the update path, which is the documented behaviour and what
worked before the SDK v2 upgrade.
Users who already have comments in Glue keep them: only columns the storage
schema knows are touched.
Two extra `GetTable` calls per comment sync are removed. No API, config or
table format change.
### Risk Level
low — one helper rewritten and its result actually used, in a path that
currently does nothing at all.
The semantics of the "unknown column" case are deliberately narrower than
the pre-SDK-v2 code; that is
argued above rather than hidden.
### Documentation Update
none — no new config, and this restores documented behaviour rather than
changing it.
### Contributor's checklist
- [x] Read through [contributor's
guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable
- [x] CI passes on my PR
--
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]
