Fix explain plan ALL_SEGMENTS_PRUNED_ON_SERVER node #9572
Conversation
…RVER to 3 and 2 respectively
|
@somandal You probably also have to fix it here - https://github.com/apache/pinot/blob/master/pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java#L2483 ? |
good catch, thanks! fixed it |
Codecov Report
@@ Coverage Diff @@
## master #9572 +/- ##
============================================
+ Coverage 60.34% 69.97% +9.62%
- Complexity 5072 5217 +145
============================================
Files 1919 1931 +12
Lines 102764 103119 +355
Branches 15616 15655 +39
============================================
+ Hits 62017 72154 +10137
+ Misses 36089 25889 -10200
- Partials 4658 5076 +418
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
As discussed offline, we should probably have a dummy passthrough node in the "all segments pruned" case so that we don't show a disconnected tree. In the above example, we'll not have any node with operatorId=2 and parentId=1 unless we have the dummy node. |
good catch. i've fixed this now. also updated the code to handle selecting the correct COMBINE node depending on if we have any which aren't PASSTHROUGH or not. (e.g. if a better node like COMBINE_AGGREGATE is available then that should be used instead of COMBINE_PASSTHROUGH in the final plan) |
siddharthteotia
changed the title
| @@ -1614,7 +1614,7 @@ public void testSelectColumnsVariationsOfAndOperatorsVerbose() { | |||
| 7, 6}); | |||
| result4.add(new Object[]{"PLAN_START(numSegmentsForThisPlan:2)", ExplainPlanRows.PLAN_START_IDS, | |||
| ExplainPlanRows.PLAN_START_IDS}); | |||
| result4.add(new Object[]{"ALL_SEGMENTS_PRUNED_ON_SERVER", 2, 1}); | |||
| result4.add(new Object[]{"ALL_SEGMENTS_PRUNED_ON_SERVER", 3, 2}); | |||
There was a problem hiding this comment.
Can we add a test for the scenario where everything got pruned across all servers and thus it should be 2, 1 because COMBINE will be absent.
There was a problem hiding this comment.
@siddharthteotia this file already has tests for this scenario. e.g.
// All segments are pruned
String query10 = "EXPLAIN PLAN FOR SELECT count(*) FROM testTable WHERE invertedIndexCol3 = 'roadrunner' AND "
+ "noIndexCol1 = 100 LIMIT 100";
List<Object[]> result10 = new ArrayList<>();
result10.add(new Object[]{"BROKER_REDUCE(limit:100)", 1, 0});
result10.add(new Object[]{"PLAN_START(numSegmentsForThisPlan:4)", ExplainPlanRows.PLAN_START_IDS,
ExplainPlanRows.PLAN_START_IDS});
result10.add(new Object[]{"ALL_SEGMENTS_PRUNED_ON_SERVER", 2, 1});
check(query10, new ResultTable(DATA_SCHEMA, result10));
This doesn't show up in the diff since the original values for the operator ID and parent ID are the same
somandal
requested review from
siddharthteotia
and removed request for
vvivekiyer
Today EXPLAIN PLAN sets the operator ID and parent ID of ALL_SEGMENTS_PRUNED_ON_SERVER as 2 and 1 respectively. This PR modifies ALL_SEGMENTS_PRUNED_ON_SERVER to use the operator ID and parent ID as 3 and 2 respectively instead. This is to bring consistency with the other explain plan nodes. The root is BROKER_REDUCE which has the ID 1 and parent ID 0, followed by the COMBINE node which has ID 2 and parent ID 1.
For scenarios where all segments were pruned across all servers, added special handling to keep the ALL_SEGMENTS_PRUNED_ON_SERVER operator ID and parent ID as 2 and 1 respectively. This is because for such scenarios no COMBINE node is present.