You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by hu...@apache.org on 2024/03/20 02:38:14 UTC
(doris) branch branch-2.0 updated: [fix](Nereids) fix bind having aggregate failed (#32497)
This is an automated email from the ASF dual-hosted git repository.
huajianlan 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 b869fa730bf [fix](Nereids) fix bind having aggregate failed (#32497)
b869fa730bf is described below
commit b869fa730bf927901821b270492780fd63319f0d
Author: 924060929 <92...@qq.com>
AuthorDate: Wed Mar 20 10:38:07 2024 +0800
[fix](Nereids) fix bind having aggregate failed (#32497)
cherry from #32490
---
.../org/apache/doris/catalog/FunctionRegistry.java | 41 ++++++++---
.../nereids/rules/analysis/BindExpression.java | 47 +++++++++++-
.../functions/AggStateFunctionBuilder.java | 5 ++
.../functions/BuiltinFunctionBuilder.java | 14 +++-
.../expressions/functions/FunctionBuilder.java | 2 +
.../expressions/functions/udf/AliasUdfBuilder.java | 5 ++
.../expressions/functions/udf/JavaUdafBuilder.java | 5 ++
.../expressions/functions/udf/JavaUdfBuilder.java | 5 ++
.../data/nereids_syntax_p0/bind_priority.out | 25 +++++++
.../suites/nereids_syntax_p0/bind_priority.groovy | 85 ++++++++++++++++++++--
10 files changed, 214 insertions(+), 20 deletions(-)
diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/FunctionRegistry.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/FunctionRegistry.java
index 3a08fce5755..c24b1587065 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/catalog/FunctionRegistry.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/FunctionRegistry.java
@@ -54,14 +54,14 @@ public class FunctionRegistry {
// to record the global alias function and other udf.
private static final String GLOBAL_FUNCTION = "__GLOBAL_FUNCTION__";
- private final Map<String, List<FunctionBuilder>> name2InternalBuiltinBuilders;
+ private final Map<String, List<FunctionBuilder>> name2BuiltinBuilders;
private final Map<String, Map<String, List<FunctionBuilder>>> name2UdfBuilders;
public FunctionRegistry() {
- name2InternalBuiltinBuilders = new ConcurrentHashMap<>();
+ name2BuiltinBuilders = new ConcurrentHashMap<>();
name2UdfBuilders = new ConcurrentHashMap<>();
- registerBuiltinFunctions(name2InternalBuiltinBuilders);
- afterRegisterBuiltinFunctions(name2InternalBuiltinBuilders);
+ registerBuiltinFunctions(name2BuiltinBuilders);
+ afterRegisterBuiltinFunctions(name2BuiltinBuilders);
}
// this function is used to test.
@@ -79,12 +79,33 @@ public class FunctionRegistry {
}
public Optional<List<FunctionBuilder>> tryGetBuiltinBuilders(String name) {
- List<FunctionBuilder> builders = name2InternalBuiltinBuilders.get(name);
- return name2InternalBuiltinBuilders.get(name) == null
+ List<FunctionBuilder> builders = name2BuiltinBuilders.get(name);
+ return name2BuiltinBuilders.get(name) == null
? Optional.empty()
: Optional.of(ImmutableList.copyOf(builders));
}
+ public boolean isAggregateFunction(String dbName, String name) {
+ name = name.toLowerCase();
+ Class<?> aggClass = org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction.class;
+ if (StringUtils.isEmpty(dbName)) {
+ List<FunctionBuilder> functionBuilders = name2BuiltinBuilders.get(name);
+ for (FunctionBuilder functionBuilder : functionBuilders) {
+ if (aggClass.isAssignableFrom(functionBuilder.functionClass())) {
+ return true;
+ }
+ }
+ }
+
+ List<FunctionBuilder> udfBuilders = findUdfBuilder(dbName, name);
+ for (FunctionBuilder udfBuilder : udfBuilders) {
+ if (aggClass.isAssignableFrom(udfBuilder.functionClass())) {
+ return true;
+ }
+ }
+ return false;
+ }
+
// currently we only find function by name and arity and args' types.
public FunctionBuilder findFunctionBuilder(String dbName, String name, List<?> arguments) {
List<FunctionBuilder> functionBuilders = null;
@@ -93,11 +114,11 @@ public class FunctionRegistry {
if (StringUtils.isEmpty(dbName)) {
// search internal function only if dbName is empty
- functionBuilders = name2InternalBuiltinBuilders.get(name.toLowerCase());
+ functionBuilders = name2BuiltinBuilders.get(name.toLowerCase());
if (CollectionUtils.isEmpty(functionBuilders) && AggStateFunctionBuilder.isAggStateCombinator(name)) {
String nestedName = AggStateFunctionBuilder.getNestedName(name);
String combinatorSuffix = AggStateFunctionBuilder.getCombinatorSuffix(name);
- functionBuilders = name2InternalBuiltinBuilders.get(nestedName.toLowerCase());
+ functionBuilders = name2BuiltinBuilders.get(nestedName.toLowerCase());
if (functionBuilders != null) {
functionBuilders = functionBuilders.stream()
.map(builder -> new AggStateFunctionBuilder(combinatorSuffix, builder))
@@ -193,8 +214,8 @@ public class FunctionRegistry {
}
synchronized (name2UdfBuilders) {
Map<String, List<FunctionBuilder>> builders = name2UdfBuilders.getOrDefault(dbName, ImmutableMap.of());
- builders.getOrDefault(name, Lists.newArrayList()).removeIf(builder -> ((UdfBuilder) builder).getArgTypes()
- .equals(argTypes));
+ builders.getOrDefault(name, Lists.newArrayList())
+ .removeIf(builder -> ((UdfBuilder) builder).getArgTypes().equals(argTypes));
}
}
}
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
index 67a82edbf53..aa44674cf98 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindExpression.java
@@ -485,10 +485,32 @@ public class BindExpression implements AnalysisRuleFactory {
logicalHaving(aggregate()).when(Plan::canBind).thenApply(ctx -> {
LogicalHaving<Aggregate<Plan>> having = ctx.root;
Aggregate<Plan> childPlan = having.child();
+
+ FunctionRegistry functionRegistry
+ = ctx.cascadesContext.getConnectContext().getEnv().getFunctionRegistry();
+
+ List<Expression> groupByExprs = childPlan.getGroupByExpressions();
+ Builder<Slot> groupBySlotsBuilder = ImmutableList.builderWithExpectedSize(groupByExprs.size());
+ for (Expression groupBy : groupByExprs) {
+ if (groupBy instanceof Slot) {
+ groupBySlotsBuilder.add((Slot) groupBy);
+ }
+ }
+ List<Slot> groupBySlots = groupBySlotsBuilder.build();
+
Set<Expression> boundConjuncts = having.getConjuncts().stream()
.map(expr -> {
- expr = bindSlot(expr, childPlan.child(), ctx.cascadesContext, false);
- return bindSlot(expr, childPlan, ctx.cascadesContext, false);
+ if (hasAggregateFunction(expr, functionRegistry)) {
+ expr = bindSlot(expr, childPlan.child(), ctx.cascadesContext, false);
+ } else {
+ expr = new SlotBinder(toScope(ctx.cascadesContext, groupBySlots),
+ ctx.cascadesContext, false, false
+ ).bind(expr);
+
+ expr = bindSlot(expr, childPlan, ctx.cascadesContext, false);
+ expr = bindSlot(expr, childPlan.children(), ctx.cascadesContext, false);
+ }
+ return expr;
})
.map(expr -> bindFunction(expr, ctx.root, ctx.cascadesContext))
.map(expr -> TypeCoercionUtils.castIfNotSameType(expr, BooleanType.INSTANCE))
@@ -550,7 +572,7 @@ public class BindExpression implements AnalysisRuleFactory {
// we need to do cast before set operation, because we maybe use these slot to do shuffle
// so, we must cast it before shuffle to get correct hash code.
List<List<NamedExpression>> childrenProjections = setOperation.collectChildrenProjections();
- ImmutableList.Builder<List<SlotReference>> childrenOutputs = ImmutableList.builder();
+ Builder<List<SlotReference>> childrenOutputs = ImmutableList.builder();
Builder<Plan> newChildren = ImmutableList.builder();
for (int i = 0; i < childrenProjections.size(); i++) {
Plan newChild;
@@ -824,4 +846,23 @@ public class BindExpression implements AnalysisRuleFactory {
});
return expression;
}
+
+ private boolean hasAggregateFunction(Expression expression, FunctionRegistry functionRegistry) {
+ return expression.anyMatch(expr -> {
+ if (expr instanceof AggregateFunction) {
+ return true;
+ } else if (expr instanceof UnboundFunction) {
+ UnboundFunction unboundFunction = (UnboundFunction) expr;
+ boolean isAggregateFunction = functionRegistry
+ .isAggregateFunction(
+ unboundFunction.getDbName(),
+ unboundFunction.getName()
+ );
+ if (isAggregateFunction) {
+ return true;
+ }
+ }
+ return false;
+ });
+ }
}
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 054aa4767b6..7b84e646785 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
@@ -50,6 +50,11 @@ public class AggStateFunctionBuilder extends FunctionBuilder {
this.nestedBuilder = Objects.requireNonNull(nestedBuilder, "nestedBuilder can not be null");
}
+ @Override
+ public Class<? extends BoundFunction> functionClass() {
+ return nestedBuilder.functionClass();
+ }
+
@Override
public boolean canApply(List<? extends Object> arguments) {
if (combinatorSuffix.equals(STATE)) {
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/BuiltinFunctionBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/BuiltinFunctionBuilder.java
index 74c4a918cf0..45cabb8d55e 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/BuiltinFunctionBuilder.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/BuiltinFunctionBuilder.java
@@ -42,13 +42,21 @@ public class BuiltinFunctionBuilder extends FunctionBuilder {
// Concrete BoundFunction's constructor
private final Constructor<BoundFunction> builderMethod;
+ private final Class<? extends BoundFunction> functionClass;
- public BuiltinFunctionBuilder(Constructor<BoundFunction> builderMethod) {
+ public BuiltinFunctionBuilder(
+ Class<? extends BoundFunction> functionClass, Constructor<BoundFunction> builderMethod) {
+ this.functionClass = Objects.requireNonNull(functionClass, "functionClass can not be null");
this.builderMethod = Objects.requireNonNull(builderMethod, "builderMethod can not be null");
this.arity = builderMethod.getParameterCount();
this.isVariableLength = arity > 0 && builderMethod.getParameterTypes()[arity - 1].isArray();
}
+ @Override
+ public Class<? extends BoundFunction> functionClass() {
+ return functionClass;
+ }
+
@Override
public boolean canApply(List<? extends Object> arguments) {
if (isVariableLength && arity > arguments.size() + 1) {
@@ -133,7 +141,9 @@ public class BuiltinFunctionBuilder extends FunctionBuilder {
+ functionClass.getSimpleName());
return Arrays.stream(functionClass.getConstructors())
.filter(constructor -> Modifier.isPublic(constructor.getModifiers()))
- .map(constructor -> new BuiltinFunctionBuilder((Constructor<BoundFunction>) constructor))
+ .map(constructor -> new BuiltinFunctionBuilder(
+ functionClass, (Constructor<BoundFunction>) constructor)
+ )
.collect(ImmutableList.toImmutableList());
}
}
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/FunctionBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/FunctionBuilder.java
index 0b3a50a239b..d1e69d3e307 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/FunctionBuilder.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/FunctionBuilder.java
@@ -27,6 +27,8 @@ import java.util.List;
* This class used to build BoundFunction(Builtin or Combinator) by a list of Expressions.
*/
public abstract class FunctionBuilder {
+ public abstract Class<? extends BoundFunction> functionClass();
+
/** check whether arguments can apply to the constructor */
public abstract boolean canApply(List<? extends Object> arguments);
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/AliasUdfBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/AliasUdfBuilder.java
index 197cb8b396d..733bd5fcae1 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/AliasUdfBuilder.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/AliasUdfBuilder.java
@@ -50,6 +50,11 @@ public class AliasUdfBuilder extends UdfBuilder {
return aliasUdf.getArgTypes();
}
+ @Override
+ public Class<? extends BoundFunction> functionClass() {
+ return AliasUdf.class;
+ }
+
@Override
public boolean canApply(List<?> arguments) {
if (arguments.size() != aliasUdf.arity()) {
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdafBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdafBuilder.java
index d0c0b067e94..89073b4eb77 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdafBuilder.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdafBuilder.java
@@ -49,6 +49,11 @@ public class JavaUdafBuilder extends UdfBuilder {
.collect(Collectors.toList())).get();
}
+ @Override
+ public Class<? extends BoundFunction> functionClass() {
+ return JavaUdaf.class;
+ }
+
@Override
public boolean canApply(List<?> arguments) {
if ((isVarArgs && arity > arguments.size() + 1) || (!isVarArgs && arguments.size() != arity)) {
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdfBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdfBuilder.java
index c11b07665a6..5df0690c660 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdfBuilder.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/udf/JavaUdfBuilder.java
@@ -52,6 +52,11 @@ public class JavaUdfBuilder extends UdfBuilder {
.collect(Collectors.toList())).get();
}
+ @Override
+ public Class<? extends BoundFunction> functionClass() {
+ return JavaUdf.class;
+ }
+
@Override
public boolean canApply(List<?> arguments) {
if ((isVarArgs && arity > arguments.size() + 1) || (!isVarArgs && arguments.size() != arity)) {
diff --git a/regression-test/data/nereids_syntax_p0/bind_priority.out b/regression-test/data/nereids_syntax_p0/bind_priority.out
index 7cd1e3ea569..b3bc666c23d 100644
--- a/regression-test/data/nereids_syntax_p0/bind_priority.out
+++ b/regression-test/data/nereids_syntax_p0/bind_priority.out
@@ -36,3 +36,28 @@ all 2
4 5
6 6
+-- !having_bind_child --
+1 10
+
+-- !having_bind_child2 --
+2 10
+
+-- !having_bind_child3 --
+2 10
+
+-- !having_bind_project --
+2 10
+
+-- !having_bind_project2 --
+
+-- !having_bind_project3 --
+
+-- !having_bind_project4 --
+2 11
+
+-- !having_bind_child4 --
+2 11
+
+-- !having_bind_child5 --
+2 11
+
diff --git a/regression-test/suites/nereids_syntax_p0/bind_priority.groovy b/regression-test/suites/nereids_syntax_p0/bind_priority.groovy
index 19b630854cb..0e02f01a18b 100644
--- a/regression-test/suites/nereids_syntax_p0/bind_priority.groovy
+++ b/regression-test/suites/nereids_syntax_p0/bind_priority.groovy
@@ -31,7 +31,7 @@ suite("bind_priority") {
sql """
insert into bind_priority_tbl values(1, 2),(3, 4)
"""
-
+
sql "SET enable_nereids_planner=true"
sql "SET enable_fallback_to_original_planner=false"
@@ -100,17 +100,17 @@ suite("bind_priority") {
);
"""
sql "insert into bind_priority_tbl2 values(3,5),(2, 6),(1,4);"
-
+
qt_bind_order_to_project_alias """
select bind_priority_tbl.b b, bind_priority_tbl2.b
- from bind_priority_tbl join bind_priority_tbl2 on bind_priority_tbl.a=bind_priority_tbl2.a
+ from bind_priority_tbl join bind_priority_tbl2 on bind_priority_tbl.a=bind_priority_tbl2.a
order by b;
"""
qt_bind_order_to_project_alias """
select bind_priority_tbl.b, bind_priority_tbl2.b b
- from bind_priority_tbl join bind_priority_tbl2 on bind_priority_tbl.a=bind_priority_tbl2.a
+ from bind_priority_tbl join bind_priority_tbl2 on bind_priority_tbl.a=bind_priority_tbl2.a
order by b;
"""
@@ -148,11 +148,86 @@ suite("bind_priority") {
) a
), tb2 as
(
- select * from tb1
+ select * from tb1
)
select * from tb2 order by id;
""")
result([[1], [2], [3]])
}
+
+ def testBindHaving = {
+ sql "drop table if exists test_bind_having_slots"
+
+ sql "create table test_bind_having_slots " +
+ "(id int, age int) " +
+ "distributed by hash(id) " +
+ "properties('replication_num'='1');"
+ sql "insert into test_bind_having_slots values(1, 10), (2, 20), (3, 30);"
+
+ order_qt_having_bind_child """
+ select id, sum(age)
+ from test_bind_having_slots s
+ group by id
+ having id = 1; -- bind id from group by
+ """
+
+ order_qt_having_bind_child2 """
+ select id + 1 as id, sum(age)
+ from test_bind_having_slots s
+ group by id
+ having id = 1; -- bind id from group by
+ """
+
+
+ order_qt_having_bind_child3 """
+ select id + 1 as id, sum(age)
+ from test_bind_having_slots s
+ group by id
+ having id + 1 = 2; -- bind id from group by
+ """
+
+ order_qt_having_bind_project """
+ select id + 1 as id, sum(age)
+ from test_bind_having_slots s
+ group by id + 1
+ having id = 2; -- bind id from project
+ """
+
+ order_qt_having_bind_project2 """
+ select id + 1 as id, sum(age)
+ from test_bind_having_slots s
+ group by id + 1
+ having id + 1 = 2; -- bind id from project
+ """
+
+
+ order_qt_having_bind_project3 """
+ select id + 1 as id, sum(age + 1) as age
+ from test_bind_having_slots s
+ group by id
+ having age = 10; -- bind id from age
+ """
+
+ order_qt_having_bind_project4 """
+ select id + 1 as id, sum(age + 1) as age
+ from test_bind_having_slots s
+ group by id
+ having age = 11; -- bind age from project
+ """
+
+ order_qt_having_bind_child4 """
+ select id + 1 as id, sum(age + 1) as age
+ from test_bind_having_slots s
+ group by id
+ having sum(age) = 10; -- bind age from s
+ """
+
+ order_qt_having_bind_child5 """
+ select id + 1 as id, sum(age + 1) as age
+ from test_bind_having_slots s
+ group by id
+ having sum(age + 1) = 11 -- bind age from s
+ """
+ }()
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org