diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java index 507be9fa8a4470..e178245d58d489 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java @@ -276,7 +276,27 @@ public void mergeToNotOwnerRemove(GroupExpression target) { this.getLowestCostTable() .forEach((properties, pair) -> target.updateLowestCostTable(properties, pair.second, pair.first)); // requestPropertiesMap - target.requestPropertiesMap.putAll(this.requestPropertiesMap); + // ATTN: when do merge, we should update target requestPropertiesMap + // ONLY IF the cost of source's request property lower than target one. + // Otherwise, the requestPropertiesMap will not sync with lowestCostTable. + // Then, we will get wrong output property when get the final plan. + for (Map.Entry entry : requestPropertiesMap.entrySet()) { + PhysicalProperties request = entry.getKey(); + if (!target.requestPropertiesMap.containsKey(request)) { + target.requestPropertiesMap.put(entry.getKey(), entry.getValue()); + } else { + PhysicalProperties sourceOutput = entry.getValue(); + PhysicalProperties targetOutput = target.getRequestPropertiesMap().get(request); + if (this.getLowestCostTable().containsKey(sourceOutput) + && target.getLowestCostTable().containsKey(targetOutput)) { + Cost sourceCost = this.getLowestCostTable().get(sourceOutput).first; + Cost targetCost = target.getLowestCostTable().get(targetOutput).first; + if (sourceCost.getValue() < targetCost.getValue()) { + target.requestPropertiesMap.put(entry.getKey(), entry.getValue()); + } + } + } + } // ruleMasks target.ruleMasks.or(this.ruleMasks); diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/memo/GroupExpressionTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/memo/GroupExpressionTest.java new file mode 100644 index 00000000000000..038cfe8c456094 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/memo/GroupExpressionTest.java @@ -0,0 +1,61 @@ +// 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.memo; + +import org.apache.doris.nereids.cost.Cost; +import org.apache.doris.nereids.properties.PhysicalProperties; +import org.apache.doris.nereids.trees.plans.FakePlan; + +import com.google.common.collect.Lists; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class GroupExpressionTest { + + @Test + public void testMergeToNotOwnerRemoveWhenTargetWithLowerCost() { + GroupExpression source = new GroupExpression(new FakePlan()); + source.updateLowestCostTable(PhysicalProperties.GATHER, Lists.newArrayList(), Cost.infinite()); + source.putOutputPropertiesMap(PhysicalProperties.GATHER, PhysicalProperties.ANY); + + GroupExpression target = new GroupExpression(new FakePlan()); + target.updateLowestCostTable(PhysicalProperties.ANY, Lists.newArrayList(), Cost.zero()); + target.putOutputPropertiesMap(PhysicalProperties.ANY, PhysicalProperties.ANY); + + source.mergeToNotOwnerRemove(target); + Assertions.assertTrue(target.getLowestCostTable().containsKey(PhysicalProperties.ANY)); + Assertions.assertTrue(target.getLowestCostTable().containsKey(PhysicalProperties.GATHER)); + Assertions.assertEquals(PhysicalProperties.ANY, target.getOutputProperties(PhysicalProperties.ANY)); + } + + @Test + public void testMergeToNotOwnerRemoveWhenSourceWithLowerCost() { + GroupExpression source = new GroupExpression(new FakePlan()); + source.updateLowestCostTable(PhysicalProperties.GATHER, Lists.newArrayList(), Cost.zero()); + source.putOutputPropertiesMap(PhysicalProperties.GATHER, PhysicalProperties.ANY); + + GroupExpression target = new GroupExpression(new FakePlan()); + target.updateLowestCostTable(PhysicalProperties.ANY, Lists.newArrayList(), Cost.infinite()); + target.putOutputPropertiesMap(PhysicalProperties.ANY, PhysicalProperties.ANY); + + source.mergeToNotOwnerRemove(target); + Assertions.assertTrue(target.getLowestCostTable().containsKey(PhysicalProperties.ANY)); + Assertions.assertTrue(target.getLowestCostTable().containsKey(PhysicalProperties.GATHER)); + Assertions.assertEquals(PhysicalProperties.GATHER, target.getOutputProperties(PhysicalProperties.ANY)); + } +}