Refactor PlanFragmenter to make the logic more clear #11912
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11912 +/- ##
============================================
- Coverage 61.50% 61.45% -0.06%
Complexity 1147 1147
============================================
Files 2378 2378
Lines 128844 128845 +1
Branches 19925 19924 -1
============================================
- Hits 79242 79178 -64
- Misses 43858 43945 +87
+ Partials 5744 5722 -22
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // Split the ExchangeNode to a MailboxReceiveNode and a MailboxSendNode, where MailboxReceiveNode is the leave node | ||
| // of the current PlanFragment, and MailboxSendNode is the root node of the next PlanFragment. | ||
| int receiverPlanFragmentId = context._currentPlanFragmentId; | ||
| int senderPlanFragmentId = context._nextPlanFragmentId.getAndIncrement(); |
There was a problem hiding this comment.
nit: visitor pattern is single threaded so i dont think we need to have atomic integer here.
this "getAndIncrement" is still confusing --> if we are handling one context per visit upon hitting an exchange then this is the only variable that's cross all fragments: IIUC, the only reason this works is b/c the nextPlanFragmentId is passed by reference so it is not really contain within the context?
There was a problem hiding this comment.
Modified to MutableInt to avoid confusion.
IMO this is the correct way to handle this recursion. Basically we share _nextPlanFragmentId across all PlanFragments to ensure uniqueness of PlanFragment ID. Added a comment to explain this
There was a problem hiding this comment.
yes i think that's correct. i was wondering if we can make it even more straightforward.
- IMO the "context" is pre-fragment
- but the global next ID is per planFragmenter --> we can create a global next ID tracker inside the planFragmenter instead (ditch the static INSTANCE approach)
WDYT?
There was a problem hiding this comment.
Good point, refactored again
| final Map<Integer, PlanFragment> _planFragmentIdToRootNodeMap; | ||
| final Map<Integer, List<Integer>> _planFragmentIdToChildrenMap; | ||
|
|
||
| public Context() { |
There was a problem hiding this comment.
why do we need this public constructor? no one is using it right?
There was a problem hiding this comment.
It is used in PinotLogicalQueryPlanner to create the initial Context
3 participants
Creates a new
Contextfor eachPlanFragmenterto avoid the convoluted logic.