github-actions[bot] commented on code in PR #65329: URL: https://github.com/apache/doris/pull/65329#discussion_r3586186124
########## regression-test/suites/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.groovy: ########## @@ -0,0 +1,153 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_iceberg_nested_schema_evolution_ddl", "p0,external,doris,external_docker,external_docker_doris") { + + String enabled = context.config.otherConfigs.get("enableIcebergTest") + if (enabled == null || !enabled.equalsIgnoreCase("true")) { + logger.info("disable iceberg test.") + return + } + + String restPort = context.config.otherConfigs.get("iceberg_rest_uri_port") + String minioPort = context.config.otherConfigs.get("iceberg_minio_port") + String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") + String catalogName = "test_iceberg_nested_schema_evolution_ddl" + String dbName = "iceberg_nested_schema_evolution_db" + String tableName = "iceberg_nested_evolution" + + sql """drop catalog if exists ${catalogName}""" + sql """ + CREATE CATALOG ${catalogName} PROPERTIES ( + 'type'='iceberg', + 'iceberg.catalog.type'='rest', + 'uri' = 'http://${externalEnvIp}:${restPort}', + "s3.access_key" = "admin", + "s3.secret_key" = "password", + "s3.endpoint" = "http://${externalEnvIp}:${minioPort}";, + "s3.region" = "us-east-1" + );""" + + sql """switch ${catalogName};""" + sql """drop database if exists ${dbName} force""" + sql """create database ${dbName}""" + sql """use ${dbName};""" + + sql """set enable_fallback_to_original_planner=false;""" + + sql """drop table if exists ${tableName}""" + sql """ + CREATE TABLE ${tableName} ( + id INT NOT NULL, + s STRUCT<a:INT, b:STRING>, + arr ARRAY<STRUCT<x:INT>>, + m MAP<STRING, STRUCT<v:INT>>, + arr_scalar ARRAY<INT>, + m_scalar MAP<STRING, INT> + ); + """ + + sql """ + INSERT INTO ${tableName} VALUES ( + 1, + STRUCT(10, 'old'), + ARRAY(STRUCT(100)), + MAP('k', STRUCT(1000)), + ARRAY(7), + MAP('k', 70) + ) + """ + + sql """ALTER TABLE ${tableName} ADD COLUMN s.c STRING NULL COMMENT 'new nested field'""" + sql """ALTER TABLE ${tableName} ADD COLUMN s.first_pos STRING NULL FIRST""" + sql """ALTER TABLE ${tableName} ADD COLUMN s.after_a STRING NULL AFTER a""" + sql """ALTER TABLE ${tableName} ADD COLUMN arr.element.y INT NULL""" + sql """ALTER TABLE ${tableName} ADD COLUMN arr.element.after_x INT NULL AFTER x""" + sql """ALTER TABLE ${tableName} ADD COLUMN m.value.y INT NULL""" + sql """ALTER TABLE ${tableName} ADD COLUMN m.value.after_v INT NULL AFTER v""" + sql """ALTER TABLE ${tableName} ADD COLUMN s.drop_me STRING NULL""" + sql """ALTER TABLE ${tableName} ADD COLUMN arr.element.drop_me INT NULL""" + sql """ALTER TABLE ${tableName} ADD COLUMN m.value.drop_me INT NULL""" + + test { + sql """ALTER TABLE ${tableName} ADD COLUMN s.required_field INT NOT NULL""" + exception "Field 'required_field' doesn't have a default value" Review Comment: This negative case no longer matches the error produced by the implementation. The nested Iceberg add path now rejects `NOT NULL` before calling Iceberg with `New nested field 's.required_field' must be nullable`, so the regression matcher looking for `Field 'required_field' doesn't have a default value` will fail. Please update the expected substring to the Doris-side validation message, or remove the early validation if the intended contract is still the Iceberg error. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/AlterTableCommand.java: ########## @@ -129,13 +128,97 @@ 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); + // Column translation cannot distinguish an omitted default from an explicit DEFAULT NULL. + if (alterTableOp instanceof ModifyColumnOp && columnDefinition != null + && (columnDefinition.hasDefaultValue() + || columnDefinition.hasOnUpdateDefaultValue())) { + throw new AnalysisException( + "DEFAULT and ON UPDATE are not supported for Iceberg MODIFY COLUMN"); + } + ColumnPath columnPath = getNestedColumnPath(alterTableOp); Review Comment: This Iceberg-specific gate still lets some parsed column-clause metadata reach external dispatch even though the Iceberg path drops it. For nested ADD/DROP/MODIFY, `PROPERTIES(...)` is stored on the op but never checked here, and `Alter.processAlterTableForExternalTable` forwards only the `ColumnPath`/`Column`/position, so `ALTER TABLE iceberg_t ADD COLUMN s.c STRING NULL PROPERTIES ("k"="v")` can succeed while ignoring the requested properties. The same early `columnPath == null` continue leaves top-level Iceberg `KEY`/generated-column definitions and `TO`/`FROM` rollup targets accepted, and grouped `ADD COLUMN (...)` is skipped by the helper checks altogether while its rollup/properties and per-column key/generated intent are also dropped before `UpdateSchema`. Please reject unsupported properties, rollup names, KEY, and generated-column clauses for all affected Iceberg column operations before dispatch, or implement explicit Iceberg semantics for them, and add negative coverage . -- 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]
