github-actions[bot] commented on code in PR #65329:
URL: https://github.com/apache/doris/pull/65329#discussion_r3578271502
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterTableCommand.java:
##########
@@ -129,13 +128,59 @@ private void validate(ConnectContext ctx) throws
UserException {
if (tableIf.isTemporary()) {
throw new AnalysisException("Do not support alter temporary
table[" + tableName + "]");
}
+ checkNestedColumnPathSupported(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 checkNestedColumnPathSupported(TableIf table,
List<AlterTableOp> alterTableOps)
+ throws AnalysisException {
+ if (table instanceof IcebergExternalTable) {
Review Comment:
The new nested-column path needs to reject rollup targets before it reaches
the external Iceberg dispatch. The grammar still accepts `ADD COLUMN s.c ...
TO/IN rollup`, `DROP COLUMN s.c FROM rollup`, and `MODIFY COLUMN s.c ... FROM
rollup`, and `LogicalPlanBuilder` stores those names on the ops. For Iceberg
tables this method returns after only checking nested MODIFY defaults, while
the op validators only treat rollups specially for `OlapTable`.
`Alter.processAlterTableForExternalTable` then calls the catalog with only
`ColumnPath`/`Column`/position, so the requested rollup target is silently
discarded and the base Iceberg schema is mutated. Please reject non-null rollup
names for Iceberg/external nested column schema changes and add negative
command coverage for these clauses.
##########
fe/fe-sql-parser/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -1554,6 +1554,19 @@ columnDef
(COMMENT comment=STRING_LITERAL)?
;
+columnDefWithPath
+ : colName=qualifiedName type=dataType
+ KEY?
Review Comment:
This path-aware definition also accepts `KEY` for nested Iceberg ALTERs, but
the key intent is never enforced or applied. `visitColumnDefWithPath` preserves
the flag on the `ColumnDefinition`, non-OLAP validation does not reject it for
ordinary primitive columns, and `validateCommonColumnInfo` only rejects
aggregation/auto-increment/type issues. The Iceberg add/modify calls then send
only type/comment/default/nullability/position to `UpdateSchema`, so `ALTER
TABLE iceberg_t ADD COLUMN s.k INT KEY NULL` can succeed as a normal nested
field. Please reject `KEY` for Iceberg/external nested ADD/MODIFY and add
negative coverage for these clauses.
##########
fe/fe-sql-parser/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -1554,6 +1554,19 @@ columnDef
(COMMENT comment=STRING_LITERAL)?
;
+columnDefWithPath
+ : colName=qualifiedName type=dataType
+ KEY?
+ (aggType=aggTypeDef)?
+ ((GENERATED ALWAYS)? AS LEFT_PAREN generatedExpr=expression
RIGHT_PAREN)?
Review Comment:
This new path-aware column definition also accepts generated-column syntax
for nested Iceberg ALTERs, but nothing later preserves that semantics in
Iceberg. `visitColumnDefWithPath` stores the `AS (...)` expression in
`GeneratedColumnDesc`, and schema-change translation carries it onto the
catalog `Column`; however the Iceberg add/modify paths only send
type/comment/default/nullability to `UpdateSchema`. A statement like `ALTER
TABLE iceberg_t ADD COLUMN s.g INT AS (id + 1) NULL` can therefore succeed as a
plain nested field, silently dropping the generated expression. Please reject
generated-column definitions for Iceberg/external nested ADD/MODIFY, or
implement explicit generated-column support, and add negative coverage for
these clauses.
--
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]
