morrySnow commented on code in PR #68853: URL: https://github.com/apache/doris/pull/68853#discussion_r4491295566
########## fe/fe-type/src/main/java/org/apache/doris/catalog/FileType.java: ########## @@ -0,0 +1,176 @@ +// 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.catalog; + +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonPreProcessable; +import org.apache.doris.thrift.TColumnType; +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.common.collect.ImmutableList; +import com.google.gson.annotations.SerializedName; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +/** + * Independent FILE identity with six canonical nullable physical children. + * Scan selection is separate metadata and never changes this schema. + */ +public final class FileType extends Type implements GsonPostProcessable, GsonPreProcessable { + public static final int FIELD_COUNT = 6; + // JSON strings can expand sixfold; inline uses padded Base64. MySQL column_length is uint32. + public static final long JSON_DISPLAY_LENGTH = Math.min(0xffff_ffffL, + 6L * (65533 + 1024 + 1024) + 128 + 4L * ((ScalarType.MAX_VARBINARY_LENGTH + 2L) / 3)); + + @SerializedName("fields") + private List<StructField> fields; + + public FileType(List<StructField> fields) { + validateFields(fields); + this.fields = canonicalFields(); + } + + public static FileType create() { + return new FileType(canonicalFields()); + } + + private static List<StructField> canonicalFields() { + return ImmutableList.of( + new StructField("uri", ScalarType.createVarcharType(65533)), + new StructField("offset", Type.BIGINT), + new StructField("size", Type.BIGINT), + new StructField("content_type", ScalarType.createVarcharType(1024)), + new StructField("checksum", ScalarType.createVarcharType(1024)), + new StructField("inline", ScalarType.createVarbinaryType(ScalarType.MAX_VARBINARY_LENGTH))); + } + + /** Return independent field descriptors so callers cannot mutate the fixed schema. */ + public List<StructField> getFields() { + return canonicalFields(); + } + + public StructField getField(String name) { + String normalized = name.toLowerCase(Locale.ROOT); + for (StructField field : getFields()) { + if (field.getName().equals(normalized)) { + return field; + } + } + return null; + } + + public static void validateFields(List<StructField> fields) { + Preconditions.checkArgument(fields != null && fields.size() == FIELD_COUNT, + "FILE requires six canonical nullable children"); + List<StructField> expected = canonicalFields(); + for (int i = 0; i < FIELD_COUNT; i++) { + StructField field = fields.get(i); + StructField canonical = expected.get(i); + Preconditions.checkArgument(field != null + && canonical.getName().equals(field.getName()) + && canonical.getName().equals(field.getOriginalName()) + && canonical.getType().equals(field.getType()) + && canonical.getType().getLength() == field.getType().getLength() + && field.getContainsNull(), + "Invalid FILE child at position %s; expected %s", i, canonical.getName()); + } + } + + @Override + public void gsonPostProcess() { + validateFields(fields); + fields = canonicalFields(); + } Review Comment: What is the significance of this operation? ########## fe/fe-type/src/main/java/org/apache/doris/catalog/FileType.java: ########## @@ -0,0 +1,176 @@ +// 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.catalog; + +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonPreProcessable; +import org.apache.doris.thrift.TColumnType; +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.common.collect.ImmutableList; +import com.google.gson.annotations.SerializedName; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +/** + * Independent FILE identity with six canonical nullable physical children. + * Scan selection is separate metadata and never changes this schema. + */ +public final class FileType extends Type implements GsonPostProcessable, GsonPreProcessable { + public static final int FIELD_COUNT = 6; + // JSON strings can expand sixfold; inline uses padded Base64. MySQL column_length is uint32. + public static final long JSON_DISPLAY_LENGTH = Math.min(0xffff_ffffL, + 6L * (65533 + 1024 + 1024) + 128 + 4L * ((ScalarType.MAX_VARBINARY_LENGTH + 2L) / 3)); + + @SerializedName("fields") + private List<StructField> fields; + + public FileType(List<StructField> fields) { + validateFields(fields); + this.fields = canonicalFields(); + } + + public static FileType create() { + return new FileType(canonicalFields()); + } + + private static List<StructField> canonicalFields() { + return ImmutableList.of( + new StructField("uri", ScalarType.createVarcharType(65533)), + new StructField("offset", Type.BIGINT), + new StructField("size", Type.BIGINT), + new StructField("content_type", ScalarType.createVarcharType(1024)), + new StructField("checksum", ScalarType.createVarcharType(1024)), + new StructField("inline", ScalarType.createVarbinaryType(ScalarType.MAX_VARBINARY_LENGTH))); + } + + /** Return independent field descriptors so callers cannot mutate the fixed schema. */ + public List<StructField> getFields() { + return canonicalFields(); + } + + public StructField getField(String name) { + String normalized = name.toLowerCase(Locale.ROOT); + for (StructField field : getFields()) { + if (field.getName().equals(normalized)) { + return field; + } + } + return null; + } + + public static void validateFields(List<StructField> fields) { + Preconditions.checkArgument(fields != null && fields.size() == FIELD_COUNT, + "FILE requires six canonical nullable children"); + List<StructField> expected = canonicalFields(); + for (int i = 0; i < FIELD_COUNT; i++) { + StructField field = fields.get(i); + StructField canonical = expected.get(i); + Preconditions.checkArgument(field != null + && canonical.getName().equals(field.getName()) + && canonical.getName().equals(field.getOriginalName()) + && canonical.getType().equals(field.getType()) + && canonical.getType().getLength() == field.getType().getLength() + && field.getContainsNull(), + "Invalid FILE child at position %s; expected %s", i, canonical.getName()); + } + } + + @Override + public void gsonPostProcess() { + validateFields(fields); + fields = canonicalFields(); + } + + @Override + public void gsonPreProcess() { + validateFields(fields); + } Review Comment: What is the significance of this operation? ########## fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java: ########## @@ -1014,6 +1040,8 @@ protected static Pair<Type, Integer> fromThrift(TTypeDesc col, int nodeIdx) { } else if (scalarType.getType() == TPrimitiveType.VARCHAR) { Preconditions.checkState(scalarType.isSetLen()); type = ScalarType.createVarcharType(scalarType.getLen()); + } else if (scalarType.getType() == TPrimitiveType.FILE) { + throw new IllegalArgumentException("FILE must use a FILE type node with six children"); Review Comment: When will the `PrimitiveType` of `ScalarType` be set to `FILE`? ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -456,6 +459,11 @@ public void createChildrenColumn(Type type, Column column) { c.setIsAllowNull(field.getContainsNull()); column.addChildrenColumn(c); } + } else if (type.isFileType()) { + for (StructField field : ((FileType) type).getFields()) { + Column child = new Column(field.getName(), field.getType(), true); Review Comment: allowNull should obtain from field ########## fe/fe-type/src/main/java/org/apache/doris/catalog/Type.java: ########## @@ -281,6 +284,7 @@ public abstract class Type { structSubTypes.add(ARRAY); structSubTypes.add(MAP); structSubTypes.add(STRUCT); + structSubTypes.add(FILE); Review Comment: struct<file> could be used in `order by`? ########## fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnToThrift.java: ########## @@ -40,6 +40,7 @@ public static void setIndexFlag(TColumn tColumn, OlapTable olapTable) { } public static TColumn toThrift(Column column) { + column.validateFileSchema(); Review Comment: why add this? ########## fe/fe-core/src/main/java/org/apache/doris/catalog/HashDistributionInfo.java: ########## @@ -65,6 +65,9 @@ public static void checkDistributionColumnType(String columnName, Type type) thr if (type.isStructType()) { throw new DdlException("Struct Type should not be used in distribution column[" + columnName + "]."); } + if (type.typeContainsFile()) { + throw new DdlException("FILE cannot be used in distribution column[" + columnName + "]"); Review Comment: ```suggestion throw new DdlException("FILE Type should not be used in distribution column[" + columnName + "]"); ``` ########## fe/fe-core/src/main/java/org/apache/doris/nereids/load/NereidsStreamLoadPlanner.java: ########## @@ -271,6 +272,7 @@ public TPipelineFragmentParams plan(TUniqueId loadId, int fragmentInstanceIdInde TPipelineFragmentParams params = new TPipelineFragmentParams(); params.setProtocolVersion(PaloInternalServiceVersion.V1); + params.setS3Resources(ThriftPlansBuilder.collectS3Resources(connectContext)); Review Comment: A design document is required to explain the working method of the "file" type. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAnalysis.java: ########## @@ -144,6 +151,35 @@ public List<Rule> buildRules() { ); } + /** Validate operator keys after expression binding. */ + public static void checkFilePlan(Plan plan) { + if (plan instanceof LogicalSort) { + ((LogicalSort<?>) plan).getOrderKeys() + .forEach(key -> ExpressionUtils.checkFileKey(key.getExpr(), "ORDER BY")); + } + if (plan instanceof LogicalAggregate) { + ((LogicalAggregate<?>) plan).getGroupByExpressions() + .forEach(key -> ExpressionUtils.checkFileKey(key, "GROUP BY")); + } + if (plan instanceof LogicalRepeat) { + ((LogicalRepeat<?>) plan).getGroupByExpressions() + .forEach(key -> ExpressionUtils.checkFileKey(key, "GROUPING SETS")); + } + if (plan instanceof LogicalSetOperation + && (!(plan instanceof LogicalUnion) || ((LogicalSetOperation) plan).getQualifier() != Qualifier.ALL)) { + plan.getOutput().forEach(key -> ExpressionUtils.checkFileKey(key, plan.getType().toString())); + } + } + + private void checkArrowFlightResult(Plan plan, ConnectContext connectContext) { Review Comment: It's rather strange to conduct protocol checks here, because it cannot be guaranteed that all queries will pass through this point. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java: ########## @@ -191,7 +191,11 @@ public TypeCheckResult checkInputDataTypes() { return commonCheckResult; } } - return checkInputDataTypesInternal(); + TypeCheckResult result = checkInputDataTypesInternal(); + if (result.success()) { + ExpressionUtils.checkFileExpression(this); + } Review Comment: Please integrate the code logic and avoid the occurrence of difficult-to-maintain special case branches. ########## fe/fe-type/src/main/java/org/apache/doris/catalog/FileType.java: ########## @@ -0,0 +1,176 @@ +// 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.catalog; + +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonPreProcessable; +import org.apache.doris.thrift.TColumnType; +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.common.collect.ImmutableList; +import com.google.gson.annotations.SerializedName; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +/** + * Independent FILE identity with six canonical nullable physical children. + * Scan selection is separate metadata and never changes this schema. + */ +public final class FileType extends Type implements GsonPostProcessable, GsonPreProcessable { + public static final int FIELD_COUNT = 6; + // JSON strings can expand sixfold; inline uses padded Base64. MySQL column_length is uint32. + public static final long JSON_DISPLAY_LENGTH = Math.min(0xffff_ffffL, + 6L * (65533 + 1024 + 1024) + 128 + 4L * ((ScalarType.MAX_VARBINARY_LENGTH + 2L) / 3)); + + @SerializedName("fields") + private List<StructField> fields; + + public FileType(List<StructField> fields) { + validateFields(fields); + this.fields = canonicalFields(); + } + + public static FileType create() { + return new FileType(canonicalFields()); + } + + private static List<StructField> canonicalFields() { + return ImmutableList.of( + new StructField("uri", ScalarType.createVarcharType(65533)), + new StructField("offset", Type.BIGINT), + new StructField("size", Type.BIGINT), + new StructField("content_type", ScalarType.createVarcharType(1024)), + new StructField("checksum", ScalarType.createVarcharType(1024)), + new StructField("inline", ScalarType.createVarbinaryType(ScalarType.MAX_VARBINARY_LENGTH))); + } + + /** Return independent field descriptors so callers cannot mutate the fixed schema. */ + public List<StructField> getFields() { + return canonicalFields(); + } + + public StructField getField(String name) { + String normalized = name.toLowerCase(Locale.ROOT); + for (StructField field : getFields()) { + if (field.getName().equals(normalized)) { + return field; + } + } + return null; + } + + public static void validateFields(List<StructField> fields) { + Preconditions.checkArgument(fields != null && fields.size() == FIELD_COUNT, + "FILE requires six canonical nullable children"); + List<StructField> expected = canonicalFields(); + for (int i = 0; i < FIELD_COUNT; i++) { + StructField field = fields.get(i); + StructField canonical = expected.get(i); + Preconditions.checkArgument(field != null + && canonical.getName().equals(field.getName()) + && canonical.getName().equals(field.getOriginalName()) + && canonical.getType().equals(field.getType()) + && canonical.getType().getLength() == field.getType().getLength() + && field.getContainsNull(), + "Invalid FILE child at position %s; expected %s", i, canonical.getName()); + } + } + + @Override + public void gsonPostProcess() { + validateFields(fields); + fields = canonicalFields(); + } + + @Override + public void gsonPreProcess() { + validateFields(fields); + } + + @Override + public PrimitiveType getPrimitiveType() { + return PrimitiveType.FILE; + } + + @Override + protected String toSql(int depth) { + return "FILE"; + } + + @Override + protected String prettyPrint(int lpad) { + return Strings.repeat(" ", lpad) + "FILE"; + } + + @Override + public String toString() { + return "FILE"; + } + + @Override + public boolean matchesType(Type other) { + return equals(other) || (other.isAnyType() && other.matchesType(this)); + } + + @Override + public boolean equals(Object other) { + return other instanceof FileType; + } + + @Override + public int hashCode() { + return FileType.class.hashCode(); + } Review Comment: What is the purpose of having different hashcodes for two `FileType` instances? ########## fe/fe-type/src/main/java/org/apache/doris/catalog/FileType.java: ########## @@ -0,0 +1,176 @@ +// 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.catalog; + +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonPreProcessable; +import org.apache.doris.thrift.TColumnType; +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.common.collect.ImmutableList; +import com.google.gson.annotations.SerializedName; + +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; + +/** + * Independent FILE identity with six canonical nullable physical children. + * Scan selection is separate metadata and never changes this schema. + */ +public final class FileType extends Type implements GsonPostProcessable, GsonPreProcessable { + public static final int FIELD_COUNT = 6; + // JSON strings can expand sixfold; inline uses padded Base64. MySQL column_length is uint32. + public static final long JSON_DISPLAY_LENGTH = Math.min(0xffff_ffffL, + 6L * (65533 + 1024 + 1024) + 128 + 4L * ((ScalarType.MAX_VARBINARY_LENGTH + 2L) / 3)); + + @SerializedName("fields") + private List<StructField> fields; + + public FileType(List<StructField> fields) { + validateFields(fields); + this.fields = canonicalFields(); + } + + public static FileType create() { + return new FileType(canonicalFields()); + } + + private static List<StructField> canonicalFields() { + return ImmutableList.of( + new StructField("uri", ScalarType.createVarcharType(65533)), Review Comment: Why use varchar instead of string? ########## fe/fe-type/src/main/java/org/apache/doris/catalog/ScalarType.java: ########## @@ -1098,4 +1101,14 @@ public boolean getVariantEnableNestedGroup() { return false; // Default to false for backward compatibility. } + @Override + public void gsonPostProcess() { + Preconditions.checkArgument(type != PrimitiveType.FILE, "FILE requires its independent FileType"); + } + + @Override + public void gsonPreProcess() { + Preconditions.checkArgument(type != PrimitiveType.FILE, "FILE requires its independent FileType"); + } Review Comment: why add these codes? ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -1115,8 +1124,28 @@ public boolean hasDefaultValue() { return defaultValue != null || realDefaultValue != null || defaultValueExprDef != null; } + /** Verify redundant column children agree with the canonical FILE type. */ + public void validateFileSchema() { + if (type.isFileType()) { + Preconditions.checkArgument(children != null && children.size() == FileType.FIELD_COUNT, + "FILE column requires six canonical children"); + List<StructField> fields = new ArrayList<>(); + for (int i = 0; i < children.size(); i++) { + Column child = children.get(i); + fields.add(new StructField(child.getName(), child.getType(), null, child.isAllowNull())); + } + FileType.validateFields(fields); + } + } + + @Override + public void gsonPreProcess() { + validateFileSchema(); Review Comment: why need this check? ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -266,11 +268,12 @@ public Column() { } public Column(String name, PrimitiveType dataType) { - this(name, ScalarType.createType(dataType), false, null, false, null, ""); + this(name, dataType == PrimitiveType.FILE ? Type.FILE : ScalarType.createType(dataType), + false, null, false, null, ""); } public Column(String name, PrimitiveType dataType, boolean isAllowNull) { - this(name, ScalarType.createType(dataType), isAllowNull); + this(name, dataType == PrimitiveType.FILE ? Type.FILE : ScalarType.createType(dataType), isAllowNull); Review Comment: This is the constructor for ScalarType. Do not use this constructor for FileType. ########## fe/fe-type/src/main/java/org/apache/doris/catalog/ScalarType.java: ########## @@ -128,6 +130,7 @@ public long getByteSize() { private String lenStr; public ScalarType(PrimitiveType type) { + Preconditions.checkArgument(type != PrimitiveType.FILE, "FILE requires its independent FileType"); Review Comment: why add this line? ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -1115,8 +1124,28 @@ public boolean hasDefaultValue() { return defaultValue != null || realDefaultValue != null || defaultValueExprDef != null; } + /** Verify redundant column children agree with the canonical FILE type. */ + public void validateFileSchema() { + if (type.isFileType()) { + Preconditions.checkArgument(children != null && children.size() == FileType.FIELD_COUNT, + "FILE column requires six canonical children"); Review Comment: why check it outside again? not all in `FileType.validateFields(fields)`? ########## fe/fe-core/src/main/java/org/apache/doris/nereids/util/TypeCoercionUtils.java: ########## @@ -1593,6 +1593,7 @@ private static Expression processComparisonPredicateInternal(ComparisonPredicate */ public static Expression processInPredicate(InPredicate inPredicate) { // check + inPredicate.children().forEach(argument -> ExpressionUtils.checkFileKey(argument, "IN")); Review Comment: It should be included in the internal checks of the InPredicate itself. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java: ########## @@ -986,6 +1012,9 @@ private static void validateCatalogDataType(Type catalogType) { } } } + } else if (catalogType.isAggStateType() && catalogType.typeContainsFile()) { + throw new AnalysisException("AGG_STATE does not support FILE or a type containing FILE: " + + catalogType.toSql()); Review Comment: This check seems rather strange. If the file does not have an aggregation function, it naturally won't be included in the agg_state type. ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -266,11 +268,12 @@ public Column() { } public Column(String name, PrimitiveType dataType) { - this(name, ScalarType.createType(dataType), false, null, false, null, ""); + this(name, dataType == PrimitiveType.FILE ? Type.FILE : ScalarType.createType(dataType), Review Comment: This is the constructor for ScalarType. Do not use this constructor for FileType. ########## fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java: ########## @@ -1115,8 +1124,28 @@ public boolean hasDefaultValue() { return defaultValue != null || realDefaultValue != null || defaultValueExprDef != null; } + /** Verify redundant column children agree with the canonical FILE type. */ + public void validateFileSchema() { + if (type.isFileType()) { + Preconditions.checkArgument(children != null && children.size() == FileType.FIELD_COUNT, + "FILE column requires six canonical children"); + List<StructField> fields = new ArrayList<>(); + for (int i = 0; i < children.size(); i++) { + Column child = children.get(i); + fields.add(new StructField(child.getName(), child.getType(), null, child.isAllowNull())); + } + FileType.validateFields(fields); + } + } + + @Override + public void gsonPreProcess() { + validateFileSchema(); + } + @Override public void gsonPostProcess() throws IOException { + validateFileSchema(); Review Comment: why need this check? ########## fe/fe-core/src/main/java/org/apache/doris/alter/MaterializedViewHandler.java: ########## @@ -629,6 +629,9 @@ private List<Column> checkAndPrepareMaterializedView(CreateMaterializedViewComma } for (Column column : newMVColumns) { + if (column.getType().typeContainsFile()) { + throw new DdlException("FILE columns are not supported in internal tables"); Review Comment: The wording similar to the error message below should be used. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java: ########## @@ -783,6 +784,12 @@ public Expr visitSearchExpression(SearchExpression searchExpression, return searchPredicate; } + @Override + public Expr visitToFile(ToFile toFile, PlanTranslatorContext context) { + context.getConnectContext().getOrRegisterS3Resource(toFile.getResourceName()); Review Comment: should not register on ConnectContext, should in PlanTranslatorContext ########## fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnToThrift.java: ########## @@ -130,6 +131,7 @@ public static TAggregationType toThrift(AggregateType aggregateType) { } public static void setChildrenTColumn(Column children, TColumn tColumn) { + children.validateFileSchema(); Review Comment: why add this? ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java: ########## @@ -1158,6 +1162,10 @@ private Plan bindProject(MatchingContext<LogicalProject<Plan>> ctx) { } } project = project.withProjects(boundProjectionsBuilder.build()); + if (project.isDistinct()) { + project.getProjects().forEach(value -> ExpressionUtils.checkFileKey( + value, "SELECT DISTINCT")); + } Review Comment: Strange code. If the file does not allow performing distinct operations, simply checking for errors in the subsequent aggregation section will suffice. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java: ########## @@ -448,6 +449,9 @@ private LogicalPlan bindGenerate(MatchingContext<LogicalGenerate<Plan>> ctx) { if (generate.getExpandColumnAlias() != null && i < generate.getExpandColumnAlias().size() && !CollectionUtils.isEmpty(generate.getExpandColumnAlias().get(i))) { int aliasCount = generate.getExpandColumnAlias().get(i).size(); + if (boundGenerator instanceof ExplodeFile && aliasCount != 6) { Review Comment: Why are there so many inquiries about the 6-field check for the "file type"? ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateMTMVInfo.java: ########## @@ -191,7 +191,12 @@ public void analyze(ConnectContext ctx) throws Exception { // analyze distribute Map<String, ColumnDefinition> columnMap = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - columns.forEach(c -> columnMap.put(c.getName(), c)); + columns.forEach(c -> { + if (c.getType().typeContainsFile()) { + throw new AnalysisException("FILE columns are not supported in internal tables"); Review Comment: The error message should be relevant to the context. Additionally, if it is not supported to create an internal table, the error should not be reported here but should be uniformly checked at the point of creating the internal table. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ColumnDefinition.java: ########## @@ -433,6 +434,10 @@ private void validateInternal(boolean isOlap, Set<String> keysSet, Set<String> c "Cannot use `VARIANT` type column named with a `.` character: " + name); } type.validateDataType(); + if (type.typeContainsFile()) { + throw new AnalysisException(isOlap ? "FILE columns are not supported in internal tables" + : "FILE columns are not supported in external tables"); + } Review Comment: should merged in validateDataType ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/check/CheckCast.java: ########## @@ -395,6 +395,10 @@ public static boolean check(DataType originalType, DataType targetType, boolean */ public static boolean check(DataType originalType, DataType targetType, boolean isStrictMode, boolean looseAggState) { + if (originalType.typeContainsFile() || targetType.typeContainsFile()) { + return !targetType.isAggStateType() + && (originalType.isNullType() || originalType.equals(targetType)); + } Review Comment: Code with a rather convoluted logic -- 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]
