924060929 commented on code in PR #12583: URL: https://github.com/apache/doris/pull/12583#discussion_r1009378540
########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/GroupByResolve.java: ########## @@ -0,0 +1,121 @@ +// 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.rules.rewrite.logical; + +import org.apache.doris.nereids.rules.Rule; +import org.apache.doris.nereids.rules.RuleType; +import org.apache.doris.nereids.rules.analysis.OneAnalysisRuleFactory; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.NamedExpression; +import org.apache.doris.nereids.trees.expressions.VirtualSlotReference; +import org.apache.doris.nereids.trees.expressions.functions.grouping.GroupingSetsFunction; +import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionRewriter; +import org.apache.doris.nereids.trees.plans.GroupPlan; +import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate; +import org.apache.doris.nereids.trees.plans.logical.LogicalGroupBy; +import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.planner.PlannerContext; + +import com.google.common.collect.Lists; + +import java.util.ArrayList; +import java.util.BitSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Parse the grouping sets operator, + * for the groupingFunc function, create a virtualSlotRefrance for a period of time. + * + * At the same time, synchronize the groupBy in groupingSets to aggregate. + */ +public class GroupByResolve extends OneAnalysisRuleFactory { + @Override + public Rule build() { + return logicalAggregate(logicalGroupBy().whenNot(LogicalGroupBy::isResolved)).thenApply(ctx -> { + LogicalAggregate<LogicalGroupBy<GroupPlan>> aggregate = ctx.root; + LogicalGroupBy<GroupPlan> groupBy = aggregate.child(); + + List<Expression> originGroupingExprs = groupBy.getOriginalGroupByExpressions(); + List<List<Expression>> groupingSets = groupBy.getGroupingSets(); + Set<VirtualSlotReference> newVirtualSlotRefs = new LinkedHashSet<>(); + newVirtualSlotRefs.addAll(groupBy.getVirtualSlotRefs()); + + // resolve grouping func in groupBy + List<NamedExpression> groupByNewOutput = + groupBy.getOutputExpressions().stream() + .map(e -> resolve(e, newVirtualSlotRefs)) + .map(NamedExpression.class::cast) + .collect(Collectors.toList()); + + List<BitSet> groupingIdList = groupBy.genGroupingIdList(originGroupingExprs, groupingSets); + List<Expression> virtualGroupingExprs = Lists.newArrayList(); + virtualGroupingExprs.addAll(newVirtualSlotRefs); + List<List<Long>> groupingList = groupBy.genGroupingList(originGroupingExprs, + virtualGroupingExprs, groupingIdList, newVirtualSlotRefs); + List<Expression> aggGroupByExpressions = new ArrayList<>(originGroupingExprs); + aggGroupByExpressions.addAll(virtualGroupingExprs); + + return new LogicalAggregate<>(aggGroupByExpressions, groupByNewOutput, + groupBy.replace(groupingSets, originGroupingExprs, + groupByNewOutput, + groupingIdList, newVirtualSlotRefs, virtualGroupingExprs, + groupingList, true, groupBy.hasChangedOutput(), groupBy.isNormalized())); + }).toRule(RuleType.GROUP_BY_RESOLVE); + } + + private Expression resolve(Expression expr, Set<VirtualSlotReference> virtualSlotRefs) { + return new ResolveGroupingFun(virtualSlotRefs).resolve(expr); + } + + private class ResolveGroupingFun extends DefaultExpressionRewriter<PlannerContext> { Review Comment: `ResolveGroupingFun` look like create new VirtualSlotReference for the parameter which is not VirtualSlotReference. So why not choose a meaningful name? e.g. `WrapParamOfGroupingFunctionToVirtualSlot`. And then you should comment it why you do this wrap. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/grouping/GroupingSetsFunction.java: ########## @@ -0,0 +1,63 @@ +// 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.grouping; + +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.VirtualSlotReference; +import org.apache.doris.nereids.trees.expressions.functions.BoundFunction; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.nereids.types.DataType; + +import java.util.List; +import java.util.Optional; + +/** + * Functions used by all groupingSets. + */ +public abstract class GroupingSetsFunction extends BoundFunction { + protected final Optional<List<Expression>> realChildren; + + public GroupingSetsFunction(String name, Expression... arguments) { + super(name, arguments); + this.realChildren = Optional.empty(); + } + + public GroupingSetsFunction(String name, Optional<List<Expression>> realChildren, Expression... arguments) { + super(name, arguments); + this.realChildren = realChildren; Review Comment: you should ensure realChildren can not be null ########## fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java: ########## @@ -906,12 +909,28 @@ private LogicalPlan withFilter(LogicalPlan input, Optional<WhereClauseContext> w private LogicalPlan withAggregate(LogicalPlan input, SelectClauseContext selectCtx, Optional<AggClauseContext> aggCtx) { return input.optionalMap(aggCtx, () -> { - List<Expression> groupByExpressions = visit(aggCtx.get().groupByItem().expression(), Expression.class); + List<Expression> groupByExpressions = visit( + aggCtx.get().groupByItem().groupingElement().expression(), Expression.class); List<NamedExpression> namedExpressions = getNamedExpressions(selectCtx.namedExpressionSeq()); return new LogicalAggregate<>(groupByExpressions, namedExpressions, input); }); } + private LogicalPlan withGroupBy(LogicalPlan input, SelectClauseContext selectCtx, + Optional<AggClauseContext> aggCtx) { + return input.optionalMap(aggCtx, () -> { + GroupingElementContext groupingElementCtx = aggCtx.get().groupByItem().groupingElement(); + List<NamedExpression> namedExpressions = getNamedExpressions(selectCtx.namedExpressionSeq()); + if (groupingElementCtx.GROUPING() != null) { + List<List<Expression>> sets = groupingElementCtx.groupingSet().stream() + .map(groupingSet -> visit(groupingSet.expression(), Expression.class)) + .collect(Collectors.toList()); + return new LogicalGroupingSets<>(sets, namedExpressions, input); + } + return null; Review Comment: you need throw a ParseException ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java: ########## @@ -0,0 +1,97 @@ +// 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; + +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; + +import com.google.common.base.Preconditions; + +import java.util.List; +import java.util.Objects; + +/** + * it is not a real column exist in table. + */ +public class VirtualSlotReference extends SlotReference { + private final List<Expression> realSlots; + private final boolean hasCast; + + public VirtualSlotReference(String name, DataType dataType, List<Expression> realSlots, boolean hasCast) { + super(name, dataType, false); + this.realSlots = Objects.requireNonNull(realSlots, "realSlots can not null"); + this.hasCast = hasCast; + } + + public VirtualSlotReference(ExprId exprId, String name, DataType dataType, + boolean nullable, List<String> qualifier, + List<Expression> realSlots, boolean hasCast) { + super(exprId, name, dataType, nullable, qualifier); + this.realSlots = Objects.requireNonNull(realSlots, "realSlots can not null"); + this.hasCast = hasCast; + } + + public List<Expression> getRealSlots() { + return realSlots; + } + + public boolean isHasCast() { Review Comment: Do not name isHas ```suggestion public boolean hasCast() { ``` ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rewrite/ExpressionRewrite.java: ########## @@ -130,6 +133,39 @@ public Rule build() { } } + private class GroupByExpressionRewrite extends OneRewriteRuleFactory { + @Override + public Rule build() { + return logicalGroupBy().then(groupBy -> { + List<List<Expression>> groupingSetList = groupBy.getGroupingSets(); + List<List<Expression>> newGroupingSetList = groupingSetList.stream().map(rewriter::rewrite) + .collect(Collectors.toList()); + + List<Expression> groupByExpressions = groupBy.getOriginalGroupByExpressions(); + List<Expression> newGroupByExpressions = rewriter.rewrite(groupByExpressions); + + List<Expression> virtualGroupingExpressions = groupBy.getVirtualGroupingExprs(); + List<Expression> newVirtualGroupingExpressions = rewriter.rewrite(virtualGroupingExpressions); + + Set<VirtualSlotReference> newVirtualRefs = + newVirtualGroupingExpressions.stream() + .filter(VirtualSlotReference.class::isInstance) + .map(VirtualSlotReference.class::cast) + .collect(Collectors.toSet()); Review Comment: This code should have the same indent ```java Set<VirtualSlotReference> newVirtualRefs = newVirtualGroupingExpressions.stream() .filter(VirtualSlotReference.class::isInstance) .map(VirtualSlotReference.class::cast) .collect(Collectors.toSet()); ``` ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/GroupByResolve.java: ########## @@ -0,0 +1,121 @@ +// 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.rules.rewrite.logical; + +import org.apache.doris.nereids.rules.Rule; +import org.apache.doris.nereids.rules.RuleType; +import org.apache.doris.nereids.rules.analysis.OneAnalysisRuleFactory; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.NamedExpression; +import org.apache.doris.nereids.trees.expressions.VirtualSlotReference; +import org.apache.doris.nereids.trees.expressions.functions.grouping.GroupingSetsFunction; +import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionRewriter; +import org.apache.doris.nereids.trees.plans.GroupPlan; +import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate; +import org.apache.doris.nereids.trees.plans.logical.LogicalGroupBy; +import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.planner.PlannerContext; + +import com.google.common.collect.Lists; + +import java.util.ArrayList; +import java.util.BitSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Parse the grouping sets operator, + * for the groupingFunc function, create a virtualSlotRefrance for a period of time. + * + * At the same time, synchronize the groupBy in groupingSets to aggregate. + */ +public class GroupByResolve extends OneAnalysisRuleFactory { + @Override + public Rule build() { + return logicalAggregate(logicalGroupBy().whenNot(LogicalGroupBy::isResolved)).thenApply(ctx -> { Review Comment: The isResolved state is a common state for all plan, we already support the Plan::canBind to detect whether the plan is resolved. So why not use Plan::canBind as the condition? You should remove the isResolved field. ```java return logicalAggregate(logicalGroupBy().when(Plan::canBind)).thenApply(ctx -> { ... }); ``` But the Plan::canBind should refine to Plan::canBindFunction and Plan::canBindSlot in the future. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/VirtualSlotReference.java: ########## @@ -0,0 +1,97 @@ +// 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; + +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.DataType; + +import com.google.common.base.Preconditions; + +import java.util.List; +import java.util.Objects; + +/** + * it is not a real column exist in table. + */ +public class VirtualSlotReference extends SlotReference { + private final List<Expression> realSlots; + private final boolean hasCast; + + public VirtualSlotReference(String name, DataType dataType, List<Expression> realSlots, boolean hasCast) { + super(name, dataType, false); + this.realSlots = Objects.requireNonNull(realSlots, "realSlots can not null"); Review Comment: You should copyOf the realSlots, the realSlots list maybe mutable inside. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalGroupBy.java: ########## @@ -0,0 +1,287 @@ +// 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.plans.logical; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.memo.GroupExpression; +import org.apache.doris.nereids.properties.LogicalProperties; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.NamedExpression; +import org.apache.doris.nereids.trees.expressions.VirtualSlotReference; +import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.PlanType; +import org.apache.doris.nereids.trees.plans.algebra.GroupBy; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; +import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.nereids.util.Utils; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; + +import java.util.ArrayList; +import java.util.BitSet; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +/** + * Logical Grouping sets. + */ +public abstract class LogicalGroupBy<CHILD_TYPE extends Plan> extends LogicalUnary<CHILD_TYPE> implements GroupBy { + + public static final String COL_GROUPING_ID = "GROUPING_ID"; + public static final String GROUPING_PREFIX = "GROUPING_PREFIX_"; + protected final List<Expression> groupByExpressions; + protected final List<NamedExpression> outputExpressions; + protected final Set<VirtualSlotReference> virtualSlotRefs; + protected final List<BitSet> groupingIdList; + protected final List<Expression> virtualGroupingExprs; + protected final List<List<Long>> groupingList; + protected final boolean isResolved; + protected final boolean changedOutput; + protected final boolean isNormalized; + + /** + * initial construction method. + */ + public LogicalGroupBy( + PlanType type, + List<Expression> groupByExpressions, + List<NamedExpression> outputExpressions, + Optional<GroupExpression> groupExpression, + Optional<LogicalProperties> logicalProperties, + CHILD_TYPE child) { + super(type, groupExpression, logicalProperties, child); + this.groupByExpressions = ImmutableList.copyOf(groupByExpressions); + this.outputExpressions = ImmutableList.copyOf(outputExpressions); + this.groupingIdList = Lists.newArrayList(); + this.virtualSlotRefs = new LinkedHashSet<>(); + VirtualSlotReference virtualSlotReference = new VirtualSlotReference( + COL_GROUPING_ID, BigIntType.INSTANCE, new ArrayList<>(), false); + this.virtualSlotRefs.add(virtualSlotReference); + this.virtualGroupingExprs = Lists.newArrayList(); + this.groupingList = Lists.newArrayList(); Review Comment: List in TreeNode should be immutable. Instead of ImmutableList, ImmutableMap. -- 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]
