github-actions[bot] commented on code in PR #69163: URL: https://github.com/apache/doris/pull/69163#discussion_r4296381783
########## fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUuidWriteConverter.java: ########## @@ -0,0 +1,165 @@ +// 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. + +package org.apache.doris.datasource.iceberg; + +import org.apache.doris.nereids.trees.expressions.ArrayItemReference; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.IsNull; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Array; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ArrayMap; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateMap; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateNamedStruct; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ElementAt; +import org.apache.doris.nereids.trees.expressions.functions.scalar.If; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.trees.expressions.literal.MapLiteral; +import org.apache.doris.nereids.trees.expressions.literal.NullLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StringLikeLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StringLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StructLiteral; +import org.apache.doris.nereids.trees.expressions.literal.UuidLiteral; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; +import org.apache.doris.nereids.types.UuidType; +import org.apache.doris.nereids.util.TypeCoercionUtils; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** Validates UUID text before ordinary Iceberg sink coercion, including nested leaves. */ +public final class IcebergUuidWriteConverter { + private IcebergUuidWriteConverter() { + } + + public static Expression convert(Expression input, DataType targetType) { + return TypeCoercionUtils.castIfNotSameType(convertUuid(input, targetType), targetType); + } + + public static boolean containsUuid(DataType type) { + if (type instanceof ArrayType) { + return containsUuid(((ArrayType) type).getItemType()); + } + if (type instanceof MapType) { + return containsUuid(((MapType) type).getKeyType()) || containsUuid(((MapType) type).getValueType()); + } + if (type instanceof StructType) { + return ((StructType) type).getFields().stream().anyMatch(field -> containsUuid(field.getDataType())); + } + return type.isUuidType(); + } + + private static Expression convertUuid(Expression input, DataType targetType) { + if (!needsConversion(input.getDataType(), targetType)) { + return input; + } + if (targetType instanceof ArrayType) { + DataType elementTarget = ((ArrayType) targetType).getItemType(); + ArrayItemReference item = new ArrayItemReference("item", input); + return new ArrayMap(new Lambda(Collections.singletonList(item.getName()), + convertUuid(item.toSlot(), elementTarget), Collections.singletonList(item))); + } + if (targetType instanceof MapType) { + MapType target = (MapType) targetType; + if (input instanceof CreateMap || input instanceof MapLiteral) { + List<Expression> source = new ArrayList<>(); + if (input instanceof MapLiteral) { + ((MapLiteral) input).getValue().forEach((key, value) -> { + source.add(key); + source.add(value); + }); + } else { + source.addAll(input.children()); + } + List<Expression> children = new ArrayList<>(); + for (int i = 0; i < source.size(); i++) { + DataType targetChild = i % 2 == 0 ? target.getKeyType() : target.getValueType(); + Expression child = source.get(i); + // A NULL-only map value has already been assigned TINYINT by generic function binding. + children.add(child instanceof NullLiteral ? new NullLiteral(targetChild) + : convertUuid(child, targetChild)); + } + return TypeCoercionUtils.processBoundFunction(new CreateMap(children.toArray(new Expression[0]))); + } + // Cast the entire map once: separately extracting keys and values would evaluate + // a volatile source twice and could pair keys with values from a different map. + return new Cast(input, targetType, false, true); Review Comment: [P2] Preserve nonstrict casts for non-UUID map values With `enable_strict_cast=false` and optional Iceberg map values, this reduced INSERT SELECT plan fails when `m` contains a valid UUID key and BIGINT value `2147483648`: ```text IcebergSink(m: MAP<UUID,INT>) Project(CAST_STRICT(m AS MAP<UUID,INT>)) Scan(m: MAP<STRING,BIGINT>) ``` The strict whole-map cast here passes the same strict context to both key and value wrappers in `create_map_wrapper`, so BIGINT-to-INT overflow aborts instead of producing the NULL value selected by ordinary sink coercion. A constructor map takes the branch above, validates only the UUID leaf, and retains normal sibling casts. Preserve single source evaluation while applying strict validation only to UUID text leaves. ########## fe/fe-core/src/main/java/org/apache/doris/analysis/UuidLiteral.java: ########## @@ -72,6 +72,12 @@ protected String toSqlImpl() { @Override protected String toSqlImpl(boolean disableTableName, boolean needExternalSql, TableType tableType, TableIf table) { + if (needExternalSql && table instanceof org.apache.doris.catalog.JdbcTable + && ((org.apache.doris.catalog.JdbcTable) table).getJdbcTableType() + == org.apache.doris.thrift.TOdbcTableType.TRINO) { Review Comment: [P2] Render typed UUID literals for the Presto dialect too `JdbcClient.createJdbcClient` routes both PRESTO and TRINO to `JdbcTrinoClient`, so the new UUID mapping also exposes native Presto UUID columns. For a Presto catalog, `WHERE u = CAST('00112233-4455-6677-8899-aabbccddeeff' AS UUID)` passes `shouldPushDownConjunct`, but this TRINO-only branch renders `u = '00112233-4455-6677-8899-aabbccddeeff'`. Presto has no implicit VARCHAR-to-UUID coercion, so equality and IN queries fail remotely. Include `TOdbcTableType.PRESTO` in this serializer, or keep these filters local, and cover both dialects. ########## fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUuidWriteConverter.java: ########## @@ -0,0 +1,165 @@ +// 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. + +package org.apache.doris.datasource.iceberg; + +import org.apache.doris.nereids.trees.expressions.ArrayItemReference; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.IsNull; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Array; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ArrayMap; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateMap; +import org.apache.doris.nereids.trees.expressions.functions.scalar.CreateNamedStruct; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ElementAt; +import org.apache.doris.nereids.trees.expressions.functions.scalar.If; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda; +import org.apache.doris.nereids.trees.expressions.literal.IntegerLiteral; +import org.apache.doris.nereids.trees.expressions.literal.MapLiteral; +import org.apache.doris.nereids.trees.expressions.literal.NullLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StringLikeLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StringLiteral; +import org.apache.doris.nereids.trees.expressions.literal.StructLiteral; +import org.apache.doris.nereids.trees.expressions.literal.UuidLiteral; +import org.apache.doris.nereids.types.ArrayType; +import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.types.MapType; +import org.apache.doris.nereids.types.StructField; +import org.apache.doris.nereids.types.StructType; +import org.apache.doris.nereids.types.UuidType; +import org.apache.doris.nereids.util.TypeCoercionUtils; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** Validates UUID text before ordinary Iceberg sink coercion, including nested leaves. */ +public final class IcebergUuidWriteConverter { + private IcebergUuidWriteConverter() { + } + + public static Expression convert(Expression input, DataType targetType) { + return TypeCoercionUtils.castIfNotSameType(convertUuid(input, targetType), targetType); + } + + public static boolean containsUuid(DataType type) { + if (type instanceof ArrayType) { + return containsUuid(((ArrayType) type).getItemType()); + } + if (type instanceof MapType) { + return containsUuid(((MapType) type).getKeyType()) || containsUuid(((MapType) type).getValueType()); + } + if (type instanceof StructType) { + return ((StructType) type).getFields().stream().anyMatch(field -> containsUuid(field.getDataType())); + } + return type.isUuidType(); + } + + private static Expression convertUuid(Expression input, DataType targetType) { + if (!needsConversion(input.getDataType(), targetType)) { + return input; + } + if (targetType instanceof ArrayType) { + DataType elementTarget = ((ArrayType) targetType).getItemType(); + ArrayItemReference item = new ArrayItemReference("item", input); + return new ArrayMap(new Lambda(Collections.singletonList(item.getName()), + convertUuid(item.toSlot(), elementTarget), Collections.singletonList(item))); + } + if (targetType instanceof MapType) { + MapType target = (MapType) targetType; + if (input instanceof CreateMap || input instanceof MapLiteral) { + List<Expression> source = new ArrayList<>(); + if (input instanceof MapLiteral) { + ((MapLiteral) input).getValue().forEach((key, value) -> { + source.add(key); + source.add(value); + }); + } else { + source.addAll(input.children()); + } + List<Expression> children = new ArrayList<>(); + for (int i = 0; i < source.size(); i++) { + DataType targetChild = i % 2 == 0 ? target.getKeyType() : target.getValueType(); + Expression child = source.get(i); + // A NULL-only map value has already been assigned TINYINT by generic function binding. + children.add(child instanceof NullLiteral ? new NullLiteral(targetChild) + : convertUuid(child, targetChild)); + } + return TypeCoercionUtils.processBoundFunction(new CreateMap(children.toArray(new Expression[0]))); + } + // Cast the entire map once: separately extracting keys and values would evaluate + // a volatile source twice and could pair keys with values from a different map. + return new Cast(input, targetType, false, true); + } + if (targetType instanceof StructType) { + if (!(input instanceof SlotReference) && !(input instanceof StructLiteral)) { + // Bind the record once before null testing and field extraction; VALUES inputs may + // be volatile and have no source slot yet. The singleton array supplies that binding. + Expression singleton = TypeCoercionUtils.processBoundFunction(new Array(input)); + Expression converted = convertUuid(singleton, ArrayType.of(targetType)); + return TypeCoercionUtils.processBoundFunction(new ElementAt(converted, new IntegerLiteral(1))); + } + List<StructField> targetFields = ((StructType) targetType).getFields(); + List<Expression> fields = new ArrayList<>(); + for (int i = 0; i < targetFields.size(); i++) { + Expression field = input instanceof StructLiteral ? ((StructLiteral) input).getValue().get(i) + : TypeCoercionUtils.processBoundFunction(new ElementAt(input, new IntegerLiteral(i + 1))); + fields.add(new StringLiteral(targetFields.get(i).getName())); + fields.add(convertUuid(field, targetFields.get(i).getDataType())); + } + Expression result = TypeCoercionUtils.processBoundFunction( + new CreateNamedStruct(fields.toArray(new Expression[0]))); + // Reconstructing a nullable struct must not turn NULL into a non-null struct of NULL fields. + return input.nullable() ? TypeCoercionUtils.processBoundFunction( + new If(new IsNull(input), new NullLiteral(result.getDataType()), result)) : result; + } + if (input instanceof StringLikeLiteral) { + return new UuidLiteral(((StringLikeLiteral) input).getStringValue()); + } + // Reject malformed UUID text on write instead of silently replacing it with NULL. + return new Cast(input, targetType, false, true); + } + + private static boolean needsConversion(DataType input, DataType semantic) { + if (semantic instanceof UuidType) { + return input.isStringLikeType(); + } + if (semantic instanceof ArrayType && input instanceof ArrayType) { Review Comment: [P2] Validate UUID leaves in serialized complex inputs With `enable_strict_cast=false` and optional Iceberg list elements, a STRING source containing `["invalid-uuid"]` reaches this supported sink plan: ```text IcebergSink(items: ARRAY<UUID>) Project(CAST(text AS ARRAY<UUID>)) Scan(text: STRING) ``` `needsConversion(STRING, ARRAY<UUID>)` falls through to false because the container cases require an already matching input shape. `convert` then inserts an ordinary cast; the array's nullable UUID SerDe swallows the parse error and the insert commits `[NULL]`. The same invalid UUID supplied through `ARRAY<STRING>` is rejected by the new validation. Recognize string-like inputs to UUID-containing containers before ordinary coercion and validate their UUID leaves; serialized MAP and STRUCT inputs have the same gap. -- 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]
