Gabriel39 commented on code in PR #69163:
URL: https://github.com/apache/doris/pull/69163#discussion_r4306444060
##########
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:
Fixed in ad56ece849. Native UUID literals now use the typed UUID syntax for
both PRESTO and TRINO. The test exercises literal rendering plus equality, IN
and NOT IN predicates for both dialects. All 73 focused FE tests pass.
##########
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:
Fixed in ad56ece849. The dynamic-map strict cast now targets an intermediate
type that changes only UUID text leaves, preserving all non-UUID source types
(including nested siblings). Ordinary sink coercion then performs narrowing.
This retains a single evaluation of volatile sources. The original SQL
reproduced the BIGINT-to-INT overflow exception under enable_strict_cast=false;
the expanded Parquet and ORC write regression now verifies a NULL value and the
preserved UUID key. Existing volatile-map checks also pass.
##########
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:
Fixed in ad56ece849. Serialized UUID-containing containers are first parsed
with UUID leaves represented as text; strict UUID validation then runs before
ordinary sink coercion. This also handles serialized containers nested inside
complex inputs. The original code reproduced invalid ARRAY/STRUCT/map-value
UUID text becoming NULL. Added FE coverage and executed Parquet/ORC regression
cases for invalid serialized ARRAY, MAP, STRUCT and nested ARRAY<MAP>, valid
compact UUID text, nullable elements and containers; these now pass without
changing golden files.
--
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]
