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.
thus "previousPlanFragmentId" really should be named "plan fragment id for
the parent PlanNode": the actual problem is
- once exchange visits the child, the plan break into fragments
- b/c mailbox send doesn't have a parent, the "plan fragment id for the
parent" definition is moot, and thus will just use the current plan fragment ID
:-/
^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]
