walterddr commented on code in PR #11885:
URL: https://github.com/apache/pinot/pull/11885#discussion_r1374827733
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/PlanFragmenter.java:
##########
@@ -128,6 +128,8 @@ public PlanNode visitExchange(ExchangeNode node, Context
context) {
}
int currentPlanFragmentId = context._previousPlanFragmentId;
int nextPlanFragmentId = ++context._currentPlanFragmentId;
+ // Set previous PlanFragment ID in the context to be the next PlanFragment
ID to be used by the child node.
+ context._previousPlanFragmentId = nextPlanFragmentId;
Review Comment:
the logic of previous/current plan fragment id looks very convoluted
in normal node, _previousPlanFragmentId is set to the "node itself before it
visit its children, thus guaranteed at the time child is being visit, current
is always what the child is suppose to be on, and the previous is what the
parent suppose to be on.
in exchange `_previousPlanFragmentId` setting to the next plan fragment id
seems weird.
the actual problem is once exchange visits the child, the plan break into
fragment's and thus --> it should be named "parent plan fragment id" and b/c
mailbox send doesn't have a parent it is defined as a the "virtual node" sits
on top of the MailboxSend and granted the same id as the current node
^which still seems weird, i wonder keeping this as a global context might
not be the best way
--
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]
