yuqi1129 commented on code in PR #12007:
URL: https://github.com/apache/gravitino/pull/12007#discussion_r3618627314
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -170,7 +171,17 @@ protected String generateAlterTableSql(
} else if (change instanceof TableChange.UpdateComment) {
TableChange.UpdateComment updateComment = (TableChange.UpdateComment)
change;
String newComment = updateComment.getNewComment();
- alterSql.add("MODIFY COMMENT \"" + newComment + "\"");
+ if (StringIdentifier.fromComment(newComment) == null) {
+ lazyLoadTable = getOrCreateTable(databaseName, tableName,
lazyLoadTable);
+ StringIdentifier identifier =
StringIdentifier.fromComment(lazyLoadTable.comment());
+ if (identifier != null) {
+ newComment = StringIdentifier.addToComment(identifier, newComment);
+ }
+ }
+ if (StringUtils.isNotEmpty(newComment)) {
Review Comment:
I looked into this more closely against the StarRocks docker IT, and it
turns out the second `addToComment` should stay unconditional — guarding it
actually breaks the round-trip.
The reason is the asymmetry between write and read:
- **Read** (`StarRocksUtils.extractTableCommentFromSql`) strips exactly
**one** (outermost) identifier marker from the `SHOW CREATE TABLE` output.
- **Write** must therefore append one **sacrificial** marker on top of the
preserved identifier. `generateCreateTableSql` already does this
unconditionally (it always calls `addToComment(DUMMY_ID, comment)` even when
the comment already carries a real identifier), and `UpdateComment` mirrors it.
So the stored comment is `body (uid42) (uid-1)`, and after the read strips
the outer `(uid-1)`, the real identifier `(uid42)` survives — which is what the
JDBC layer needs to correlate the table with its Gravitino entity.
I verified this with
`TestStarRocksTableOperations.testTableCommentWithSqlLiteralCharacters`: adding
an `fromComment(...) == null` guard made the write store only `body (uid42)`,
the read then stripped `(uid42)`, and the loaded comment lost its identifier
entirely. I have reverted that change, so the behavior is unchanged from
before. Thanks for the careful look here.
--
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]
