morrySnow commented on code in PR #28596:
URL: https://github.com/apache/doris/pull/28596#discussion_r1430850022
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVCache.java:
##########
@@ -64,6 +64,7 @@ public MTMVCache(Plan logicalPlan, List<NamedExpression>
mvOutputExpressions) {
public static MTMVCache from(MTMV mtmv, ConnectContext connectContext) {
LogicalPlan unboundMvPlan = new
NereidsParser().parseSingle(mtmv.getQuerySql());
// TODO: connect context set current db when create mv by use database
+ // view should also disable the predicate infer and join eliminate.
Review Comment:
do we really need this comment?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/PlaceholderExpression.java:
##########
@@ -33,7 +33,7 @@
* @see PlaceholderCollector
*/
public class PlaceholderExpression extends Expression implements
AlwaysNotNullable {
-
+ protected boolean distinct;
Review Comment:
add some comment to explain this attr since it is not easy to understand
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -49,7 +50,8 @@ public class MTMVRelationManager implements MTMVHookService {
private Map<BaseTableInfo, Set<BaseTableInfo>> tableMTMVs =
Maps.newConcurrentMap();
public Set<BaseTableInfo> getMtmvsByBaseTable(BaseTableInfo table) {
- return tableMTMVs.get(table);
+ Set<BaseTableInfo> baseTableInfos = tableMTMVs.get(table);
Review Comment:
getOrDefault
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/PlaceholderExpression.java:
##########
@@ -33,7 +33,7 @@
* @see PlaceholderCollector
*/
public class PlaceholderExpression extends Expression implements
AlwaysNotNullable {
-
+ protected boolean distinct;
Review Comment:
all attr should be final
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/PlaceholderExpression.java:
##########
@@ -46,10 +46,23 @@ public PlaceholderExpression(List<Expression> children,
Class<? extends Expressi
this.position = position;
}
+ public PlaceholderExpression(List<Expression> children, Class<? extends
Expression> delegateClazz, int position,
Review Comment:
the original ctor should call this(children, delegateClazz, position, false);
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/RuleSet.java:
##########
@@ -121,6 +126,7 @@ public class RuleSet {
.add(PushDownProjectThroughSemiJoin.INSTANCE)
.add(TransposeAggSemiJoin.INSTANCE)
.add(TransposeAggSemiJoinProject.INSTANCE)
+ .add(OrExpansion.INSTANCE)
Review Comment:
this is a wrong change?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -63,10 +85,12 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
// get view and query aggregate and top plan correspondingly
Pair<Plan, LogicalAggregate<Plan>> viewTopPlanAndAggPair =
splitToTopPlanAndAggregate(viewStructInfo);
if (viewTopPlanAndAggPair == null) {
+ logger.info(currentClassName + "split to view to top plan and agg
fail so return null");
return null;
}
Pair<Plan, LogicalAggregate<Plan>> queryTopPlanAndAggPair =
splitToTopPlanAndAggregate(queryStructInfo);
if (queryTopPlanAndAggPair == null) {
+ logger.info(currentClassName + "split to query to top plan and agg
fail so return null");
Review Comment:
```suggestion
logger.info(currentClassName + " split to query to top plan and
agg fail so return null");
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -63,10 +85,12 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
// get view and query aggregate and top plan correspondingly
Pair<Plan, LogicalAggregate<Plan>> viewTopPlanAndAggPair =
splitToTopPlanAndAggregate(viewStructInfo);
if (viewTopPlanAndAggPair == null) {
+ logger.info(currentClassName + "split to view to top plan and agg
fail so return null");
Review Comment:
```suggestion
logger.info(currentClassName + " split to view to top plan and
agg fail so return null");
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -53,6 +64,17 @@
*/
public abstract class AbstractMaterializedViewAggregateRule extends
AbstractMaterializedViewRule {
+ protected static final Map<PlaceholderExpression, PlaceholderExpression>
+ AGGREGATE_ROLL_UP_EQUIVALENT_FUNCTION_MAP = new HashMap<>();
+ protected final String currentClassName = this.getClass().getSimpleName();
+ private final Logger logger = LogManager.getLogger(this.getClass());
+
+ static {
+ AGGREGATE_ROLL_UP_EQUIVALENT_FUNCTION_MAP.put(
+ PlaceholderExpression.of(Count.class, 0, true),
+ new PlaceholderExpression(ImmutableList.of(),
BitmapUnion.class, 0));
Review Comment:
do not use PlaceholderExpression as we negotiate offline
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -226,14 +255,44 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
}
// only support sum roll up, support other agg functions later.
- private AggregateFunction rollup(AggregateFunction originFunction,
- Expression mappedExpression) {
- Class<? extends AggregateFunction> rollupAggregateFunction =
originFunction.getRollup();
+ private Function rollup(AggregateFunction queryFunction,
+ Expression queryFunctionShuttled,
+ Map<Expression, Expression> mvExprToMvScanExprQueryBased) {
+ Expression rollupParam = null;
+ if (mvExprToMvScanExprQueryBased.containsKey(queryFunctionShuttled)) {
+ // function can not rewrite by view
+ rollupParam =
mvExprToMvScanExprQueryBased.get(queryFunctionShuttled);
+ } else {
+ // try to use complex roll up param
+ // eg: query is count(distinct param), mv sql is
bitmap_union(to_bitmap(param))
+ for (Expression mvExprShuttled :
mvExprToMvScanExprQueryBased.keySet()) {
+ if (!(mvExprShuttled instanceof Function)) {
+ continue;
+ }
+ if (isAggregateFunctionEquivalent(queryFunction, (Function)
mvExprShuttled)) {
+ rollupParam =
mvExprToMvScanExprQueryBased.get(mvExprShuttled);
+ }
+ }
+ }
+ if (rollupParam == null) {
+ return null;
+ }
+ // do roll up
+ Class<? extends Function> rollupAggregateFunction =
queryFunction.getRollup();
if (rollupAggregateFunction == null) {
return null;
}
if (Sum.class.isAssignableFrom(rollupAggregateFunction)) {
- return new Sum(originFunction.isDistinct(), mappedExpression);
+ return new Sum(queryFunction.isDistinct(), rollupParam);
+ }
+ if (Max.class.isAssignableFrom(rollupAggregateFunction)) {
+ return new Max(queryFunction.isDistinct(), rollupParam);
+ }
+ if (Min.class.isAssignableFrom(rollupAggregateFunction)) {
+ return new Min(queryFunction.isDistinct(), rollupParam);
+ }
+ if (BitmapUnionCount.class.isAssignableFrom(rollupAggregateFunction)) {
+ return new BitmapUnionCount(rollupParam);
Review Comment:
i think u should add a new trait named CouldRollUp and a new interface into
this trait, such as `constructRollUp`, and return rollup object directly
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -151,14 +184,101 @@ protected List<Plan> rewrite(Plan queryPlan,
CascadesContext cascadesContext) {
rewritedPlan,
materializationContext);
if (rewritedPlan == null) {
+ logger.info(currentClassName + " rewrite query by view
fail so continue");
continue;
}
+ if (!checkPartitionIsValid(queryStructInfo,
materializationContext, cascadesContext)) {
+ logger.info(currentClassName + " check partition
validation fail so continue");
+ continue;
+ }
+ // run rbo job on mv rewritten plan
+ CascadesContext rewrittenPlanContext =
+
CascadesContext.initContext(cascadesContext.getStatementContext(), rewritedPlan,
+
cascadesContext.getCurrentJobContext().getRequiredProperties());
+ Rewriter.getWholeTreeRewriter(cascadesContext).execute();
+ rewritedPlan = rewrittenPlanContext.getRewritePlan();
+ logger.info(currentClassName + "rewrite by materialized view
success");
rewriteResults.add(rewritedPlan);
}
}
return rewriteResults;
}
+ protected boolean checkPartitionIsValid(
+ StructInfo queryInfo,
+ MaterializationContext materializationContext,
+ CascadesContext cascadesContext) {
+ // check partition is valid or not
+ MTMV mtmv = materializationContext.getMtmv();
+ PartitionInfo mvPartitionInfo = mtmv.getPartitionInfo();
+ if (PartitionType.UNPARTITIONED.equals(mvPartitionInfo.getType())) {
+ // if not partition, if rewrite success, it means mv is available
+ return true;
+ }
+ // check mv related table partition is valid or not
+ MTMVPartitionInfo mvCustomPartitionInfo = mtmv.getMvPartitionInfo();
+ BaseTableInfo relatedPartitionTable =
mvCustomPartitionInfo.getRelatedTable();
+ if (relatedPartitionTable == null) {
+ return true;
+ }
+ Optional<LogicalOlapScan> relatedTableRelation =
queryInfo.getRelations().stream()
+ .filter(relation -> relatedPartitionTable.equals(new
BaseTableInfo(relation.getTable()))
+ && relation instanceof LogicalOlapScan)
+ .map(relation -> (LogicalOlapScan) relation)
+ .findFirst();
+ if (!relatedTableRelation.isPresent()) {
+ logger.warn("mv is partition update, but related table relation is
null");
Review Comment:
if u want print warn log, u should print some context, such as query id,
relation name, etc..
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -151,14 +184,101 @@ protected List<Plan> rewrite(Plan queryPlan,
CascadesContext cascadesContext) {
rewritedPlan,
materializationContext);
if (rewritedPlan == null) {
+ logger.info(currentClassName + " rewrite query by view
fail so continue");
continue;
}
+ if (!checkPartitionIsValid(queryStructInfo,
materializationContext, cascadesContext)) {
+ logger.info(currentClassName + " check partition
validation fail so continue");
+ continue;
+ }
+ // run rbo job on mv rewritten plan
+ CascadesContext rewrittenPlanContext =
+
CascadesContext.initContext(cascadesContext.getStatementContext(), rewritedPlan,
+
cascadesContext.getCurrentJobContext().getRequiredProperties());
+ Rewriter.getWholeTreeRewriter(cascadesContext).execute();
+ rewritedPlan = rewrittenPlanContext.getRewritePlan();
+ logger.info(currentClassName + "rewrite by materialized view
success");
rewriteResults.add(rewritedPlan);
}
}
return rewriteResults;
}
+ protected boolean checkPartitionIsValid(
+ StructInfo queryInfo,
+ MaterializationContext materializationContext,
+ CascadesContext cascadesContext) {
+ // check partition is valid or not
+ MTMV mtmv = materializationContext.getMtmv();
+ PartitionInfo mvPartitionInfo = mtmv.getPartitionInfo();
+ if (PartitionType.UNPARTITIONED.equals(mvPartitionInfo.getType())) {
+ // if not partition, if rewrite success, it means mv is available
+ return true;
+ }
+ // check mv related table partition is valid or not
+ MTMVPartitionInfo mvCustomPartitionInfo = mtmv.getMvPartitionInfo();
+ BaseTableInfo relatedPartitionTable =
mvCustomPartitionInfo.getRelatedTable();
+ if (relatedPartitionTable == null) {
+ return true;
+ }
+ Optional<LogicalOlapScan> relatedTableRelation =
queryInfo.getRelations().stream()
+ .filter(relation -> relatedPartitionTable.equals(new
BaseTableInfo(relation.getTable()))
+ && relation instanceof LogicalOlapScan)
+ .map(relation -> (LogicalOlapScan) relation)
+ .findFirst();
Review Comment:
```suggestion
Optional<LogicalOlapScan> relatedTableRelation =
queryInfo.getRelations().stream()
.filter(LogicalOlapScan.class::isInstance)
.filter(relation -> relatedPartitionTable.equals(new
BaseTableInfo(relation.getTable())))
.map(LogicalOlapScan.class::cast)
.findFirst();
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -151,14 +184,101 @@ protected List<Plan> rewrite(Plan queryPlan,
CascadesContext cascadesContext) {
rewritedPlan,
materializationContext);
if (rewritedPlan == null) {
+ logger.info(currentClassName + " rewrite query by view
fail so continue");
continue;
}
+ if (!checkPartitionIsValid(queryStructInfo,
materializationContext, cascadesContext)) {
+ logger.info(currentClassName + " check partition
validation fail so continue");
+ continue;
+ }
+ // run rbo job on mv rewritten plan
+ CascadesContext rewrittenPlanContext =
+
CascadesContext.initContext(cascadesContext.getStatementContext(), rewritedPlan,
+
cascadesContext.getCurrentJobContext().getRequiredProperties());
+ Rewriter.getWholeTreeRewriter(cascadesContext).execute();
+ rewritedPlan = rewrittenPlanContext.getRewritePlan();
+ logger.info(currentClassName + "rewrite by materialized view
success");
rewriteResults.add(rewritedPlan);
}
}
return rewriteResults;
}
+ protected boolean checkPartitionIsValid(
Review Comment:
add comment to explain what valid means
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -63,10 +85,12 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
// get view and query aggregate and top plan correspondingly
Pair<Plan, LogicalAggregate<Plan>> viewTopPlanAndAggPair =
splitToTopPlanAndAggregate(viewStructInfo);
if (viewTopPlanAndAggPair == null) {
+ logger.info(currentClassName + "split to view to top plan and agg
fail so return null");
Review Comment:
this is not a bug? so just use info log?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -80,40 +100,51 @@ protected List<Plan> rewrite(Plan queryPlan,
CascadesContext cascadesContext) {
if (queryPlan.getGroupExpression().isPresent()
&& materializationContext.alreadyRewrite(
queryPlan.getGroupExpression().get().getOwnerGroup().getGroupId())) {
+ logger.info(currentClassName + " this group is already
rewritten so skip");
continue;
}
- Plan mvPlan =
materializationContext.getMtmv().getCache().getLogicalPlan();
- List<StructInfo> viewStructInfos = extractStructInfo(mvPlan,
cascadesContext);
+ MTMV mtmv = materializationContext.getMtmv();
Review Comment:
some function use `Mtmv` and others use `MTMV`. we need to unify them
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -151,14 +184,101 @@ protected List<Plan> rewrite(Plan queryPlan,
CascadesContext cascadesContext) {
rewritedPlan,
materializationContext);
if (rewritedPlan == null) {
+ logger.info(currentClassName + " rewrite query by view
fail so continue");
continue;
}
+ if (!checkPartitionIsValid(queryStructInfo,
materializationContext, cascadesContext)) {
+ logger.info(currentClassName + " check partition
validation fail so continue");
+ continue;
+ }
+ // run rbo job on mv rewritten plan
+ CascadesContext rewrittenPlanContext =
+
CascadesContext.initContext(cascadesContext.getStatementContext(), rewritedPlan,
+
cascadesContext.getCurrentJobContext().getRequiredProperties());
+ Rewriter.getWholeTreeRewriter(cascadesContext).execute();
+ rewritedPlan = rewrittenPlanContext.getRewritePlan();
+ logger.info(currentClassName + "rewrite by materialized view
success");
rewriteResults.add(rewritedPlan);
}
}
return rewriteResults;
}
+ protected boolean checkPartitionIsValid(
+ StructInfo queryInfo,
+ MaterializationContext materializationContext,
+ CascadesContext cascadesContext) {
+ // check partition is valid or not
+ MTMV mtmv = materializationContext.getMtmv();
+ PartitionInfo mvPartitionInfo = mtmv.getPartitionInfo();
+ if (PartitionType.UNPARTITIONED.equals(mvPartitionInfo.getType())) {
+ // if not partition, if rewrite success, it means mv is available
+ return true;
+ }
+ // check mv related table partition is valid or not
+ MTMVPartitionInfo mvCustomPartitionInfo = mtmv.getMvPartitionInfo();
+ BaseTableInfo relatedPartitionTable =
mvCustomPartitionInfo.getRelatedTable();
+ if (relatedPartitionTable == null) {
+ return true;
+ }
+ Optional<LogicalOlapScan> relatedTableRelation =
queryInfo.getRelations().stream()
+ .filter(relation -> relatedPartitionTable.equals(new
BaseTableInfo(relation.getTable()))
+ && relation instanceof LogicalOlapScan)
+ .map(relation -> (LogicalOlapScan) relation)
+ .findFirst();
+ if (!relatedTableRelation.isPresent()) {
+ logger.warn("mv is partition update, but related table relation is
null");
+ return false;
+ }
+ OlapTable relatedTable = relatedTableRelation.get().getTable();
+ Map<Long, Set<Long>> mvToBasePartitionMap;
+ try {
+ mvToBasePartitionMap = MTMVUtil.getMvToBasePartitions(mtmv,
relatedTable);
+ } catch (AnalysisException e) {
+ logger.error("mvRewriteSuccess getMvToBasePartitions fail", e);
Review Comment:
i think should be warning, not error
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -53,6 +64,17 @@
*/
public abstract class AbstractMaterializedViewAggregateRule extends
AbstractMaterializedViewRule {
+ protected static final Map<PlaceholderExpression, PlaceholderExpression>
+ AGGREGATE_ROLL_UP_EQUIVALENT_FUNCTION_MAP = new HashMap<>();
+ protected final String currentClassName = this.getClass().getSimpleName();
Review Comment:
add a blank line between static and non-static attr
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -88,13 +112,14 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
needRollUp =
!queryGroupShuttledExpression.equals(viewGroupShuttledExpression);
}
if (!needRollUp) {
- List<Expression> rewrittenQueryGroupExpr =
rewriteExpression(queryTopPlan.getOutput(),
+ List<Expression> rewrittenQueryGroupExpr =
rewriteExpression(queryTopPlan.getExpressions(),
queryTopPlan,
materializationContext.getMvExprToMvScanExprMapping(),
queryToViewSlotMapping,
true);
- if (rewrittenQueryGroupExpr == null) {
+ if (rewrittenQueryGroupExpr.isEmpty()) {
// can not rewrite, bail out.
+ logger.info(currentClassName + " can not rewrite expression
when not need roll up");
Review Comment:
remove all info logs as we negotiate offline
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/MaterializedViewFilterProjectJoinRule.java:
##########
@@ -0,0 +1,49 @@
+// 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.exploration.mv;
+
+import org.apache.doris.nereids.rules.Rule;
+import org.apache.doris.nereids.rules.RulePromise;
+import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.rules.rewrite.RewriteRuleFactory;
+import org.apache.doris.nereids.trees.plans.Plan;
+import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
+import org.apache.doris.nereids.trees.plans.logical.LogicalJoin;
+import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
+
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+
+/**
+ * This is responsible for join pattern such as filter on project on join
+ */
+public class MaterializedViewFilterProjectJoinRule extends
AbstractMaterializedViewJoinRule
+ implements RewriteRuleFactory {
+
+ public static final MaterializedViewFilterProjectJoinRule INSTANCE = new
MaterializedViewFilterProjectJoinRule();
+
+ @Override
+ public List<Rule> buildRules() {
+ return ImmutableList.of(
+ logicalFilter(logicalProject(logicalJoin(any(),
any()))).thenApplyMulti(ctx -> {
+ LogicalFilter<LogicalProject<LogicalJoin<Plan, Plan>>>
root = ctx.root;
+ return rewrite(root, ctx.cascadesContext);
+ }).toRule(RuleType.MATERIALIZED_VIEW_FILTER_PROJECT_JOIN,
RulePromise.EXPLORE));
Review Comment:
use ExplorationRuleFactory to avoid use RulePromise.EXPLORE explicity
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -36,26 +48,32 @@
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.algebra.CatalogRelation;
import org.apache.doris.nereids.trees.plans.logical.LogicalFilter;
+import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan;
import org.apache.doris.nereids.util.ExpressionUtils;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Sets;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.stream.Collectors;
/**
* The abstract class for all materialized view rules
*/
public abstract class AbstractMaterializedViewRule {
Review Comment:
why not let this class implement ExplorationRuleFactory?
--
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]
