From 9a94681b292f32b4e45543c5d8b8a5ebf7eca928 Mon Sep 17 00:00:00 2001 From: morrySnow <101034200+morrySnow@users.noreply.github.com> Date: Thu, 9 May 2024 10:51:41 +0800 Subject: [PATCH] [refactor](type) AggStateType should not extends ScalarType (#34463) 1. let AggStateType extends Type 2. remove useless interface isFixedLengthType and supportsTablePartitioning 3. let MapType implement interface isSupported 4. let VariantType extends ScalarType --- .../apache/doris/catalog/AggStateType.java | 49 +++++++++++++++---- .../org/apache/doris/catalog/ArrayType.java | 12 +---- .../org/apache/doris/catalog/MapType.java | 5 ++ .../org/apache/doris/catalog/ScalarType.java | 22 --------- .../org/apache/doris/catalog/StructType.java | 13 +---- .../java/org/apache/doris/catalog/Type.java | 39 +++++++++------ .../org/apache/doris/catalog/VariantType.java | 46 ++--------------- .../java/org/apache/doris/analysis/Expr.java | 4 +- .../doris/nereids/types/AggStateType.java | 2 +- .../agg_state/max/test_agg_state_max.groovy | 2 +- 10 files changed, 78 insertions(+), 116 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/AggStateType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/AggStateType.java index 72f93a2f69..e28364103f 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/AggStateType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/AggStateType.java @@ -17,16 +17,19 @@ package org.apache.doris.catalog; +import org.apache.doris.thrift.TScalarType; import org.apache.doris.thrift.TTypeDesc; import org.apache.doris.thrift.TTypeNode; +import org.apache.doris.thrift.TTypeNodeType; -import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import com.google.gson.annotations.SerializedName; import java.util.ArrayList; import java.util.List; +import java.util.Objects; -public class AggStateType extends ScalarType { +public class AggStateType extends Type { @SerializedName(value = "subTypes") private List subTypes; @@ -42,11 +45,12 @@ public class AggStateType extends ScalarType { public AggStateType(String functionName, Boolean resultIsNullable, List subTypes, List subTypeNullables) { - super(PrimitiveType.AGG_STATE); - Preconditions.checkState(subTypes != null); - Preconditions.checkState(subTypeNullables != null); - Preconditions.checkState(subTypes.size() == subTypeNullables.size(), - "AggStateType' subTypes.size()!=subTypeNullables.size()"); + Objects.requireNonNull(subTypes, "subTypes should not be null"); + Objects.requireNonNull(subTypeNullables, "subTypeNullables should not be null"); + if (subTypes.size() != subTypeNullables.size()) { + throw new IllegalStateException("AggStateType's subTypes.size() [" + subTypes.size() + + "] != subTypeNullables.size() [" + subTypeNullables.size() + "]"); + } this.functionName = functionName; this.subTypes = subTypes; this.subTypeNullables = subTypeNullables; @@ -86,13 +90,24 @@ public class AggStateType extends ScalarType { return stringBuilder.toString(); } + @Override + protected String prettyPrint(int lpad) { + return Strings.repeat(" ", lpad) + toSql(); + } + @Override public void toThrift(TTypeDesc container) { - super.toThrift(container); - List types = new ArrayList(); + TTypeNode node = new TTypeNode(); + container.types.add(node); + // ATTN: use scalar only for compatibility. + node.setType(TTypeNodeType.SCALAR); + TScalarType scalarType = new TScalarType(); + scalarType.setType(getPrimitiveType().toThrift()); + node.setScalarType(scalarType); + List types = new ArrayList<>(); for (int i = 0; i < subTypes.size(); i++) { TTypeDesc desc = new TTypeDesc(); - desc.setTypes(new ArrayList()); + desc.setTypes(new ArrayList<>()); subTypes.get(i).toThrift(desc); desc.setIsNullable(subTypeNullables.get(i)); types.add(desc); @@ -122,4 +137,18 @@ public class AggStateType extends ScalarType { } return true; } + + public PrimitiveType getPrimitiveType() { + return PrimitiveType.AGG_STATE; + } + + @Override + public int getSlotSize() { + return PrimitiveType.AGG_STATE.getSlotSize(); + } + + @Override + public boolean matchesType(Type t) { + return t.isAggStateType() || t.isStringType(); + } } diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/ArrayType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/ArrayType.java index 27f02c802c..19a55a0675 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/ArrayType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/ArrayType.java @@ -198,7 +198,7 @@ public class ArrayType extends Type { @Override public boolean isSupported() { - return !itemType.isNull(); + return itemType.isSupported() && !itemType.isNull(); } @Override @@ -223,16 +223,6 @@ public class ArrayType extends Type { return thrift; } - @Override - public boolean isFixedLengthType() { - return false; - } - - @Override - public boolean supportsTablePartitioning() { - return false; - } - @Override public int getSlotSize() { return PrimitiveType.ARRAY.getSlotSize(); diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/MapType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/MapType.java index 1291a5f364..53cd926ebe 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/MapType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/MapType.java @@ -250,4 +250,9 @@ public class MapType extends Type { public int hashCode() { return Objects.hash(keyType, valueType); } + + @Override + public boolean isSupported() { + return keyType.isSupported() && !keyType.isNull() && valueType.isSupported() && !valueType.isNull(); + } } diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java index 12cf7af02b..4b4155c3d7 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/ScalarType.java @@ -704,9 +704,6 @@ public class ScalarType extends Type { case JSONB: stringBuilder.append("JSON"); break; - case AGG_STATE: - stringBuilder.append("AGG_STATE(UNKNOWN)"); - break; default: stringBuilder.append("unknown type: ").append(type); break; @@ -860,17 +857,6 @@ public class ScalarType extends Type { return type == PrimitiveType.CHAR && (len == -1 || len == MAX_CHAR_LENGTH); } - @Override - public boolean isFixedLengthType() { - return type == PrimitiveType.BOOLEAN || type == PrimitiveType.TINYINT - || type == PrimitiveType.SMALLINT || type == PrimitiveType.INT - || type == PrimitiveType.BIGINT || type == PrimitiveType.FLOAT - || type == PrimitiveType.DOUBLE || type == PrimitiveType.DATE - || type == PrimitiveType.DATETIME || type == PrimitiveType.DECIMALV2 || type.isDecimalV3Type() - || type == PrimitiveType.CHAR || type == PrimitiveType.DATEV2 || type == PrimitiveType.DATETIMEV2 - || type == PrimitiveType.TIMEV2; - } - @Override public boolean isSupported() { switch (type) { @@ -882,14 +868,6 @@ public class ScalarType extends Type { } } - @Override - public boolean supportsTablePartitioning() { - if (!isSupported() || isComplexType()) { - return false; - } - return true; - } - @Override public int getSlotSize() { return type.getSlotSize(); diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/StructType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/StructType.java index 4f21286c63..e447f2dcee 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/StructType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/StructType.java @@ -18,7 +18,6 @@ package org.apache.doris.catalog; import org.apache.doris.thrift.TColumnType; -import org.apache.doris.thrift.TStructField; import org.apache.doris.thrift.TTypeDesc; import org.apache.doris.thrift.TTypeNode; import org.apache.doris.thrift.TTypeNodeType; @@ -320,7 +319,7 @@ public class StructType extends Type { Preconditions.checkNotNull(fields); Preconditions.checkState(!fields.isEmpty()); node.setType(TTypeNodeType.STRUCT); - node.setStructFields(new ArrayList()); + node.setStructFields(new ArrayList<>()); for (StructField field : fields) { field.toThrift(container, node); } @@ -342,16 +341,6 @@ public class StructType extends Type { return thrift; } - @Override - public boolean isFixedLengthType() { - return false; - } - - @Override - public boolean supportsTablePartitioning() { - return false; - } - @Override public int getSlotSize() { return PrimitiveType.STRUCT.getSlotSize(); diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java index 258a0f57bf..46cda54753 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java @@ -600,7 +600,7 @@ public abstract class Type { } public boolean isScalarType(PrimitiveType t) { - return isScalarType() && ((ScalarType) this).getPrimitiveType() == t; + return isScalarType() && this.getPrimitiveType() == t; } public boolean isFixedPointType() { @@ -632,11 +632,6 @@ public abstract class Type { return isScalarType(PrimitiveType.LARGEINT); } - // TODO: Handle complex types properly. Some instances may be fixed length. - public boolean isFixedLengthType() { - return false; - } - public boolean isNumericType() { return isFixedPointType() || isFloatingPointType() || isDecimalV2() || isDecimalV3(); } @@ -679,7 +674,7 @@ public abstract class Type { } public boolean isAggStateType() { - return isScalarType(PrimitiveType.AGG_STATE); + return this instanceof AggStateType; } public boolean isMultiRowType() { @@ -771,13 +766,6 @@ public abstract class Type { return -1; } - /** - * Indicates whether we support partitioning tables on columns of this type. - */ - public boolean supportsTablePartitioning() { - return false; - } - public PrimitiveType getPrimitiveType() { return PrimitiveType.INVALID_TYPE; } @@ -862,6 +850,27 @@ public abstract class Type { return true; } else if (sourceType.isStructType() && targetType.isStructType()) { return StructType.canCastTo((StructType) sourceType, (StructType) targetType); + } else if (sourceType.isAggStateType() && targetType.getPrimitiveType().isCharFamily()) { + return true; + } else if (sourceType.isAggStateType() && targetType.isAggStateType()) { + AggStateType sourceAggState = (AggStateType) sourceType; + AggStateType targetAggState = (AggStateType) targetType; + if (!sourceAggState.getFunctionName().equalsIgnoreCase(targetAggState.getFunctionName())) { + return false; + } + if (sourceAggState.getSubTypes().size() != targetAggState.getSubTypes().size()) { + return false; + } + for (int i = 0; i < sourceAggState.getSubTypes().size(); i++) { + // target subtype is not null but source subtype is nullable + if (!targetAggState.getSubTypeNullables().get(i) && sourceAggState.getSubTypeNullables().get(i)) { + return false; + } + if (!canCastTo(sourceAggState.getSubTypes().get(i), targetAggState.getSubTypes().get(i))) { + return false; + } + } + return true; } return sourceType.isNull() || sourceType.getPrimitiveType().isCharFamily(); } @@ -967,7 +976,7 @@ public abstract class Type { MapType mapType = (MapType) this; return mapType.getValueType().exceedsMaxNestingDepth(d + 1); } else { - Preconditions.checkState(isScalarType()); + Preconditions.checkState(isScalarType() || isAggStateType()); } return false; } diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java index e9363dd014..924b197e4d 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/VariantType.java @@ -21,58 +21,20 @@ import org.apache.doris.thrift.TTypeDesc; import org.apache.doris.thrift.TTypeNode; import org.apache.doris.thrift.TTypeNodeType; -public class VariantType extends Type { +public class VariantType extends ScalarType { + public VariantType() { - - } - - @Override - public String toSql(int depth) { - return "VARIANT"; - } - - @Override - protected String prettyPrint(int lpad) { - return "VARIANT"; - } - - @Override - public boolean equals(Object other) { - return other instanceof VariantType; + super(PrimitiveType.VARIANT); } @Override public void toThrift(TTypeDesc container) { + // not use ScalarType's toThrift for compatibility, because VariantType is not extends ScalarType previously TTypeNode node = new TTypeNode(); container.types.add(node); node.setType(TTypeNodeType.VARIANT); } - @Override - public String toString() { - return toSql(0); - } - - @Override - public PrimitiveType getPrimitiveType() { - return PrimitiveType.VARIANT; - } - - @Override - public boolean supportsTablePartitioning() { - return false; - } - - @Override - public int getSlotSize() { - return PrimitiveType.VARIANT.getSlotSize(); - } - - @Override - public boolean isSupported() { - return true; - } - @Override public boolean matchesType(Type t) { return t.isVariantType() || t.isStringType(); diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java index 91fd0dc982..c31bacbf87 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/Expr.java @@ -1922,11 +1922,11 @@ public abstract class Expr extends TreeNode implements ParseNode, Cloneabl f.setNullableMode(NullableMode.ALWAYS_NOT_NULLABLE); } else { Function original = f; - f = ((AggregateFunction) f).clone(); + f = f.clone(); f.setArgs(argList); if (isUnion) { f.setName(new FunctionName(name + AGG_UNION_SUFFIX)); - f.setReturnType((ScalarType) argList.get(0)); + f.setReturnType(argList.get(0)); f.setNullableMode(NullableMode.ALWAYS_NOT_NULLABLE); } if (isMerge) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/types/AggStateType.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/types/AggStateType.java index e7d8dbdfe3..ffda27e7ba 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/types/AggStateType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/types/AggStateType.java @@ -78,7 +78,7 @@ public class AggStateType extends DataType { @Override public Type toCatalogDataType() { - List types = subTypes.stream().map(t -> t.toCatalogDataType()).collect(Collectors.toList()); + List types = subTypes.stream().map(DataType::toCatalogDataType).collect(Collectors.toList()); return Expr.createAggStateType(functionName, types, subTypeNullables); } diff --git a/regression-test/suites/datatype_p0/agg_state/max/test_agg_state_max.groovy b/regression-test/suites/datatype_p0/agg_state/max/test_agg_state_max.groovy index 4bcc98a8b2..86618fd0ba 100644 --- a/regression-test/suites/datatype_p0/agg_state/max/test_agg_state_max.groovy +++ b/regression-test/suites/datatype_p0/agg_state/max/test_agg_state_max.groovy @@ -30,7 +30,7 @@ suite("test_agg_state_max") { test { sql "insert into a_table values(100,max_state(null));" - exception "which is illegal for non_nullable" + exception "can not cast from origin type AGG_STATE to target type=AGG_STATE" } sql """insert into a_table