From 3def246846a231408dbc9763de10d32c1f45b37b Mon Sep 17 00:00:00 2001 From: morrySnow Date: Mon, 20 May 2024 18:37:13 +0800 Subject: [PATCH] [fix](Nereids) LogicalPlanDeepCopier copy scan conjuncts in wrong way intro by PR #34894 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 9f87d6a60a69be..277f5ae345a6f8 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.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 Plan visitLogicalDeferMaterializeOlapScan(LogicalDeferMaterializeOlapScan .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 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 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()); + public Plan visitLogicalExternalRelation(LogicalExternalRelation relation, + DeepCopierContext context) { + if (context.getRelationReplaceMap().containsKey(relation.getRelationId())) { + return context.getRelationReplaceMap().get(relation.getRelationId()); } - Set conjuncts = esScan.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()); - 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 66882354e20415..ea278aa203c438 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.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 Plan withGroupExprLogicalPropChildren(Optional groupExpr 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 LogicalEsScan withRelationId(RelationId relationId) { 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 00000000000000..bc6313f5291044 --- /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 0b61dd24dac3cf..f7bdae5c2f3bf3 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 @@ /** * 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 LogicalFileScan(RelationId id, ExternalTable table, List qualifie Sets.newHashSet(), SelectedPartitions.NOT_PRUNED, tableSample); } - public Set getConjuncts() { - return conjuncts; - } - public SelectedPartitions getSelectedPartitions() { return selectedPartitions; } @@ -108,6 +100,7 @@ public Plan withGroupExprLogicalPropChildren(Optional groupExpr 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 R accept(PlanVisitor visitor, C context) { @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 1f295f37da7c8d..cde2b6be242a08 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.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 LogicalJdbcScan(RelationId id, TableIf table, List qualifier, 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 LogicalJdbcScan withGroupExpression(Optional groupExpres 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 LogicalJdbcScan withRelationId(RelationId relationId) { 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 1e57ad80c45601..414cb335af15b6 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.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 LogicalOdbcScan withGroupExpression(Optional groupExpres 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 LogicalOdbcScan withRelationId(RelationId relationId) { 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 95f9c6c96d6f73..046964c351d60a 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.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 @@ default R visitLogicalEmptyRelation(LogicalEmptyRelation emptyRelation, C contex 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) {