Gabriel39 commented on PR #65329:
URL: https://github.com/apache/doris/pull/65329#issuecomment-5018462412
Static review findings for HEAD `1eb8cc8e87b9c7228cfe65024d54fc9397761b03`:
1. **[P1] Nested `MODIFY COLUMN` can silently clear existing Iceberg field
docs when `COMMENT` is omitted.**
`ColumnDefinition` tracks `commentSpecified`, but
`IcebergMetadataOps.modifyColumn` only uses that flag when validating
collection pseudo-fields. The actual primitive and complex update paths always
pass `column.getComment()` to `updateColumn` or `updateColumnDoc`. Since an
omitted comment becomes an empty string, a statement such as `ALTER TABLE t
MODIFY COLUMN s.a BIGINT` clears the existing doc on `s.a`. For a full `STRUCT`
modification, `applyStructChange` can similarly clear the docs of every
existing child whose comment is not repeated.
Please preserve `currentCol.doc()` when `column.isCommentSpecified()` is
false, and only clear the doc for an explicitly empty `COMMENT`. Struct
children should likewise distinguish an omitted comment from an explicitly
empty one.
Relevant code: `IcebergMetadataOps.java` lines 1107-1133, 1209-1215, and
1529-1547.
2. **[P2] The regression suite executes nested comment changes but never
verifies that the comments were persisted.**
`test_iceberg_nested_schema_evolution_ddl.groovy` modifies comments on
struct, array-element-struct, and map-value-struct fields, but the final
metadata query selects only `COLUMN_NAME` and `COLUMN_TYPE`. The unit coverage
only verifies calls on a mocked `UpdateSchema`, so it cannot catch the
metadata-loss case above.
Please add read-back coverage for: comment-only modification, type
modification with omitted `COMMENT` preserving the old doc, explicit empty
`COMMENT` clearing it, and full `STRUCT` modification preserving unspecified
child docs.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
