github-actions[bot] commented on code in PR #65329:
URL: https://github.com/apache/doris/pull/65329#discussion_r3592526239
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java:
##########
@@ -207,8 +217,15 @@ public String getComment(boolean escapeQuota) {
* toSql
*/
public String toSql() {
+ return toSql("`" + name + "`");
+ }
+
+ /**
+ * Convert this column definition to SQL with a caller-provided column
name.
+ */
+ public String toSql(String columnNameSql) {
Review Comment:
[P2] Preserve omitted nullability in MODIFY rendering
The new `nullableSpecified` bit lets direct execution distinguish omitted
nullability from explicit `NULL`, but this renderer still emits `NULL` whenever
the type defaults nullable. As a result, parsing `MODIFY COLUMN info.metric
BIGINT`, rendering it, and reparsing it changes `nullableSpecified` from false
to true; the Iceberg path can then call `makeColumnOptional` on a required
field. Please omit the nullability clause here when it was not specified
(programmatic boolean constructors already mark it explicit) and add a MODIFY
round-trip test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyColumnCommentOp.java:
##########
@@ -79,8 +88,8 @@ public boolean allowOpRowBinlog() {
@Override
public String toSql() {
StringBuilder sb = new StringBuilder();
- sb.append("MODIFY COLUMN COMMENT ").append(colName);
- sb.append(" '").append(comment).append("'");
+ sb.append("MODIFY COLUMN ").append(columnPath.toSql());
+ sb.append(" COMMENT '").append(comment).append("'");
Review Comment:
[P2] Escape the comment literal in the new renderer
A valid statement can use a double-quoted comment containing an apostrophe,
for example `COMMENT "owner's field"`. `stripQuotes` stores that exact value,
but this line emits `COMMENT 'owner's field'`, which is invalid on reparse.
Please render the comment through the established escaped string-literal path
(for example the double-quoted `SqlUtils.escapeQuota` form used by column
definitions) and cover both quote characters in a round-trip test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterTableCommand.java:
##########
@@ -129,13 +128,132 @@ private void validate(ConnectContext ctx) throws
UserException {
if (tableIf.isTemporary()) {
throw new AnalysisException("Do not support alter temporary
table[" + tableName + "]");
}
+ checkColumnOperationsSupported(tableIf, ops);
+ for (AlterTableOp op : ops) {
+ op.setTableName(tbl);
+ op.validate(ctx);
+ }
if (tableIf instanceof OlapTable) {
rewriteAlterOpForOlapTable(ctx, (OlapTable) tableIf);
} else {
checkExternalTableOperationAllow(tableIf);
}
}
+ static void checkColumnOperationsSupported(TableIf table,
List<AlterTableOp> alterTableOps)
+ throws AnalysisException {
+ if (table instanceof IcebergExternalTable) {
+ for (AlterTableOp alterTableOp : alterTableOps) {
+ ColumnDefinition columnDefinition =
getColumnDefinition(alterTableOp);
+ ColumnPath nestedColumnPath =
getNestedColumnPath(alterTableOp);
Review Comment:
[P2] Use nested-aware name validation for Iceberg fields
This code recognizes a nested path, but the later generic `op.validate()`
still applies top-level Doris system-column prefix rules to its leaf. For an
imported Spark schema with `s.__DORIS_metric`, `MODIFY COLUMN s.__doris_metric
BIGINT` fails in `FeNameFormat`'s case-insensitive hidden-prefix guard before
the canonical Iceberg resolver, even though nested `StructField`s carry no
Doris hidden-column role. `ADD` has the same problem for a new legal
reserved-prefix nested field, and `RENAME` rejects such a target. Please retain
the ordinary type/comment checks for nested external operations without
applying top-level hidden/shadow-prefix semantics, and add command coverage for
an imported reserved-prefix leaf.
--
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]
