Re: [PR] Short circuit SubPlanFragmenter because we don't support multiple sub-plans yet [pinot]

2024-06-06 Thread via GitHub 


Jackie-Jiang commented on code in PR #13306:
URL: https://github.com/apache/pinot/pull/13306#discussion_r1629889683


##
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/PinotLogicalQueryPlanner.java:
##
@@ -51,70 +48,72 @@ private PinotLogicalQueryPlanner() {
* Converts a Calcite {@link RelRoot} into a Pinot {@link SubPlan}.
*/
   public static SubPlan makePlan(RelRoot relRoot) {
-PlanNode rootNode = relNodeToStageNode(relRoot.rel);
-QueryPlanMetadata metadata =
-new 
QueryPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields);
+PlanNode rootNode = relNodeToPlanNode(relRoot.rel);
+PlanFragment rootFragment = planNodeToPlanFragment(rootNode);
+return new SubPlan(rootFragment,
+new 
SubPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields), List.of());
 
+// TODO: Currently we don't support multiple sub-plans. Revisit the 
following logic when we add the support.
 // Fragment the stage tree into multiple SubPlans.
-SubPlanFragmenter.Context subPlanContext = new SubPlanFragmenter.Context();
-subPlanContext._subPlanIdToRootNodeMap.put(0, rootNode);
-subPlanContext._subPlanIdToMetadataMap.put(0, new 
SubPlanMetadata(metadata.getTableNames(), metadata.getFields()));
-rootNode.visit(SubPlanFragmenter.INSTANCE, subPlanContext);
-
-Map subPlanMap = new HashMap<>();
-for (Map.Entry subPlanEntry : 
subPlanContext._subPlanIdToRootNodeMap.entrySet()) {
-  int subPlanId = subPlanEntry.getKey();
-  PlanNode subPlanRoot = subPlanEntry.getValue();
-
-  // Fragment the SubPlan into multiple PlanFragments.
-  PlanFragmenter fragmenter = new PlanFragmenter();
-  PlanFragmenter.Context fragmenterContext = fragmenter.createContext();
-  subPlanRoot = subPlanRoot.visit(fragmenter, fragmenterContext);
-  Int2ObjectOpenHashMap planFragmentMap = 
fragmenter.getPlanFragmentMap();
-  Int2ObjectOpenHashMap childPlanFragmentIdsMap = 
fragmenter.getChildPlanFragmentIdsMap();
-
-  // Sub plan root needs to send final results back to the Broker
-  // TODO: Should be SINGLETON (currently SINGLETON has to be local, so 
use BROADCAST_DISTRIBUTED instead)
-  MailboxSendNode subPlanRootSenderNode =
-  new MailboxSendNode(subPlanRoot.getPlanFragmentId(), 
subPlanRoot.getDataSchema(), 0,
-  RelDistribution.Type.BROADCAST_DISTRIBUTED, 
PinotRelExchangeType.getDefaultExchangeType(), null, null,
-  false, false);
-  subPlanRootSenderNode.addInput(subPlanRoot);
-  PlanFragment planFragment1 = new PlanFragment(1, subPlanRootSenderNode, 
new ArrayList<>());
-  planFragmentMap.put(1, planFragment1);
-  for (Int2ObjectMap.Entry entry : 
childPlanFragmentIdsMap.int2ObjectEntrySet()) {
-PlanFragment planFragment = planFragmentMap.get(entry.getIntKey());
-List childPlanFragments = planFragment.getChildren();
-IntListIterator childPlanFragmentIdIterator = 
entry.getValue().iterator();
-while (childPlanFragmentIdIterator.hasNext()) {
-  
childPlanFragments.add(planFragmentMap.get(childPlanFragmentIdIterator.nextInt()));
-}
-  }
-  MailboxReceiveNode rootReceiveNode =
-  new MailboxReceiveNode(0, subPlanRoot.getDataSchema(), 
subPlanRoot.getPlanFragmentId(),
-  RelDistribution.Type.BROADCAST_DISTRIBUTED, 
PinotRelExchangeType.getDefaultExchangeType(), null, null,
-  false, false, subPlanRootSenderNode);
-  PlanFragment rootPlanFragment = new PlanFragment(0, rootReceiveNode, 
Collections.singletonList(planFragment1));
-  SubPlan subPlan = new SubPlan(rootPlanFragment, 
subPlanContext._subPlanIdToMetadataMap.get(0), new ArrayList<>());
-  subPlanMap.put(subPlanId, subPlan);
-}
-for (Map.Entry> subPlanToChildrenEntry : 
subPlanContext._subPlanIdToChildrenMap.entrySet()) {
-  int subPlanId = subPlanToChildrenEntry.getKey();
-  List subPlanChildren = subPlanToChildrenEntry.getValue();
-  for (int subPlanChild : subPlanChildren) {
-
subPlanMap.get(subPlanId).getChildren().add(subPlanMap.get(subPlanChild));
-  }
-}
-return subPlanMap.get(0);
+//SubPlanFragmenter.Context subPlanContext = new 
SubPlanFragmenter.Context();
+//subPlanContext._subPlanIdToRootNodeMap.put(0, rootNode);
+//subPlanContext._subPlanIdToMetadataMap.put(0,
+//new 
SubPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields));
+//rootNode.visit(SubPlanFragmenter.INSTANCE, subPlanContext);
+//
+//Map subPlanMap = new HashMap<>();
+//for (Map.Entry subPlanEntry : 
subPlanContext._subPlanIdToRootNodeMap.entrySet()) {
+//  SubPlan subPlan =
+//  new SubPlan(planNodeToPlanFragment(subPlanEntry.getValue()), 
subPlanContext._subPlanIdToMetadataMap.get(0),
+//  new ArrayList<>());
+//  subPlanMa

Re: [PR] Short circuit SubPlanFragmenter because we don't support multiple sub-plans yet [pinot]

2024-06-04 Thread via GitHub 


gortiz commented on code in PR #13306:
URL: https://github.com/apache/pinot/pull/13306#discussion_r1627085061


##
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/PinotLogicalQueryPlanner.java:
##
@@ -51,70 +48,72 @@ private PinotLogicalQueryPlanner() {
* Converts a Calcite {@link RelRoot} into a Pinot {@link SubPlan}.
*/
   public static SubPlan makePlan(RelRoot relRoot) {
-PlanNode rootNode = relNodeToStageNode(relRoot.rel);
-QueryPlanMetadata metadata =
-new 
QueryPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields);
+PlanNode rootNode = relNodeToPlanNode(relRoot.rel);
+PlanFragment rootFragment = planNodeToPlanFragment(rootNode);
+return new SubPlan(rootFragment,
+new 
SubPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields), List.of());
 
+// TODO: Currently we don't support multiple sub-plans. Revisit the 
following logic when we add the support.
 // Fragment the stage tree into multiple SubPlans.
-SubPlanFragmenter.Context subPlanContext = new SubPlanFragmenter.Context();
-subPlanContext._subPlanIdToRootNodeMap.put(0, rootNode);
-subPlanContext._subPlanIdToMetadataMap.put(0, new 
SubPlanMetadata(metadata.getTableNames(), metadata.getFields()));
-rootNode.visit(SubPlanFragmenter.INSTANCE, subPlanContext);
-
-Map subPlanMap = new HashMap<>();
-for (Map.Entry subPlanEntry : 
subPlanContext._subPlanIdToRootNodeMap.entrySet()) {
-  int subPlanId = subPlanEntry.getKey();
-  PlanNode subPlanRoot = subPlanEntry.getValue();
-
-  // Fragment the SubPlan into multiple PlanFragments.
-  PlanFragmenter fragmenter = new PlanFragmenter();
-  PlanFragmenter.Context fragmenterContext = fragmenter.createContext();
-  subPlanRoot = subPlanRoot.visit(fragmenter, fragmenterContext);
-  Int2ObjectOpenHashMap planFragmentMap = 
fragmenter.getPlanFragmentMap();
-  Int2ObjectOpenHashMap childPlanFragmentIdsMap = 
fragmenter.getChildPlanFragmentIdsMap();
-
-  // Sub plan root needs to send final results back to the Broker
-  // TODO: Should be SINGLETON (currently SINGLETON has to be local, so 
use BROADCAST_DISTRIBUTED instead)
-  MailboxSendNode subPlanRootSenderNode =
-  new MailboxSendNode(subPlanRoot.getPlanFragmentId(), 
subPlanRoot.getDataSchema(), 0,
-  RelDistribution.Type.BROADCAST_DISTRIBUTED, 
PinotRelExchangeType.getDefaultExchangeType(), null, null,
-  false, false);
-  subPlanRootSenderNode.addInput(subPlanRoot);
-  PlanFragment planFragment1 = new PlanFragment(1, subPlanRootSenderNode, 
new ArrayList<>());
-  planFragmentMap.put(1, planFragment1);
-  for (Int2ObjectMap.Entry entry : 
childPlanFragmentIdsMap.int2ObjectEntrySet()) {
-PlanFragment planFragment = planFragmentMap.get(entry.getIntKey());
-List childPlanFragments = planFragment.getChildren();
-IntListIterator childPlanFragmentIdIterator = 
entry.getValue().iterator();
-while (childPlanFragmentIdIterator.hasNext()) {
-  
childPlanFragments.add(planFragmentMap.get(childPlanFragmentIdIterator.nextInt()));
-}
-  }
-  MailboxReceiveNode rootReceiveNode =
-  new MailboxReceiveNode(0, subPlanRoot.getDataSchema(), 
subPlanRoot.getPlanFragmentId(),
-  RelDistribution.Type.BROADCAST_DISTRIBUTED, 
PinotRelExchangeType.getDefaultExchangeType(), null, null,
-  false, false, subPlanRootSenderNode);
-  PlanFragment rootPlanFragment = new PlanFragment(0, rootReceiveNode, 
Collections.singletonList(planFragment1));
-  SubPlan subPlan = new SubPlan(rootPlanFragment, 
subPlanContext._subPlanIdToMetadataMap.get(0), new ArrayList<>());
-  subPlanMap.put(subPlanId, subPlan);
-}
-for (Map.Entry> subPlanToChildrenEntry : 
subPlanContext._subPlanIdToChildrenMap.entrySet()) {
-  int subPlanId = subPlanToChildrenEntry.getKey();
-  List subPlanChildren = subPlanToChildrenEntry.getValue();
-  for (int subPlanChild : subPlanChildren) {
-
subPlanMap.get(subPlanId).getChildren().add(subPlanMap.get(subPlanChild));
-  }
-}
-return subPlanMap.get(0);
+//SubPlanFragmenter.Context subPlanContext = new 
SubPlanFragmenter.Context();
+//subPlanContext._subPlanIdToRootNodeMap.put(0, rootNode);
+//subPlanContext._subPlanIdToMetadataMap.put(0,
+//new 
SubPlanMetadata(RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel), 
relRoot.fields));
+//rootNode.visit(SubPlanFragmenter.INSTANCE, subPlanContext);
+//
+//Map subPlanMap = new HashMap<>();
+//for (Map.Entry subPlanEntry : 
subPlanContext._subPlanIdToRootNodeMap.entrySet()) {
+//  SubPlan subPlan =
+//  new SubPlan(planNodeToPlanFragment(subPlanEntry.getValue()), 
subPlanContext._subPlanIdToMetadataMap.get(0),
+//  new ArrayList<>());
+//  subPlanMap.put(

Re: [PR] Short circuit SubPlanFragmenter because we don't support multiple sub-plans yet [pinot]

2024-06-04 Thread via GitHub 


Jackie-Jiang merged PR #13306:
URL: https://github.com/apache/pinot/pull/13306


-- 
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: commits-unsubscr...@pinot.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Re: [PR] Short circuit SubPlanFragmenter because we don't support multiple sub-plans yet [pinot]

2024-06-03 Thread via GitHub 


codecov-commenter commented on PR #13306:
URL: https://github.com/apache/pinot/pull/13306#issuecomment-2146421045

   ## 
[Codecov](https://app.codecov.io/gh/apache/pinot/pull/13306?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   Attention: Patch coverage is `0%` with `26 lines` in your changes missing 
coverage. Please review.
   > Project coverage is 27.77%. Comparing base 
[(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 to head 
[(`0336865`)](https://app.codecov.io/gh/apache/pinot/commit/0336865a0637b98fa1eb03b53f7ef05c74551703?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 546 commits behind head on master.
   
   | 
[Files](https://app.codecov.io/gh/apache/pinot/pull/13306?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[...uery/planner/logical/PinotLogicalQueryPlanner.java](https://app.codecov.io/gh/apache/pinot/pull/13306?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fplanner%2Flogical%2FPinotLogicalQueryPlanner.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9sb2dpY2FsL1Bpbm90TG9naWNhbFF1ZXJ5UGxhbm5lci5qYXZh)
 | 0.00% | [26 Missing :warning: 
](https://app.codecov.io/gh/apache/pinot/pull/13306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 |
   
   Additional details and impacted files
   
   
   ```diff
   @@  Coverage Diff  @@
   ## master   #13306   +/-   ##
   =
   - Coverage 61.75%   27.77%   -33.98% 
   + Complexity  207  198-9 
   =
 Files  2436 2535   +99 
 Lines133233   139293 +6060 
 Branches  2063621538  +902 
   =
   - Hits  8227438690-43584 
   - Misses4491197652+52741 
   + Partials   6048 2951 -3097 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
   | 
[integration](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
   | 
[integration1](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `<0.01% <0.00%> (-0.01%)` | :arrow_down: |
   | 
[integration2](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <0.00%> (ø)` | |
   | 
[java-11](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `<0.01% <0.00%> (-61.71%)` | :arrow_down: |
   | 
[java-21](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `27.77% <0.00%> (-33.85%)` | :arrow_down: |
   | 
[skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `27.77% <0.00%> (-33.98%)` | :arrow_down: |
   | 
[skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `27.77% <0.00%> (+0.04%)` | :arrow_up: |
   | 
[temurin](https://app.codecov.io/gh/apache/pinot/pull/13306/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `27.77% <0.00%> (-33.98%)` | :arrow_down: