This is an automated email from the ASF dual-hosted git repository. panxiaolei pushed a commit to branch branch-2.0 in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-2.0 by this push: new 16943adbc3 [Bug](agg-state) fix core dump on not nullable argument for aggstate's nested argument (#21331) (#21500) 16943adbc3 is described below commit 16943adbc3aefbcaf0bf6e0d9d5fddbee53b0901 Author: Pxl <pxl...@qq.com> AuthorDate: Wed Jul 5 10:59:52 2023 +0800 [Bug](agg-state) fix core dump on not nullable argument for aggstate's nested argument (#21331) (#21500) fix core dump on not nullable argument for aggstate's nested argument --- be/src/vec/exprs/vectorized_agg_fn.cpp | 21 +++++++++++--- .../org/apache/doris/analysis/ArithmeticExpr.java | 2 +- .../apache/doris/analysis/NativeInsertStmt.java | 22 ++++++++++----- .../doris/catalog/MaterializedIndexMeta.java | 3 -- .../functions/AggStateFunctionBuilder.java | 13 ++------- .../functions/combinator/MergeCombinator.java | 6 ++-- .../functions/combinator/StateCombinator.java | 2 +- .../functions/combinator/UnionCombinator.java | 6 ++-- .../apache/doris/nereids/types/AggStateType.java | 11 ++++++-- .../java/org/apache/doris/analysis/ExprTest.java | 2 +- .../org/apache/doris/analysis/LoadStmtTest.java | 4 +-- .../org/apache/doris/planner/RepeatNodeTest.java | 4 +-- .../agg_state/max/test_agg_state_max.out | 33 ++++++++++++++++++++++ regression-test/data/mv_p0/k1ap2spa/k1ap2spa.out | 2 ++ .../agg_state/max/test_agg_state_max.groovy | 27 ++++++++++++++++-- .../suites/mv_p0/k1ap2spa/k1ap2spa.groovy | 1 + 16 files changed, 114 insertions(+), 45 deletions(-) diff --git a/be/src/vec/exprs/vectorized_agg_fn.cpp b/be/src/vec/exprs/vectorized_agg_fn.cpp index a6895ff728..2dcf12207a 100644 --- a/be/src/vec/exprs/vectorized_agg_fn.cpp +++ b/be/src/vec/exprs/vectorized_agg_fn.cpp @@ -168,7 +168,12 @@ Status AggFnEvaluator::prepare(RuntimeState* state, const RowDescriptor& desc, "Agg state function input type must be agg_state but get {}", argument_types[0]->get_family_name()); } - if (match_suffix(_fn.name.function_name, AGG_UNION_SUFFIX)) { + + std::string type_function_name = + assert_cast<const DataTypeAggState*>(argument_types[0].get()) + ->get_nested_function() + ->get_name(); + if (type_function_name + AGG_UNION_SUFFIX == _fn.name.function_name) { if (_data_type->is_nullable()) { return Status::InternalError( "Union function return type must be not nullable, real={}", @@ -180,11 +185,19 @@ Status AggFnEvaluator::prepare(RuntimeState* state, const RowDescriptor& desc, _data_type->get_name()); } _function = get_agg_state_function<AggregateStateUnion>(argument_types, _data_type); - } else if (match_suffix(_fn.name.function_name, AGG_MERGE_SUFFIX)) { + } else if (type_function_name + AGG_MERGE_SUFFIX == _fn.name.function_name) { + auto type = assert_cast<const DataTypeAggState*>(argument_types[0].get()) + ->get_nested_function() + ->get_return_type(); + if (!type->equals(*_data_type)) { + return Status::InternalError("{}'s expect return type is {}, but input {}", + argument_types[0]->get_name(), type->get_name(), + _data_type->get_name()); + } _function = get_agg_state_function<AggregateStateMerge>(argument_types, _data_type); } else { - return Status::InternalError( - "Aggregate Function {} is not endwith '_merge' or '_union'", _fn.signature); + return Status::InternalError("{} not match function {}", argument_types[0]->get_name(), + _fn.name.function_name); } } else { _function = AggregateFunctionSimpleFactory::instance().get( diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/ArithmeticExpr.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/ArithmeticExpr.java index de437e154c..176a55fb02 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/ArithmeticExpr.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/ArithmeticExpr.java @@ -260,7 +260,7 @@ public class ArithmeticExpr extends Expr { if (children.size() == 1) { return op.toString() + " " + getChild(0).toSql(); } else { - return getChild(0).toSql() + " " + op.toString() + " " + getChild(1).toSql(); + return "(" + getChild(0).toSql() + " " + op.toString() + " " + getChild(1).toSql() + ")"; } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java index fcd85fd80b..408fcc0952 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/NativeInsertStmt.java @@ -437,9 +437,7 @@ public class NativeInsertStmt extends InsertStmt { mentionedColumns.add(col.getName()); targetColumns.add(col); } - realTargetColumnNames = targetColumns.stream().map(column -> column.getName()).collect(Collectors.toList()); } else { - realTargetColumnNames = targetColumnNames; for (String colName : targetColumnNames) { Column col = targetTable.getColumn(colName); if (col == null) { @@ -453,8 +451,8 @@ public class NativeInsertStmt extends InsertStmt { // hll column mush in mentionedColumns for (Column col : targetTable.getBaseSchema()) { if (col.getType().isObjectStored() && !mentionedColumns.contains(col.getName())) { - throw new AnalysisException(" object-stored column " + col.getName() - + " mush in insert into columns"); + throw new AnalysisException( + " object-stored column " + col.getName() + " mush in insert into columns"); } } } @@ -535,20 +533,30 @@ public class NativeInsertStmt extends InsertStmt { } // check if size of select item equal with columns mentioned in statement - if (mentionedColumns.size() != queryStmt.getResultExprs().size() - || realTargetColumnNames.size() != queryStmt.getResultExprs().size()) { + if (mentionedColumns.size() != queryStmt.getResultExprs().size()) { ErrorReport.reportAnalysisException(ErrorCode.ERR_WRONG_VALUE_COUNT); } // Check if all columns mentioned is enough checkColumnCoverage(mentionedColumns, targetTable.getBaseSchema()); + realTargetColumnNames = targetColumns.stream().map(column -> column.getName()).collect(Collectors.toList()); Map<String, Expr> slotToIndex = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER); - for (int i = 0; i < realTargetColumnNames.size(); i++) { + for (int i = 0; i < queryStmt.getResultExprs().size(); i++) { slotToIndex.put(realTargetColumnNames.get(i), queryStmt.getResultExprs().get(i) .checkTypeCompatibility(targetTable.getColumn(realTargetColumnNames.get(i)).getType())); } + for (Column column : targetTable.getBaseSchema()) { + if (!slotToIndex.containsKey(column.getName())) { + if (column.getDefaultValue() == null) { + slotToIndex.put(column.getName(), new NullLiteral()); + } else { + slotToIndex.put(column.getName(), new StringLiteral(column.getDefaultValue())); + } + } + } + // handle VALUES() or SELECT constant list if (isValuesOrConstantSelect) { SelectStmt selectStmt = (SelectStmt) queryStmt; diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java index 07c33b85bc..12cab9f7ec 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java @@ -193,9 +193,6 @@ public class MaterializedIndexMeta implements Writable, GsonPostProcessable { // mv_count_sale_amt -> mva_SUM__CASE WHEN `sale_amt` IS NULL THEN 0 ELSE 1 END List<SlotRef> slots = new ArrayList<>(); entry.getValue().collect(SlotRef.class, slots); - if (slots.size() > 1) { - throw new IOException("DefineExpr have multiple slot in MaterializedIndex, Expr=" + entry.getKey()); - } String name = MaterializedIndexMeta.normalizeName(slots.get(0).toSqlWithoutTbl()); Column matchedColumn = null; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggStateFunctionBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggStateFunctionBuilder.java index e07f263040..054aa4767b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggStateFunctionBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggStateFunctionBuilder.java @@ -18,15 +18,12 @@ package org.apache.doris.nereids.trees.expressions.functions; import org.apache.doris.nereids.trees.expressions.Expression; -import org.apache.doris.nereids.trees.expressions.SlotReference; import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; import org.apache.doris.nereids.trees.expressions.functions.combinator.MergeCombinator; import org.apache.doris.nereids.trees.expressions.functions.combinator.StateCombinator; import org.apache.doris.nereids.trees.expressions.functions.combinator.UnionCombinator; import org.apache.doris.nereids.types.AggStateType; -import com.google.common.collect.ImmutableList; - import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -66,9 +63,7 @@ public class AggStateFunctionBuilder extends FunctionBuilder { return false; } - return nestedBuilder.canApply(((AggStateType) argument.getDataType()).getSubTypes().stream().map(t -> { - return new SlotReference("mocked", t); - }).collect(ImmutableList.toImmutableList())); + return nestedBuilder.canApply(((AggStateType) argument.getDataType()).getMockedExpressions()); } } @@ -95,11 +90,7 @@ public class AggStateFunctionBuilder extends FunctionBuilder { Expression arg = (Expression) arguments.get(0); AggStateType type = (AggStateType) arg.getDataType(); - List<Expression> nestedArgumens = type.getSubTypes().stream().map(t -> { - return new SlotReference("mocked", t); - }).collect(Collectors.toList()); - - return (AggregateFunction) nestedBuilder.build(nestedName, nestedArgumens); + return (AggregateFunction) nestedBuilder.build(nestedName, type.getMockedExpressions()); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/MergeCombinator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/MergeCombinator.java index 69b9514f13..b529ae2de3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/MergeCombinator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/MergeCombinator.java @@ -32,7 +32,6 @@ import com.google.common.collect.ImmutableList; import java.util.List; import java.util.Objects; -import java.util.stream.Collectors; /** * AggState combinator merge @@ -47,13 +46,12 @@ public class MergeCombinator extends AggregateFunction super(nested.getName() + AggStateFunctionBuilder.MERGE_SUFFIX, arguments); this.nested = Objects.requireNonNull(nested, "nested can not be null"); - inputType = new AggStateType(nested.getName(), nested.getArgumentsTypes(), - nested.getArguments().stream().map(Expression::nullable).collect(Collectors.toList())); + inputType = (AggStateType) arguments.get(0).getDataType(); } @Override public MergeCombinator withChildren(List<Expression> children) { - return new MergeCombinator(children, nested.withChildren(children)); + return new MergeCombinator(children, nested); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java index d7da86c525..9b97a7afd4 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java @@ -59,7 +59,7 @@ public class StateCombinator extends ScalarFunction @Override public StateCombinator withChildren(List<Expression> children) { - return new StateCombinator(children, nested.withChildren(children)); + return new StateCombinator(children, nested); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/UnionCombinator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/UnionCombinator.java index 21261998ec..67f09a50eb 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/UnionCombinator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/UnionCombinator.java @@ -32,7 +32,6 @@ import com.google.common.collect.ImmutableList; import java.util.List; import java.util.Objects; -import java.util.stream.Collectors; /** * AggState combinator union @@ -47,13 +46,12 @@ public class UnionCombinator extends AggregateFunction super(nested.getName() + AggStateFunctionBuilder.UNION_SUFFIX, arguments); this.nested = Objects.requireNonNull(nested, "nested can not be null"); - inputType = new AggStateType(nested.getName(), nested.getArgumentsTypes(), - nested.getArguments().stream().map(Expression::nullable).collect(Collectors.toList())); + inputType = (AggStateType) arguments.get(0).getDataType(); } @Override public UnionCombinator withChildren(List<Expression> children) { - return new UnionCombinator(children, nested.withChildren(children)); + return new UnionCombinator(children, nested); } @Override 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 cc467b0eb6..a999512d1b 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 @@ -19,11 +19,14 @@ package org.apache.doris.nereids.types; import org.apache.doris.analysis.Expr; import org.apache.doris.catalog.Type; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.SlotReference; import org.apache.doris.nereids.types.coercion.AbstractDataType; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; +import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; @@ -56,8 +59,12 @@ public class AggStateType extends DataType { this.functionName = functionName; } - public List<DataType> getSubTypes() { - return subTypes; + public List<Expression> getMockedExpressions() { + List<Expression> result = new ArrayList<Expression>(); + for (int i = 0; i < subTypes.size(); i++) { + result.add(new SlotReference("mocked", subTypes.get(i), subTypeNullables.get(i))); + } + return result; } @Override diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/ExprTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/ExprTest.java index 5262a29034..a91199189a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/ExprTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/ExprTest.java @@ -264,7 +264,7 @@ public class ExprTest { DataInputStream dis = new DataInputStream(new FileInputStream(file)); Expr readExpr = Expr.readIn(dis); Assert.assertTrue(readExpr instanceof ArithmeticExpr); - Assert.assertEquals("cos(1) + 100 / 200", readExpr.toSql()); + Assert.assertEquals("(cos(1) + (100 / 200))", readExpr.toSql()); // 3. delete files dis.close(); diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/LoadStmtTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/LoadStmtTest.java index 59070dc80c..e8f8309867 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/LoadStmtTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/LoadStmtTest.java @@ -150,13 +150,13 @@ public class LoadStmtTest { columnDescs.descs = columns1; columnDescs.isColumnDescsRewrited = false; Load.rewriteColumns(columnDescs); - String orig = "`c1` + 1 + 1"; + String orig = "((`c1` + 1) + 1)"; Assert.assertEquals(orig, columns1.get(4).getExpr().toString()); List<ImportColumnDesc> columns2 = getColumns("c1,c2,c3,tmp_c5 = tmp_c4+1, tmp_c4=c1 + 1"); columnDescs.descs = columns2; columnDescs.isColumnDescsRewrited = false; - String orig2 = "`tmp_c4` + 1"; + String orig2 = "(`tmp_c4` + 1)"; Load.rewriteColumns(columnDescs); Assert.assertEquals(orig2, columns2.get(3).getExpr().toString()); diff --git a/fe/fe-core/src/test/java/org/apache/doris/planner/RepeatNodeTest.java b/fe/fe-core/src/test/java/org/apache/doris/planner/RepeatNodeTest.java index 1687d0c1a6..a4a09d0870 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/planner/RepeatNodeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/planner/RepeatNodeTest.java @@ -67,9 +67,9 @@ public class RepeatNodeTest extends TestWithFeService { String sql2 = "select /*+ SET_VAR(enable_nereids_planner=false) */ (id + 1) id_, name, sum(cost) from mycost group by grouping sets((id_, name),());"; String explainString2 = getSQLPlanOrErrorMsg("explain " + sql2); System.out.println(explainString2); - Assertions.assertTrue(explainString2.contains("exprs: (`id` + 1), `name`, `cost`")); + Assertions.assertTrue(explainString2.contains("exprs: ((`id` + 1)), `name`, `cost`")); Assertions.assertTrue( - explainString2.contains(" output slots: `(`id` + 1)`, ``name``, ``cost``, ``GROUPING_ID``")); + explainString2.contains(" output slots: `((`id` + 1))`, ``name``, ``cost``, ``GROUPING_ID``")); String sql3 = "select /*+ SET_VAR(enable_nereids_planner=false) */ 1 as id_, name, sum(cost) from mycost group by grouping sets((id_, name),());"; String explainString3 = getSQLPlanOrErrorMsg("explain " + sql3); diff --git a/regression-test/data/datatype_p0/agg_state/max/test_agg_state_max.out b/regression-test/data/datatype_p0/agg_state/max/test_agg_state_max.out index 3f5d60ed72..ea6bed327d 100644 --- a/regression-test/data/datatype_p0/agg_state/max/test_agg_state_max.out +++ b/regression-test/data/datatype_p0/agg_state/max/test_agg_state_max.out @@ -9,6 +9,22 @@ 6 6999 7 7999 +-- !select -- +7999 + +-- !select -- +0 999 +1 1999 +2 2999 +3 3999 +4 4999 +5 5999 +6 6999 +7 7999 + +-- !select -- +7999 + -- !select -- 0 999 1 1999 @@ -20,3 +36,20 @@ 7 7999 100 \N +-- !select -- +7999 + +-- !select -- +0 999 +1 1999 +2 2999 +3 3999 +4 4999 +5 5999 +6 6999 +7 7999 +100 \N + +-- !select -- +7999 + diff --git a/regression-test/data/mv_p0/k1ap2spa/k1ap2spa.out b/regression-test/data/mv_p0/k1ap2spa/k1ap2spa.out index 16fe69c3ff..c7d574c013 100644 --- a/regression-test/data/mv_p0/k1ap2spa/k1ap2spa.out +++ b/regression-test/data/mv_p0/k1ap2spa/k1ap2spa.out @@ -1,11 +1,13 @@ -- This file is automatically generated. You should know what you did if you want to edit this -- !select_star -- +\N 4 \N d -4 -4 -4 d 1 1 1 a 2 2 2 b 3 -3 \N c -- !select_mv -- +\N 5 2 2 3 3 4 2 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 1ec3e354de..3397ccf3bd 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 @@ -16,9 +16,6 @@ // under the License. suite("test_agg_state_max") { - // todo: will core dump now, need fix. - sql"set enable_nereids_planner=false;" - sql "set enable_agg_state=true" sql """ DROP TABLE IF EXISTS a_table; """ sql """ @@ -40,8 +37,24 @@ suite("test_agg_state_max") { select e1/1000,max_state(e1) from (select 1 k1) as t lateral view explode_numbers(8000) tmp1 as e1;""" + sql"set enable_nereids_planner=false;" + qt_select """ select k1,max_merge(k2) from a_table group by k1 order by k1; + """ + qt_select """ select max_merge(tmp) from (select k1,max_union(k2) tmp from a_table group by k1)t; + """ + test { + sql "select k1,min_merge(k2) from a_table group by k1 order by k1;" + exception "not match function" + } + sql"set enable_nereids_planner=true;" qt_select """ select k1,max_merge(k2) from a_table group by k1 order by k1; """ + qt_select """ select max_merge(tmp) from (select k1,max_union(k2) tmp from a_table group by k1)t; + """ + test { + sql "select k1,min_merge(k2) from a_table group by k1 order by k1;" + exception "not match function" + } sql """ DROP TABLE IF EXISTS a_table2; """ sql """ @@ -58,6 +71,14 @@ suite("test_agg_state_max") { select e1/1000,max_state(e1) from (select 1 k1) as t lateral view explode_numbers(8000) tmp1 as e1;""" + sql"set enable_nereids_planner=false;" + qt_select """ select k1,max_merge(k2) from a_table2 group by k1 order by k1; + """ + qt_select """ select max_merge(tmp) from (select k1,max_union(k2) tmp from a_table group by k1)t; + """ + sql"set enable_nereids_planner=true;" qt_select """ select k1,max_merge(k2) from a_table2 group by k1 order by k1; """ + qt_select """ select max_merge(tmp) from (select k1,max_union(k2) tmp from a_table group by k1)t; + """ } diff --git a/regression-test/suites/mv_p0/k1ap2spa/k1ap2spa.groovy b/regression-test/suites/mv_p0/k1ap2spa/k1ap2spa.groovy index 24d250ce97..3bfbca6d2b 100644 --- a/regression-test/suites/mv_p0/k1ap2spa/k1ap2spa.groovy +++ b/regression-test/suites/mv_p0/k1ap2spa/k1ap2spa.groovy @@ -40,6 +40,7 @@ suite ("k1ap2spa") { createMV("create materialized view k1ap2spa as select abs(k1)+1,sum(abs(k2+1)) from d_table group by abs(k1)+1;") sql "insert into d_table select -4,-4,-4,'d';" + sql "insert into d_table(k4,k2) values('d',4);" qt_select_star "select * from d_table order by k1;" --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org