github-actions[bot] commented on code in PR #65329:
URL: https://github.com/apache/doris/pull/65329#discussion_r3628488469
##########
regression-test/suites/doc/sql-manual/basic-elements/data-types/struct-md.groovy:
##########
@@ -184,7 +184,7 @@ suite("struct-md", "p0") {
"replication_allocation" = "tag.location.default: 1"
);
"""
- exception "Aggregate type SUM is not compatible with primitive type
STRUCT<id:INT,name:TEXT>"
+ exception "Aggregate type SUM is not compatible with primitive type
STRUCT<`id`:INT,`name`:TEXT>"
Review Comment:
[P1] Keep ordinary STRUCT identifiers unquoted in these baselines
`StructField.toSql()` now quotes only names for which
`NereidsParser.isValidUnquotedIdentifier` is false. Both `id` and `name` are
valid unquoted identifiers (`NAME` is in `nonReserved`), so this validation
still produces `STRUCT<id:INT,name:TEXT>`, not the backticked text. The six
changed expectations in this file and the two in `test_array_function.groovy`
will deterministically miss and fail. Please restore the unquoted expectations,
or make the renderer and all tests consistently always quote.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterTableCommand.java:
##########
@@ -129,13 +128,151 @@ 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) {
+ checkIcebergCompoundColumnOperations(alterTableOps);
+ for (AlterTableOp alterTableOp : alterTableOps) {
+ ColumnDefinition columnDefinition =
getColumnDefinition(alterTableOp);
+ ColumnPath nestedColumnPath =
getNestedColumnPath(alterTableOp);
+ // Keep this before AddColumnOp.validate(), whose generic NOT
NULL check would mask
+ // the Iceberg nested-field invariant. Metadata validation
retains the same guard for other callers.
+ if (alterTableOp instanceof AddColumnOp && columnDefinition !=
null && nestedColumnPath != null
+ && !columnDefinition.isNullable()) {
+ throw new AnalysisException("New nested field '" +
nestedColumnPath.getFullPath()
+ + "' must be nullable");
+ }
+ if (!isIcebergColumnSchemaOperation(alterTableOp)) {
+ continue;
+ }
+ if (getRollupName(alterTableOp) != null) {
+ throw new AnalysisException("Rollup is not supported for
Iceberg column operations");
+ }
+ Map<String, String> properties = alterTableOp.getProperties();
+ if (properties != null && !properties.isEmpty()) {
+ throw new AnalysisException("PROPERTIES are not supported
for Iceberg column operations");
+ }
+ checkIcebergColumnDefinition(alterTableOp, columnDefinition);
+ if (alterTableOp instanceof AddColumnsOp) {
+ for (ColumnDefinition definition : ((AddColumnsOp)
alterTableOp).getColumnDefinitions()) {
+ checkIcebergColumnDefinition(alterTableOp, definition);
+ }
+ }
+ }
+ return;
+ }
+ for (AlterTableOp alterTableOp : alterTableOps) {
+ ColumnPath columnPath = getNestedColumnPath(alterTableOp);
+ if (columnPath != null) {
+ throw new AnalysisException("Nested column path is only
supported for Iceberg tables: "
+ + columnPath.getFullPath());
+ }
+ }
+ }
+
+ private static void
checkIcebergCompoundColumnOperations(List<AlterTableOp> alterTableOps)
+ throws AnalysisException {
+ if (alterTableOps.size() <= 1) {
+ return;
+ }
+ for (AlterTableOp alterTableOp : alterTableOps) {
Review Comment:
[P1] Prevent partial commits from compound Iceberg ALTERs
This gate still permits multiple top-level operations, but
`Alter.processAlterTableForExternalTable` dispatches each one to a separate
metadata method and `UpdateSchema.commit()`. On a v3 table, `ADD COLUMN ok INT
NULL, DROP COLUMN _row_id` commits `ok` first, then the new row-lineage
validation rejects `_row_id`; the client sees a failed statement while `ok`
remains persisted. A reserved rename target or reorder after a valid clause has
the same problem. Please preflight every clause before any commit, stage the
operations in one `UpdateSchema`, or reject compound Iceberg column operations.
--
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]
