morrySnow commented on code in PR #21883:
URL: https://github.com/apache/doris/pull/21883#discussion_r1267994543
##########
fe/fe-core/src/main/java/org/apache/doris/planner/PlanFragment.java:
##########
@@ -171,8 +170,8 @@ public PlanFragment(PlanFragmentId id, PlanNode root,
DataPartition partition, D
public PlanFragment(PlanFragmentId id, PlanNode root, DataPartition
partition,
Set<RuntimeFilterId> builderRuntimeFilterIds, Set<RuntimeFilterId>
targetRuntimeFilterIds) {
this(id, root, partition);
- this.builderRuntimeFilterIds =
ImmutableSet.copyOf(builderRuntimeFilterIds);
- this.targetRuntimeFilterIds =
ImmutableSet.copyOf(targetRuntimeFilterIds);
+ this.builderRuntimeFilterIds = new HashSet<>(builderRuntimeFilterIds);
Review Comment:
why change to mutable set?
##########
fe/fe-core/src/main/java/org/apache/doris/planner/MultiCastPlanFragment.java:
##########
@@ -28,13 +30,20 @@
public class MultiCastPlanFragment extends PlanFragment {
private final List<ExchangeNode> destNodeList = Lists.newArrayList();
- public MultiCastPlanFragment(PlanFragment planFragment) {
+ private final CTEId cteId;
Review Comment:
if cteId only use to get CTEScanNode from context, u could use FragmentId
directly and avoid use Nereids' class in there
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterGenerator.java:
##########
@@ -147,6 +144,16 @@ public PhysicalPlan
visitPhysicalHashJoin(PhysicalHashJoin<? extends Plan, ? ext
public PhysicalCTEConsumer visitPhysicalCTEConsumer(PhysicalCTEConsumer
scan, CascadesContext context) {
RuntimeFilterContext ctx = context.getRuntimeFilterContext();
scan.getOutput().forEach(slot -> ctx.getAliasTransferMap().put(slot,
Pair.of(scan, slot)));
+ Set<CTEId> processedCTE =
context.getRuntimeFilterContext().getProcessedCTE();
+ CTEId cteId = scan.getCteId();
+ if (!processedCTE.contains(cteId)) {
+ PhysicalCTEProducer cteProducer = context.getRuntimeFilterContext()
+ .getCteProduceMap().get(cteId);
+ PhysicalPlan inputPlanNode = (PhysicalPlan) cteProducer.child(0);
+ // handle cte internal
+ inputPlanNode.accept(this, context);
+ processedCTE.add(cteId);
+ }
Review Comment:
it is weird, if u want process Producer first then process consumer side.
the better way to do that is override visitCteAnchor and control its traverse
order to process its left child first
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/RuntimeFilterTranslator.java:
##########
@@ -157,15 +159,25 @@ public void createLegacyRuntimeFilter(RuntimeFilter
filter, JoinNodeBase node, P
//bitmap rf requires isBroadCast=false, it always requires
merge filter
origFilter.setIsBroadcast(false);
}
- boolean isLocalTarget = scanNodeList.stream().allMatch(e ->
e.getFragmentId().equals(node.getFragmentId()));
+ boolean isLocalTarget = scanNodeList.stream().allMatch(e ->
+ e.getStatisticalType() != StatisticalType.CTE_SCAN_NODE
Review Comment:
```suggestion
!(e instanceof CTEScanNode)
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalSetOperation.java:
##########
@@ -130,4 +137,41 @@ public int getArity() {
return children.size();
}
+ @Override
+ public boolean pushDownRuntimeFilter(CascadesContext context,
IdGenerator<RuntimeFilterId> generator,
Review Comment:
why we need to add this interface into AbstractPlan? i think a good design
transferMap is enough to handle all cases. in our design pattern, we should not
add any optimize rule logic in classes of Plan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
