Skip to content

Commit

Permalink
[fix](Nereids) repeat's output properties is not right (#44297)
Browse files Browse the repository at this point in the history 
Related PR: #37219

Problem Summary:
when child's output is hash distribution, repeat's output follow child
only when all child's hash columns in all repeat set. otherwise,
repeat's output should be any.
  • Loading branch information
@morrySnow
morrySnow authored and Your Name committed Nov 20, 2024
1 parent 2332c5a commit 7cd2ecf
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 15 deletions.
5 changes: 3 additions & 2 deletions fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,9 @@ public PhysicalProperties visitPhysicalRepeat(PhysicalRepeat<? extends Plan> rep
intersectGroupingKeys, Utils.fastToImmutableSet(groupingSets.get(i))
);
}
if (!intersectGroupingKeys.isEmpty()) {
List<ExprId> orderedShuffledColumns = distributionSpecHash.getOrderedShuffledColumns();
List<ExprId> orderedShuffledColumns = distributionSpecHash.getOrderedShuffledColumns();
if (!intersectGroupingKeys.isEmpty() && intersectGroupingKeys.size()
>= Sets.newHashSet(orderedShuffledColumns).size()) {
boolean hashColumnsChanged = false;
for (Expression intersectGroupingKey : intersectGroupingKeys) {
if (!(intersectGroupingKey instanceof SlotReference)) {
Expand Down
74 changes: 61 additions & 13 deletions ...ore/src/test/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriverTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@
import org.apache.doris.nereids.trees.plans.physical.PhysicalLimit;
import org.apache.doris.nereids.trees.plans.physical.PhysicalNestedLoopJoin;
import org.apache.doris.nereids.trees.plans.physical.PhysicalQuickSort;
import org.apache.doris.nereids.trees.plans.physical.PhysicalRepeat;
import org.apache.doris.nereids.trees.plans.physical.PhysicalTopN;
import org.apache.doris.nereids.types.BigIntType;
import org.apache.doris.nereids.types.IntegerType;
import org.apache.doris.nereids.types.TinyIntType;
import org.apache.doris.nereids.util.ExpressionUtils;
import org.apache.doris.nereids.util.JoinUtils;
import org.apache.doris.qe.ConnectContext;
Expand Down Expand Up @@ -133,7 +135,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -172,7 +174,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -211,7 +213,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -251,7 +253,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -291,7 +293,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -331,7 +333,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -371,7 +373,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
Assertions.assertEquals(-1, actual.getTableId());
Expand Down Expand Up @@ -412,7 +414,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
Assertions.assertEquals(-1, actual.getTableId());
Expand Down Expand Up @@ -453,7 +455,7 @@ ExpressionUtils.EMPTY_CONDITION, ExpressionUtils.EMPTY_CONDITION, new Distribute

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
Assertions.assertEquals(-1, actual.getTableId());
Expand Down Expand Up @@ -576,7 +578,7 @@ ExpressionUtils.EMPTY_CONDITION, new DistributeHint(DistributeType.NONE),

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.NATURAL, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -626,7 +628,7 @@ Pair<List<ExprId>, List<ExprId>> getOnClauseUsedSlots(

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.EXECUTION_BUCKETED, actual.getShuffleType());
// check merged
Expand Down Expand Up @@ -659,7 +661,7 @@ void testNestedLoopJoin() {

PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(leftHash, actual);
}
Expand Down Expand Up @@ -712,7 +714,7 @@ void testGlobalPhaseAggregate() {
ChildOutputPropertyDeriver deriver = new ChildOutputPropertyDeriver(Lists.newArrayList(child));
PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertTrue(result.getOrderSpec().getOrderKeys().isEmpty());
Assertions.assertTrue(result.getDistributionSpec() instanceof DistributionSpecHash);
Assertions.assertInstanceOf(DistributionSpecHash.class, result.getDistributionSpec());
DistributionSpecHash actual = (DistributionSpecHash) result.getDistributionSpec();
Assertions.assertEquals(ShuffleType.EXECUTION_BUCKETED, actual.getShuffleType());
Assertions.assertEquals(Lists.newArrayList(partition).stream()
Expand Down Expand Up @@ -837,4 +839,50 @@ void testAssertNumRows() {
PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertEquals(PhysicalProperties.GATHER, result);
}

@Test
void testRepeatReturnAny() {
SlotReference c1 = new SlotReference(
new ExprId(1), "c1", TinyIntType.INSTANCE, true, ImmutableList.of());
SlotReference c2 = new SlotReference(
new ExprId(2), "c2", TinyIntType.INSTANCE, true, ImmutableList.of());
SlotReference c3 = new SlotReference(
new ExprId(3), "c3", TinyIntType.INSTANCE, true, ImmutableList.of());
PhysicalRepeat<GroupPlan> repeat = new PhysicalRepeat<>(
ImmutableList.of(ImmutableList.of(c1, c2), ImmutableList.of(c1), ImmutableList.of(c1, c3)),
ImmutableList.of(c1, c2, c3),
logicalProperties,
groupPlan
);
GroupExpression groupExpression = new GroupExpression(repeat);
new Group(null, groupExpression, null);
PhysicalProperties child = PhysicalProperties.createHash(
ImmutableList.of(new ExprId(1), new ExprId(2)), ShuffleType.EXECUTION_BUCKETED);
ChildOutputPropertyDeriver deriver = new ChildOutputPropertyDeriver(Lists.newArrayList(child));
PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertEquals(PhysicalProperties.ANY, result);
}

@Test
void testRepeatReturnChild() {
SlotReference c1 = new SlotReference(
new ExprId(1), "c1", TinyIntType.INSTANCE, true, ImmutableList.of());
SlotReference c2 = new SlotReference(
new ExprId(2), "c2", TinyIntType.INSTANCE, true, ImmutableList.of());
SlotReference c3 = new SlotReference(
new ExprId(3), "c3", TinyIntType.INSTANCE, true, ImmutableList.of());
PhysicalRepeat<GroupPlan> repeat = new PhysicalRepeat<>(
ImmutableList.of(ImmutableList.of(c1, c2), ImmutableList.of(c1), ImmutableList.of(c1, c3)),
ImmutableList.of(c1, c2, c3),
logicalProperties,
groupPlan
);
GroupExpression groupExpression = new GroupExpression(repeat);
new Group(null, groupExpression, null);
PhysicalProperties child = PhysicalProperties.createHash(
ImmutableList.of(new ExprId(1)), ShuffleType.EXECUTION_BUCKETED);
ChildOutputPropertyDeriver deriver = new ChildOutputPropertyDeriver(Lists.newArrayList(child));
PhysicalProperties result = deriver.getOutputProperties(null, groupExpression);
Assertions.assertEquals(child, result);
}
}

0 comments on commit 7cd2ecf

Please sign in to comment.