Skip to content

Commit 28b2d23

Browse files
authored
[chore](cte) use a better way to get child in enforce regulator (#59395)
related PR #58964 Problem Summary: This pull request refactors how child physical plans are accessed in the `ChildrenPropertiesRegulator` class, simplifying the code and improving test clarity. The main change is the removal of the `getChildPhysicalPlan` helper method, replacing its usage with direct access to the plan from the `children` list. The tests are also updated to build child mocks more locally, improving test isolation and readability. Refactoring and Simplification: * Removed the `getChildPhysicalPlan` method from `ChildrenPropertiesRegulator`, and replaced its usage in `visitPhysicalFilter` and `visitPhysicalProject` with direct access to the child plan via `children.get(0).getPlan()`. This simplifies the code by eliminating unnecessary indirection.
1 parent 5df1162 commit 28b2d23

File tree

2 files changed

+28
-32
lines changed

2 files changed

+28
-32
lines changed

fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -281,8 +281,7 @@ public List<List<PhysicalProperties>> visitPhysicalFilter(PhysicalFilter<? exten
281281
DistributionSpec distributionSpec = originChildrenProperties.get(0).getDistributionSpec();
282282
// process must shuffle
283283
if (distributionSpec instanceof DistributionSpecMustShuffle) {
284-
Plan child = filter.child();
285-
Plan realChild = getChildPhysicalPlan(child);
284+
Plan realChild = children.get(0).getPlan();
286285
if (realChild instanceof PhysicalProject
287286
|| realChild instanceof PhysicalFilter
288287
|| realChild instanceof PhysicalLimit) {
@@ -320,19 +319,6 @@ private boolean isBucketShuffleDownGrade(Plan oneSidePlan, DistributionSpecHash
320319
}
321320
}
322321

323-
private Plan getChildPhysicalPlan(Plan plan) {
324-
if (!(plan instanceof GroupPlan)) {
325-
return null;
326-
}
327-
GroupPlan groupPlan = (GroupPlan) plan;
328-
if (groupPlan == null || groupPlan.getGroup() == null
329-
|| groupPlan.getGroup().getPhysicalExpressions().isEmpty()) {
330-
return null;
331-
} else {
332-
return groupPlan.getGroup().getPhysicalExpressions().get(0).getPlan();
333-
}
334-
}
335-
336322
private PhysicalOlapScan findDownGradeBucketShuffleCandidate(GroupPlan groupPlan) {
337323
if (groupPlan == null || groupPlan.getGroup() == null
338324
|| groupPlan.getGroup().getPhysicalExpressions().isEmpty()) {
@@ -602,8 +588,7 @@ public List<List<PhysicalProperties>> visitPhysicalProject(PhysicalProject<? ext
602588
DistributionSpec distributionSpec = originChildrenProperties.get(0).getDistributionSpec();
603589
// process must shuffle
604590
if (distributionSpec instanceof DistributionSpecMustShuffle) {
605-
Plan child = project.child();
606-
Plan realChild = getChildPhysicalPlan(child);
591+
Plan realChild = children.get(0).getPlan();
607592
if (realChild instanceof PhysicalLimit) {
608593
visit(project, context);
609594
} else if (realChild instanceof PhysicalProject) {

fe/fe-core/src/test/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulatorTest.java

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -47,44 +47,29 @@
4747

4848
public class ChildrenPropertiesRegulatorTest {
4949

50-
private List<GroupExpression> children;
5150
private JobContext mockedJobContext;
5251
private List<PhysicalProperties> originOutputChildrenProperties
5352
= Lists.newArrayList(PhysicalProperties.MUST_SHUFFLE);
5453

5554
@BeforeEach
5655
public void setUp() {
57-
Group childGroup = Mockito.mock(Group.class);
58-
Mockito.when(childGroup.getLogicalProperties()).thenReturn(Mockito.mock(LogicalProperties.class));
59-
GroupExpression child = Mockito.mock(GroupExpression.class);
60-
Mockito.when(child.getOutputProperties(Mockito.any())).thenReturn(PhysicalProperties.MUST_SHUFFLE);
61-
Mockito.when(child.getOwnerGroup()).thenReturn(childGroup);
62-
Map<PhysicalProperties, Pair<Cost, List<PhysicalProperties>>> lct = Maps.newHashMap();
63-
lct.put(PhysicalProperties.MUST_SHUFFLE, Pair.of(Cost.zero(), Lists.newArrayList()));
64-
Mockito.when(child.getLowestCostTable()).thenReturn(lct);
65-
children = Lists.newArrayList(child);
66-
6756
mockedJobContext = Mockito.mock(JobContext.class);
6857
Mockito.when(mockedJobContext.getCascadesContext()).thenReturn(Mockito.mock(CascadesContext.class));
69-
7058
}
7159

7260
@Test
7361
public void testMustShuffleProjectProjectCanNotMerge() {
7462
testMustShuffleProject(PhysicalProject.class, DistributionSpecExecutionAny.class, false);
75-
7663
}
7764

7865
@Test
7966
public void testMustShuffleProjectProjectCanMerge() {
8067
testMustShuffleProject(PhysicalProject.class, DistributionSpecMustShuffle.class, true);
81-
8268
}
8369

8470
@Test
8571
public void testMustShuffleProjectFilter() {
8672
testMustShuffleProject(PhysicalFilter.class, DistributionSpecMustShuffle.class, true);
87-
8873
}
8974

9075
@Test
@@ -111,6 +96,19 @@ public void testMustShuffleProject(Class<? extends Plan> childClazz,
11196
Mockito.when(mockedGroupPlan.getGroup()).thenReturn(mockedGroup);
11297
// let AbstractTreeNode's init happy
11398
Mockito.when(mockedGroupPlan.getAllChildrenTypes()).thenReturn(new BitSet());
99+
100+
List<GroupExpression> children;
101+
Group childGroup = Mockito.mock(Group.class);
102+
Mockito.when(childGroup.getLogicalProperties()).thenReturn(Mockito.mock(LogicalProperties.class));
103+
GroupExpression child = Mockito.mock(GroupExpression.class);
104+
Mockito.when(child.getOutputProperties(Mockito.any())).thenReturn(PhysicalProperties.MUST_SHUFFLE);
105+
Mockito.when(child.getOwnerGroup()).thenReturn(childGroup);
106+
Map<PhysicalProperties, Pair<Cost, List<PhysicalProperties>>> lct = Maps.newHashMap();
107+
lct.put(PhysicalProperties.MUST_SHUFFLE, Pair.of(Cost.zero(), Lists.newArrayList()));
108+
Mockito.when(child.getLowestCostTable()).thenReturn(lct);
109+
Mockito.when(child.getPlan()).thenReturn(mockedChild);
110+
children = Lists.newArrayList(child);
111+
114112
PhysicalProject parentPlan = new PhysicalProject<>(Lists.newArrayList(), null, mockedGroupPlan);
115113
GroupExpression parent = new GroupExpression(parentPlan);
116114
parentPlan = parentPlan.withGroupExpression(Optional.of(parent));
@@ -157,6 +155,19 @@ private void testMustShuffleFilter(Class<? extends Plan> childClazz) {
157155
Mockito.when(mockedGroupPlan.getGroup()).thenReturn(mockedGroup);
158156
// let AbstractTreeNode's init happy
159157
Mockito.when(mockedGroupPlan.getAllChildrenTypes()).thenReturn(new BitSet());
158+
159+
List<GroupExpression> children;
160+
Group childGroup = Mockito.mock(Group.class);
161+
Mockito.when(childGroup.getLogicalProperties()).thenReturn(Mockito.mock(LogicalProperties.class));
162+
GroupExpression child = Mockito.mock(GroupExpression.class);
163+
Mockito.when(child.getOutputProperties(Mockito.any())).thenReturn(PhysicalProperties.MUST_SHUFFLE);
164+
Mockito.when(child.getOwnerGroup()).thenReturn(childGroup);
165+
Map<PhysicalProperties, Pair<Cost, List<PhysicalProperties>>> lct = Maps.newHashMap();
166+
lct.put(PhysicalProperties.MUST_SHUFFLE, Pair.of(Cost.zero(), Lists.newArrayList()));
167+
Mockito.when(child.getLowestCostTable()).thenReturn(lct);
168+
Mockito.when(child.getPlan()).thenReturn(mockedChild);
169+
children = Lists.newArrayList(child);
170+
160171
GroupExpression parent = new GroupExpression(new PhysicalFilter<>(Sets.newHashSet(), null, mockedGroupPlan));
161172
ChildrenPropertiesRegulator regulator = new ChildrenPropertiesRegulator(parent, children,
162173
new ArrayList<>(originOutputChildrenProperties), null, mockedJobContext);

0 commit comments

Comments
 (0)