From 2704651fde2d81b3bf9d6cc0fd46e467e3b31728 Mon Sep 17 00:00:00 2001 From: starocean999 <40539150+starocean999@users.noreply.github.com> Date: Fri, 30 Dec 2022 14:26:21 +0800 Subject: [PATCH] [fix](nereids) hll and bitmap type can't be used as order by and group by exprs (#15471) hll, bitmap, array and quantile state type can't be used in order by, group by and some agg exprs. --- .../rules/analysis/CheckAfterRewrite.java | 24 +++++++++++ .../ForbiddenMetricTypeArguments.java | 26 ++++++++++++ .../expressions/functions/agg/Count.java | 4 +- .../trees/expressions/functions/agg/Max.java | 4 +- .../trees/expressions/functions/agg/Min.java | 4 +- .../trees/expressions/functions/agg/Ndv.java | 3 +- .../apache/doris/nereids/types/DataType.java | 17 ++++++++ .../aggregate_group_by_metric_type.groovy | 40 +++++++++++++++++++ 8 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ForbiddenMetricTypeArguments.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java index d913f381ae..24287bf908 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/CheckAfterRewrite.java @@ -17,6 +17,7 @@ package org.apache.doris.nereids.rules.analysis; +import org.apache.doris.catalog.Type; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.rules.Rule; import org.apache.doris.nereids.rules.RuleType; @@ -24,7 +25,10 @@ import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.Slot; import org.apache.doris.nereids.trees.expressions.VirtualSlotReference; import org.apache.doris.nereids.trees.expressions.functions.ExpressionTrait; +import org.apache.doris.nereids.trees.expressions.functions.ForbiddenMetricTypeArguments; import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate; +import org.apache.doris.nereids.trees.plans.logical.LogicalSort; import org.apache.commons.lang.StringUtils; @@ -41,6 +45,7 @@ public class CheckAfterRewrite extends OneAnalysisRuleFactory { public Rule build() { return any().then(plan -> { checkAllSlotReferenceFromChildren(plan); + checkMetricTypeIsUsedCorrectly(plan); return null; }).toRule(RuleType.CHECK_ANALYSIS); } @@ -79,4 +84,23 @@ public class CheckAfterRewrite extends OneAnalysisRuleFactory { }) .collect(Collectors.toSet()); } + + private void checkMetricTypeIsUsedCorrectly(Plan plan) { + if (plan instanceof LogicalAggregate) { + if (((LogicalAggregate) plan).getGroupByExpressions().stream() + .anyMatch(expression -> expression.getDataType().isOnlyMetricType()) + || ((LogicalAggregate) plan).getAggregateFunctions().stream() + .filter(aggregateFunction -> aggregateFunction instanceof ForbiddenMetricTypeArguments).anyMatch( + aggregateFunction -> aggregateFunction.getArgumentsTypes().stream() + .anyMatch(dataType -> dataType.isOnlyMetricType()))) { + throw new AnalysisException(Type.OnlyMetricTypeErrorMsg); + } + } else if (plan instanceof LogicalSort) { + if (((LogicalSort) plan).getOrderKeys().stream().anyMatch(( + orderKey -> orderKey.getExpr().getDataType() + .isOnlyMetricType()))) { + throw new AnalysisException(Type.OnlyMetricTypeErrorMsg); + } + } + } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ForbiddenMetricTypeArguments.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ForbiddenMetricTypeArguments.java new file mode 100644 index 0000000000..a5a5e16eee --- /dev/null +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/ForbiddenMetricTypeArguments.java @@ -0,0 +1,26 @@ +// 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.nereids.trees.expressions.functions; + +/** + * means the function doesn't accept MetricType Arguments + * + * e.g. Count, Min, Max, Ndv etc + */ +public interface ForbiddenMetricTypeArguments { +} diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java index cc05cec814..1dfe0cca17 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Count.java @@ -22,6 +22,7 @@ import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.AlwaysNotNullable; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.functions.ForbiddenMetricTypeArguments; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.BigIntType; import org.apache.doris.nereids.types.DataType; @@ -32,7 +33,8 @@ import com.google.common.collect.ImmutableList; import java.util.List; /** count agg function. */ -public class Count extends AggregateFunction implements AlwaysNotNullable, CustomSignature { +public class Count extends AggregateFunction implements AlwaysNotNullable, CustomSignature, + ForbiddenMetricTypeArguments { private final boolean isStar; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Max.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Max.java index 25764d8b72..a374313453 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Max.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Max.java @@ -20,6 +20,7 @@ package org.apache.doris.nereids.trees.expressions.functions.agg; import org.apache.doris.catalog.FunctionSignature; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.functions.ForbiddenMetricTypeArguments; import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable; import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; @@ -31,7 +32,8 @@ import com.google.common.collect.ImmutableList; import java.util.List; /** max agg function. */ -public class Max extends AggregateFunction implements UnaryExpression, PropagateNullable, CustomSignature { +public class Max extends AggregateFunction implements UnaryExpression, PropagateNullable, CustomSignature, + ForbiddenMetricTypeArguments { public Max(Expression child) { super("max", child); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Min.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Min.java index ea5e05584c..aadb48471c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Min.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Min.java @@ -20,6 +20,7 @@ package org.apache.doris.nereids.trees.expressions.functions.agg; import org.apache.doris.catalog.FunctionSignature; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.CustomSignature; +import org.apache.doris.nereids.trees.expressions.functions.ForbiddenMetricTypeArguments; import org.apache.doris.nereids.trees.expressions.functions.PropagateNullable; import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; @@ -31,7 +32,8 @@ import com.google.common.collect.ImmutableList; import java.util.List; /** min agg function. */ -public class Min extends AggregateFunction implements UnaryExpression, PropagateNullable, CustomSignature { +public class Min extends AggregateFunction implements UnaryExpression, PropagateNullable, CustomSignature, + ForbiddenMetricTypeArguments { public Min(Expression child) { super("min", child); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Ndv.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Ndv.java index f38c75d22f..0133ec572a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Ndv.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/Ndv.java @@ -21,6 +21,7 @@ import org.apache.doris.catalog.FunctionSignature; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.AlwaysNotNullable; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.functions.ForbiddenMetricTypeArguments; import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; import org.apache.doris.nereids.types.BigIntType; @@ -57,7 +58,7 @@ import java.util.List; * AggregateFunction 'ndv'. This class is generated by GenerateFunction. */ public class Ndv extends AggregateFunction - implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNotNullable { + implements UnaryExpression, ExplicitlyCastableSignature, AlwaysNotNullable, ForbiddenMetricTypeArguments { public static final List SIGNATURES = ImmutableList.of( FunctionSignature.ret(BigIntType.INSTANCE).args(TinyIntType.INSTANCE), diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java index 8c4be55c86..73e00c8986 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java @@ -492,6 +492,23 @@ public abstract class DataType implements AbstractDataType { return this instanceof HllType; } + public boolean isQuantileState() { + return this instanceof QuantileStateType; + } + + public boolean isArray() { + return this instanceof ArrayType; + } + + // only metric types have the following constraint: + // 1. don't support as key column + // 2. don't support filter + // 3. don't support group by + // 4. don't support index + public boolean isOnlyMetricType() { + return isHll() || isBitmap() || isQuantileState() || isArray(); + } + public DataType promotion() { if (PROMOTION_MAP.containsKey(this.getClass())) { return PROMOTION_MAP.get(this.getClass()).get(); diff --git a/regression-test/suites/query_p0/aggregate/aggregate_group_by_metric_type.groovy b/regression-test/suites/query_p0/aggregate/aggregate_group_by_metric_type.groovy index c44bc1b7ea..78b687b7cf 100644 --- a/regression-test/suites/query_p0/aggregate/aggregate_group_by_metric_type.groovy +++ b/regression-test/suites/query_p0/aggregate/aggregate_group_by_metric_type.groovy @@ -46,6 +46,31 @@ suite("aggregate_group_by_metric_type") { exception "${error_msg}" } + sql 'set enable_vectorized_engine=true;' + sql 'set enable_fallback_to_original_planner=false' + sql 'set enable_nereids_planner=true' + + test { + sql "select distinct user_ids from test_group_by_hll_and_bitmap" + exception "${error_msg}" + } + + test { + sql "select distinct hll_set from test_group_by_hll_and_bitmap" + exception "${error_msg}" + } + + test { + sql "select user_ids from test_group_by_hll_and_bitmap order by user_ids" + exception "${error_msg}" + } + + test { + sql "select hll_set from test_group_by_hll_and_bitmap order by hll_set" + exception "${error_msg}" + } + sql 'set enable_nereids_planner=false' + sql "DROP TABLE test_group_by_hll_and_bitmap" sql "DROP TABLE IF EXISTS test_group_by_array" @@ -68,5 +93,20 @@ suite("aggregate_group_by_metric_type") { exception "${error_msg}" } + sql 'set enable_nereids_planner=true' + test { + sql "select distinct c_array from test_group_by_array" + exception "${error_msg}" + } + test { + sql "select c_array from test_group_by_array order by c_array" + exception "${error_msg}" + } + test { + sql "select c_array,count(*) from test_group_by_array group by c_array" + exception "${error_msg}" + } + sql 'set enable_nereids_planner=false' + sql "DROP TABLE test_group_by_array" }