github-actions[bot] commented on code in PR #65329: URL: https://github.com/apache/doris/pull/65329#discussion_r3536107443
########## regression-test/suites/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.groovy: ########## @@ -0,0 +1,154 @@ +// 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 """set show_column_comment_in_describe=true;""" + + 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>> + ); + """ + + sql """ + INSERT INTO ${tableName} VALUES ( + 1, + STRUCT(10, 'old'), + ARRAY(STRUCT(100)), + MAP('k', STRUCT(1000)) + ) + """ + + sql """ALTER TABLE ${tableName} ADD COLUMN s.c STRING NULL COMMENT 'new nested field'""" + sql """ALTER TABLE ${tableName} ADD COLUMN arr.element.y INT NULL""" + sql """ALTER TABLE ${tableName} ADD COLUMN m.value.y 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" + } + + sql """ALTER TABLE ${tableName} MODIFY COLUMN s.a BIGINT""" + sql """ALTER TABLE ${tableName} MODIFY COLUMN arr.element.x BIGINT""" + sql """ALTER TABLE ${tableName} MODIFY COLUMN m.value.v BIGINT""" + + test { + sql """ALTER TABLE ${tableName} DROP COLUMN s.c""" + exception "Nested drop column is not supported for Iceberg table in current phase" + } + test { + sql """ALTER TABLE ${tableName} RENAME COLUMN s.c TO c2""" + exception "Nested rename column is not supported for Iceberg table in current phase" + } + + sql """ + INSERT INTO ${tableName} VALUES ( + 2, + STRUCT(20, 'new', 'c2'), + ARRAY(STRUCT(200, 201)), + MAP('k', STRUCT(2000, 2001)) + ) + """ + + def descRows = sql """DESC ${tableName}""" + def normalizeType = { String s -> s.toLowerCase().replaceAll("\\s+", "") } + def typeOf = { String name -> + def row = descRows.find { it[0].toString().equalsIgnoreCase(name) } + assertTrue(row != null, "column not found: ${name}") + return normalizeType(row[1].toString()) + } + + def sType = typeOf("s") + assertTrue(sType.contains("a:bigint"), sType) + assertTrue(sType.contains("c:text"), sType) + + def arrType = typeOf("arr") + assertTrue(arrType.contains("x:bigint"), arrType) + assertTrue(arrType.contains("y:int"), arrType) + + def mType = typeOf("m") + assertTrue(mType.contains("v:bigint"), mType) + assertTrue(mType.contains("y:int"), mType) + + def queryRows = sql """ + SELECT id, + element_at(s, 'a'), + element_at(s, 'c'), + element_at(arr[1], 'x'), + element_at(arr[1], 'y'), + element_at(m['k'], 'v'), + element_at(m['k'], 'y') + FROM ${tableName} + ORDER BY id + """ + assertTrue(queryRows.size() == 2, queryRows.toString()) Review Comment: Please convert these deterministic result checks to `qt_`/`order_qt_` output checks and add the generated `.out` file. This suite validates the final `DESC` schema and the ordered query result only through Groovy `assertTrue` calls, and there is currently no `regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.out`. That leaves the user-visible output outside the regression framework baseline checks, contrary to the repo rule for determined expected results. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergMetadataOps.java: ########## @@ -927,13 +1046,60 @@ private void validateForModifyComplexColumn(Column column, NestedField currentCo throw new UserException(e.getMessage(), e); } if (currentCol.isOptional() && !column.isAllowNull()) { - throw new UserException("Cannot change nullable column " + column.getName() + " to not null"); + throw new UserException("Cannot change nullable column " + columnPath + " to not null"); } if (column.getDefaultValue() != null || column.getDefaultValueExprDef() != null) { throw new UserException("Complex type default value only supports NULL"); } } + @VisibleForTesting + org.apache.iceberg.types.Type resolveNestedColumnPath(Schema schema, ColumnPath columnPath, String operation) + throws UserException { + org.apache.iceberg.types.Type currentType = schema.asStruct(); + String currentPath = ""; + for (String part : columnPath.getParts()) { + if (!currentPath.isEmpty()) { + currentPath += "."; Review Comment: Please make nested path resolution case-insensitive and return the canonical Iceberg path before calling `UpdateSchema`. Doris lower-cases Iceberg metadata when it builds `Column`/`StructField`, so a Spark-created schema such as `Info STRUCT<Metric: INT>` is exposed to users as `info.metric`. This resolver uses `StructType.field(part)`, which is exact-name lookup, and `modifyColumn` also calls exact `schema.findField(columnPath.getFullPath())`; as a result `ALTER TABLE ... MODIFY COLUMN info.metric ...` and adding a child under `info` reject the existing field even though that is the only Doris-visible spelling. Please resolve with Iceberg case-insensitive APIs, then pass the canonical path names to validation and `updateSchema.*`. -- 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]
