From 45c145fdf7b6c350a7f34b80942bd91e99b3a690 Mon Sep 17 00:00:00 2001 From: morrySnow <101034200+morrySnow@users.noreply.github.com> Date: Mon, 20 May 2024 21:49:53 +0800 Subject: [PATCH] [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 +++---------------- .../trees/plans/logical/LogicalEsScan.java | 18 +---- .../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(-) create mode 100644 fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalExternalRelation.java 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 9f87d6a60a..277f5ae345 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 conjuncts = fileScan.getConjuncts().stream() + LogicalExternalRelation newRelation = relation.withRelationId(StatementScopeIdGenerator.newRelationId()); + updateReplaceMapWithOutput(relation, newRelation, context.exprIdReplaceMap); + Set conjuncts = relation.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 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 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()); - } - Set conjuncts = esScan.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 66882354e2..ea278aa203 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 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 qualifier, Optional groupExpression, Optional logicalProperties, Set 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 qualifier) { @@ -83,6 +79,7 @@ public class LogicalEsScan extends LogicalCatalogRelation { conjuncts); } + @Override public LogicalEsScan withConjuncts(Set 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 accept(PlanVisitor 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 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 0000000000..bc6313f529 --- /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 conjuncts; + + public LogicalExternalRelation(RelationId relationId, PlanType type, TableIf table, List qualifier, + Set conjuncts, + Optional groupExpression, Optional 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 conjuncts); + + @Override + public abstract LogicalExternalRelation withRelationId(RelationId relationId); + + public Set getConjuncts() { + return conjuncts; + } + + @Override + public boolean equals(Object o) { + return super.equals(o) && Objects.equals(conjuncts, ((LogicalExternalRelation) o).conjuncts); + } + + @Override + public R accept(PlanVisitor 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 0b61dd24da..f7bdae5c2f 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 conjuncts; private final SelectedPartitions selectedPartitions; private final Optional tableSample; @@ -55,9 +53,7 @@ public class LogicalFileScan extends LogicalCatalogRelation { public LogicalFileScan(RelationId id, ExternalTable table, List qualifier, Optional groupExpression, Optional logicalProperties, Set conjuncts, SelectedPartitions selectedPartitions, Optional 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 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 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 1f295f37da..cde2b6be24 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 conjuncts; +public class LogicalJdbcScan extends LogicalExternalRelation { /** * Constructor for LogicalJdbcScan. @@ -51,9 +48,7 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { Optional groupExpression, Optional logicalProperties, Set 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 qualifier) { @@ -81,6 +76,7 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { Optional.of(getLogicalProperties()), conjuncts); } + @Override public LogicalJdbcScan withConjuncts(Set conjuncts) { return new LogicalJdbcScan(relationId, table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts); @@ -101,13 +97,4 @@ public class LogicalJdbcScan extends LogicalCatalogRelation { public R accept(PlanVisitor 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 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 1e57ad80c4..414cb335af 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 conjuncts; +public class LogicalOdbcScan extends LogicalExternalRelation { public LogicalOdbcScan(RelationId id, TableIf table, List qualifier, Optional groupExpression, Optional logicalProperties, Set 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 qualifier) { @@ -77,6 +72,7 @@ public class LogicalOdbcScan extends LogicalCatalogRelation { Optional.of(getLogicalProperties()), conjuncts); } + @Override public LogicalOdbcScan withConjuncts(Set conjuncts) { return new LogicalOdbcScan(relationId, table, qualifier, Optional.empty(), Optional.of(getLogicalProperties()), conjuncts); @@ -97,13 +93,4 @@ public class LogicalOdbcScan extends LogicalCatalogRelation { public R accept(PlanVisitor 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 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 95f9c6c96d..046964c351 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 { 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) {