This is an automated email from the ASF dual-hosted git repository. yiguolei pushed a commit to branch branch-2.1 in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/branch-2.1 by this push: new 45c145fdf7b [fix](Nereids) LogicalPlanDeepCopier copy scan conjuncts in wrong way (#35077) 45c145fdf7b is described below commit 45c145fdf7b6c350a7f34b80942bd91e99b3a690 Author: morrySnow <101034200+morrys...@users.noreply.github.com> AuthorDate: Mon May 20 21:49:53 2024 +0800 [fix](Nereids) LogicalPlanDeepCopier copy scan conjuncts in wrong way (#35077) pick from master #35076 intro by PR #34933 This PR attempts to address the issue of losing conjuncts when performing a deep copy of the outer structure. However, the timing of copying the conjuncts is incorrect, resulting in the inability to map slots within the conjuncts to the output of the outer structure. --- .../trees/copier/LogicalPlanDeepCopier.java | 73 ++++------------------ .../nereids/trees/plans/logical/LogicalEsScan.java | 18 +----- .../plans/logical/LogicalExternalRelation.java | 68 ++++++++++++++++++++ .../trees/plans/logical/LogicalFileScan.java | 16 ++--- .../trees/plans/logical/LogicalJdbcScan.java | 19 +----- .../trees/plans/logical/LogicalOdbcScan.java | 19 +----- .../trees/plans/visitor/RelationVisitor.java | 13 ++-- 7 files changed, 102 insertions(+), 124 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/copier/LogicalPlanDeepCopier.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/copier/LogicalPlanDeepCopier.java index 9f87d6a60a6..277f5ae345a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/copier/LogicalPlanDeepCopier.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/copier/LogicalPlanDeepCopier.java @@ -39,17 +39,14 @@ import org.apache.doris.nereids.trees.plans.logical.LogicalCTEProducer; import org.apache.doris.nereids.trees.plans.logical.LogicalDeferMaterializeOlapScan; import org.apache.doris.nereids.trees.plans.logical.LogicalDeferMaterializeTopN; import org.apache.doris.nereids.trees.plans.logical.LogicalEmptyRelation; -import org.apache.doris.nereids.trees.plans.logical.LogicalEsScan; import org.apache.doris.nereids.trees.plans.logical.LogicalExcept; -import org.apache.doris.nereids.trees.plans.logical.LogicalFileScan; +import org.apache.doris.nereids.trees.plans.logical.LogicalExternalRelation; import org.apache.doris.nereids.trees.plans.logical.LogicalFilter; import org.apache.doris.nereids.trees.plans.logical.LogicalGenerate; import org.apache.doris.nereids.trees.plans.logical.LogicalHaving; import org.apache.doris.nereids.trees.plans.logical.LogicalIntersect; -import org.apache.doris.nereids.trees.plans.logical.LogicalJdbcScan; import org.apache.doris.nereids.trees.plans.logical.LogicalJoin; import org.apache.doris.nereids.trees.plans.logical.LogicalLimit; -import org.apache.doris.nereids.trees.plans.logical.LogicalOdbcScan; import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan; import org.apache.doris.nereids.trees.plans.logical.LogicalOneRowRelation; import org.apache.doris.nereids.trees.plans.logical.LogicalPartitionTopN; @@ -187,69 +184,23 @@ public class LogicalPlanDeepCopier extends DefaultPlanRewriter<DeepCopierContext .collect(ImmutableSet.toImmutableSet()); SlotReference newRowId = (SlotReference) ExpressionDeepCopier.INSTANCE .deepCopy(deferMaterializeOlapScan.getColumnIdSlot(), context); - LogicalDeferMaterializeOlapScan newMaterializeOlapScan = - new LogicalDeferMaterializeOlapScan(newScan, newSlotIds, newRowId); - return newMaterializeOlapScan; + return new LogicalDeferMaterializeOlapScan(newScan, newSlotIds, newRowId); } @Override - public Plan visitLogicalFileScan(LogicalFileScan fileScan, DeepCopierContext context) { - if (context.getRelationReplaceMap().containsKey(fileScan.getRelationId())) { - return context.getRelationReplaceMap().get(fileScan.getRelationId()); - } - Set<Expression> conjuncts = fileScan.getConjuncts().stream() - .map(p -> ExpressionDeepCopier.INSTANCE.deepCopy(p, context)) - .collect(ImmutableSet.toImmutableSet()); - LogicalFileScan newFileScan = fileScan.withConjuncts(conjuncts) - .withRelationId(StatementScopeIdGenerator.newRelationId()); - updateReplaceMapWithOutput(fileScan, newFileScan, context.exprIdReplaceMap); - context.putRelation(fileScan.getRelationId(), newFileScan); - return newFileScan; - } - - @Override - public Plan visitLogicalJdbcScan(LogicalJdbcScan jdbcScan, DeepCopierContext context) { - if (context.getRelationReplaceMap().containsKey(jdbcScan.getRelationId())) { - return context.getRelationReplaceMap().get(jdbcScan.getRelationId()); - } - Set<Expression> conjuncts = jdbcScan.getConjuncts().stream() - .map(p -> ExpressionDeepCopier.INSTANCE.deepCopy(p, context)) - .collect(ImmutableSet.toImmutableSet()); - LogicalJdbcScan newJdbcScan = jdbcScan.withConjuncts(conjuncts) - .withRelationId(StatementScopeIdGenerator.newRelationId()); - updateReplaceMapWithOutput(jdbcScan, newJdbcScan, context.exprIdReplaceMap); - context.putRelation(jdbcScan.getRelationId(), newJdbcScan); - return newJdbcScan; - } - - @Override - public Plan visitLogicalOdbcScan(LogicalOdbcScan odbcScan, DeepCopierContext context) { - if (context.getRelationReplaceMap().containsKey(odbcScan.getRelationId())) { - return context.getRelationReplaceMap().get(odbcScan.getRelationId()); - } - Set<Expression> conjuncts = odbcScan.getConjuncts().stream() - .map(p -> ExpressionDeepCopier.INSTANCE.deepCopy(p, context)) - .collect(ImmutableSet.toImmutableSet()); - LogicalOdbcScan newOdbcScan = odbcScan.withConjuncts(conjuncts) - .withRelationId(StatementScopeIdGenerator.newRelationId()); - updateReplaceMapWithOutput(odbcScan, newOdbcScan, context.exprIdReplaceMap); - context.putRelation(odbcScan.getRelationId(), newOdbcScan); - return newOdbcScan; - } - - @Override - public Plan visitLogicalEsScan(LogicalEsScan esScan, DeepCopierContext context) { - if (context.getRelationReplaceMap().containsKey(esScan.getRelationId())) { - return context.getRelationReplaceMap().get(esScan.getRelationId()); + public Plan visitLogicalExternalRelation(LogicalExternalRelation relation, + DeepCopierContext context) { + if (context.getRelationReplaceMap().containsKey(relation.getRelationId())) { + return context.getRelationReplaceMap().get(relation.getRelationId()); } - Set<Expression> conjuncts = esScan.getConjuncts().stream() + LogicalExternalRelation newRelation = relation.withRelationId(StatementScopeIdGenerator.newRelationId()); + updateReplaceMapWithOutput(relation, newRelation, context.exprIdReplaceMap); + Set<Expression> conjuncts = relation.getConjuncts().stream() .map(p -> ExpressionDeepCopier.INSTANCE.deepCopy(p, context)) .collect(ImmutableSet.toImmutableSet()); - LogicalEsScan newEsScan = esScan.withConjuncts(conjuncts) - .withRelationId(StatementScopeIdGenerator.newRelationId()); - updateReplaceMapWithOutput(esScan, newEsScan, context.exprIdReplaceMap); - context.putRelation(esScan.getRelationId(), newEsScan); - return newEsScan; + newRelation = newRelation.withConjuncts(conjuncts); + context.putRelation(relation.getRelationId(), newRelation); + return newRelation; } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalEsScan.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalEsScan.java index 66882354e20..ea278aa203c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalEsScan.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalEsScan.java @@ -31,16 +31,13 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.Set; /** * Logical scan for external es catalog. */ -public class LogicalEsScan extends LogicalCatalogRelation { - - private final Set<Expression> conjuncts; +public class LogicalEsScan extends LogicalExternalRelation { /** * Constructor for LogicalEsScan. @@ -48,8 +45,7 @@ public class LogicalEsScan extends LogicalCatalogRelation { public LogicalEsScan(RelationId id, ExternalTable table, List<String> qualifier, Optional<GroupExpression> groupExpression, Optional<LogicalProperties> logicalProperties, Set<Expression> conjuncts) { - super(id, PlanType.LOGICAL_ES_SCAN, table, qualifier, groupExpression, logicalProperties); - this.conjuncts = ImmutableSet.copyOf(Objects.requireNonNull(conjuncts, "conjuncts should not be null")); + super(id, PlanType.LOGICAL_ES_SCAN, table, qualifier, conjuncts, groupExpression, logicalProperties); } public LogicalEsScan(RelationId id, ExternalTable table, List<String> qualifier) { @@ -83,6 +79,7 @@ public class LogicalEsScan extends LogicalCatalogRelation { conjuncts); } + @Override public LogicalEsScan withConjuncts(Set<Expression> conjuncts) { return new LogicalEsScan(relationId, (ExternalTable) table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts); @@ -98,13 +95,4 @@ public class LogicalEsScan extends LogicalCatalogRelation { public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { return visitor.visitLogicalEsScan(this, context); } - - @Override - public boolean equals(Object o) { - return super.equals(o) && Objects.equals(conjuncts, ((LogicalEsScan) o).conjuncts); - } - - public Set<Expression> getConjuncts() { - return this.conjuncts; - } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRelation.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRelation.java new file mode 100644 index 00000000000..bc6313f5291 --- /dev/null +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRelation.java @@ -0,0 +1,68 @@ +// 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.catalog.TableIf; +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.plans.PlanType; +import org.apache.doris.nereids.trees.plans.RelationId; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; + +import com.google.common.collect.ImmutableSet; + +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +/** + * abstract class catalog relation for logical relation + */ +public abstract class LogicalExternalRelation extends LogicalCatalogRelation { + + // TODO remove this conjuncts when old planner is removed + protected final Set<Expression> conjuncts; + + public LogicalExternalRelation(RelationId relationId, PlanType type, TableIf table, List<String> qualifier, + Set<Expression> conjuncts, + Optional<GroupExpression> groupExpression, Optional<LogicalProperties> logicalProperties) { + super(relationId, type, table, qualifier, groupExpression, logicalProperties); + this.conjuncts = ImmutableSet.copyOf(Objects.requireNonNull(conjuncts, "conjuncts should not be null")); + } + + public abstract LogicalExternalRelation withConjuncts(Set<Expression> conjuncts); + + @Override + public abstract LogicalExternalRelation withRelationId(RelationId relationId); + + public Set<Expression> getConjuncts() { + return conjuncts; + } + + @Override + public boolean equals(Object o) { + return super.equals(o) && Objects.equals(conjuncts, ((LogicalExternalRelation) o).conjuncts); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitLogicalExternalRelation(this, context); + } +} diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java index 0b61dd24dac..f7bdae5c2f3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFileScan.java @@ -42,10 +42,8 @@ import java.util.Set; /** * Logical file scan for external catalog. */ -public class LogicalFileScan extends LogicalCatalogRelation { +public class LogicalFileScan extends LogicalExternalRelation { - // TODO remove this conjuncts when old planner is removed - private final Set<Expression> conjuncts; private final SelectedPartitions selectedPartitions; private final Optional<TableSample> tableSample; @@ -55,9 +53,7 @@ public class LogicalFileScan extends LogicalCatalogRelation { public LogicalFileScan(RelationId id, ExternalTable table, List<String> qualifier, Optional<GroupExpression> groupExpression, Optional<LogicalProperties> logicalProperties, Set<Expression> conjuncts, SelectedPartitions selectedPartitions, Optional<TableSample> tableSample) { - super(id, PlanType.LOGICAL_FILE_SCAN, table, qualifier, - groupExpression, logicalProperties); - this.conjuncts = conjuncts; + super(id, PlanType.LOGICAL_FILE_SCAN, table, qualifier, conjuncts, groupExpression, logicalProperties); this.selectedPartitions = selectedPartitions; this.tableSample = tableSample; } @@ -68,10 +64,6 @@ public class LogicalFileScan extends LogicalCatalogRelation { Sets.newHashSet(), SelectedPartitions.NOT_PRUNED, tableSample); } - public Set<Expression> getConjuncts() { - return conjuncts; - } - public SelectedPartitions getSelectedPartitions() { return selectedPartitions; } @@ -108,6 +100,7 @@ public class LogicalFileScan extends LogicalCatalogRelation { groupExpression, logicalProperties, conjuncts, selectedPartitions, tableSample); } + @Override public LogicalFileScan withConjuncts(Set<Expression> conjuncts) { return new LogicalFileScan(relationId, (ExternalTable) table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts, selectedPartitions, tableSample); @@ -131,8 +124,7 @@ public class LogicalFileScan extends LogicalCatalogRelation { @Override public boolean equals(Object o) { - return super.equals(o) && Objects.equals(conjuncts, ((LogicalFileScan) o).conjuncts) - && Objects.equals(selectedPartitions, ((LogicalFileScan) o).selectedPartitions); + return super.equals(o) && Objects.equals(selectedPartitions, ((LogicalFileScan) o).selectedPartitions); } /** diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJdbcScan.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJdbcScan.java index 1f295f37da7..cde2b6be242 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJdbcScan.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJdbcScan.java @@ -33,16 +33,13 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.Set; /** * Logical scan for external jdbc catalog and jdbc table. */ -public class LogicalJdbcScan extends LogicalCatalogRelation { - - private final Set<Expression> conjuncts; +public class LogicalJdbcScan extends LogicalExternalRelation { /** * Constructor for LogicalJdbcScan. @@ -51,9 +48,7 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { Optional<GroupExpression> groupExpression, Optional<LogicalProperties> logicalProperties, Set<Expression> conjuncts) { - super(id, PlanType.LOGICAL_JDBC_SCAN, table, qualifier, - groupExpression, logicalProperties); - this.conjuncts = ImmutableSet.copyOf(Objects.requireNonNull(conjuncts, "conjuncts should not be null")); + super(id, PlanType.LOGICAL_JDBC_SCAN, table, qualifier, conjuncts, groupExpression, logicalProperties); } public LogicalJdbcScan(RelationId id, TableIf table, List<String> qualifier) { @@ -81,6 +76,7 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { Optional.of(getLogicalProperties()), conjuncts); } + @Override public LogicalJdbcScan withConjuncts(Set<Expression> conjuncts) { return new LogicalJdbcScan(relationId, table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts); @@ -101,13 +97,4 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { return visitor.visitLogicalJdbcScan(this, context); } - - @Override - public boolean equals(Object o) { - return super.equals(o) && Objects.equals(conjuncts, ((LogicalJdbcScan) o).conjuncts); - } - - public Set<Expression> getConjuncts() { - return this.conjuncts; - } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOdbcScan.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOdbcScan.java index 1e57ad80c45..414cb335af1 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOdbcScan.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalOdbcScan.java @@ -32,24 +32,19 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.Set; /** * Logical scan for external odbc table. */ -public class LogicalOdbcScan extends LogicalCatalogRelation { - - private final Set<Expression> conjuncts; +public class LogicalOdbcScan extends LogicalExternalRelation { public LogicalOdbcScan(RelationId id, TableIf table, List<String> qualifier, Optional<GroupExpression> groupExpression, Optional<LogicalProperties> logicalProperties, Set<Expression> conjuncts) { - super(id, PlanType.LOGICAL_ODBC_SCAN, table, qualifier, - groupExpression, logicalProperties); - this.conjuncts = ImmutableSet.copyOf(Objects.requireNonNull(conjuncts, "conjuncts should not be null")); + super(id, PlanType.LOGICAL_ODBC_SCAN, table, qualifier, conjuncts, groupExpression, logicalProperties); } public LogicalOdbcScan(RelationId id, TableIf table, List<String> qualifier) { @@ -77,6 +72,7 @@ public class LogicalOdbcScan extends LogicalCatalogRelation { Optional.of(getLogicalProperties()), conjuncts); } + @Override public LogicalOdbcScan withConjuncts(Set<Expression> conjuncts) { return new LogicalOdbcScan(relationId, table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts); @@ -97,13 +93,4 @@ public class LogicalOdbcScan extends LogicalCatalogRelation { public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { return visitor.visitLogicalOdbcScan(this, context); } - - @Override - public boolean equals(Object o) { - return super.equals(o) && Objects.equals(conjuncts, ((LogicalOdbcScan) o).conjuncts); - } - - public Set<Expression> getConjuncts() { - return this.conjuncts; - } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/visitor/RelationVisitor.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/visitor/RelationVisitor.java index 95f9c6c96d6..046964c351d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/visitor/RelationVisitor.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/visitor/RelationVisitor.java @@ -24,6 +24,7 @@ import org.apache.doris.nereids.trees.plans.logical.LogicalCatalogRelation; import org.apache.doris.nereids.trees.plans.logical.LogicalDeferMaterializeOlapScan; import org.apache.doris.nereids.trees.plans.logical.LogicalEmptyRelation; import org.apache.doris.nereids.trees.plans.logical.LogicalEsScan; +import org.apache.doris.nereids.trees.plans.logical.LogicalExternalRelation; import org.apache.doris.nereids.trees.plans.logical.LogicalFileScan; import org.apache.doris.nereids.trees.plans.logical.LogicalJdbcScan; import org.apache.doris.nereids.trees.plans.logical.LogicalOdbcScan; @@ -92,20 +93,24 @@ public interface RelationVisitor<R, C> { return visitLogicalRelation(emptyRelation, context); } + default R visitLogicalExternalRelation(LogicalExternalRelation relation, C context) { + return visitLogicalCatalogRelation(relation, context); + } + default R visitLogicalEsScan(LogicalEsScan esScan, C context) { - return visitLogicalCatalogRelation(esScan, context); + return visitLogicalExternalRelation(esScan, context); } default R visitLogicalFileScan(LogicalFileScan fileScan, C context) { - return visitLogicalCatalogRelation(fileScan, context); + return visitLogicalExternalRelation(fileScan, context); } default R visitLogicalJdbcScan(LogicalJdbcScan jdbcScan, C context) { - return visitLogicalCatalogRelation(jdbcScan, context); + return visitLogicalExternalRelation(jdbcScan, context); } default R visitLogicalOdbcScan(LogicalOdbcScan odbcScan, C context) { - return visitLogicalCatalogRelation(odbcScan, context); + return visitLogicalExternalRelation(odbcScan, context); } default R visitLogicalOlapScan(LogicalOlapScan olapScan, C context) { --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org