Skip to content

Commit 8a79cc8

Browse files
abhioncbrabhioncbr
committed
Renamed PhysicalExplainPlanVisitor class and other minor PR comments fix.
1 parent 0ec2a69 commit 8a79cc8

File tree

6 files changed

+35
-26
lines changed

6 files changed

+35
-26
lines changed

‎pinot-common/src/main/java/org/apache/pinot/sql/parsers/parser/SqlPhysicalExplain.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,11 @@
2323
import org.apache.calcite.sql.SqlNode;
2424
import org.apache.calcite.sql.parser.SqlParserPos;
2525

26-
26+
/**
27+
* Calcite extension for creating a physical plan sql node from a EXPLAIN IMPLEMENTATION query.
28+
*
29+
* <p>Syntax: EXPLAIN IMPLEMENTATION PLAN [ [INCLUDING | EXCLUDING] [ALL] ATTRIBUTES ] FOR SELECT</p>
30+
*/
2731
public class SqlPhysicalExplain extends SqlExplain {
2832
public SqlPhysicalExplain(SqlParserPos pos, SqlNode explicandum, SqlLiteral detailLevel, SqlLiteral depth,
2933
SqlLiteral format, int dynamicParameterCount) {

‎pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@
6161
import org.apache.pinot.common.config.provider.TableCache;
6262
import org.apache.pinot.query.context.PlannerContext;
6363
import org.apache.pinot.query.planner.DispatchableSubPlan;
64-
import org.apache.pinot.query.planner.ExplainPlanPlanVisitor;
64+
import org.apache.pinot.query.planner.PhysicalExplainPlanVisitor;
6565
import org.apache.pinot.query.planner.PlannerUtils;
6666
import org.apache.pinot.query.planner.QueryPlan;
6767
import org.apache.pinot.query.planner.SubPlan;
@@ -170,11 +170,11 @@ public QueryEnvironment(TypeFactory typeFactory, CalciteSchema rootSchema, Worke
170170
public QueryPlannerResult planQuery(String sqlQuery, SqlNodeAndOptions sqlNodeAndOptions, long requestId) {
171171
try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
172172
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
173-
RelRoot relRoot = planQueryLogical(sqlNodeAndOptions.getSqlNode(), plannerContext);
173+
RelRoot relRoot = compileQuery(sqlNodeAndOptions.getSqlNode(), plannerContext);
174174
// TODO: current code only assume one SubPlan per query, but we should support multiple SubPlans per query.
175175
// Each SubPlan should be able to run independently from Broker then set the results into the dependent
176176
// SubPlan for further processing.
177-
DispatchableSubPlan dispatchableSubPlan = planQueryDispatchable(relRoot, plannerContext, requestId);
177+
DispatchableSubPlan dispatchableSubPlan = toDispatchableSubPlan(relRoot, plannerContext, requestId);
178178
return new QueryPlannerResult(dispatchableSubPlan, null, dispatchableSubPlan.getTableNames());
179179
} catch (CalciteContextException e) {
180180
throw new RuntimeException("Error composing query plan for '" + sqlQuery
@@ -200,11 +200,11 @@ public QueryPlannerResult explainQuery(String sqlQuery, SqlNodeAndOptions sqlNod
200200
try (PlannerContext plannerContext = new PlannerContext(_config, _catalogReader, _typeFactory, _hepProgram)) {
201201
SqlExplain explain = (SqlExplain) sqlNodeAndOptions.getSqlNode();
202202
plannerContext.setOptions(sqlNodeAndOptions.getOptions());
203-
RelRoot relRoot = planQueryLogical(explain.getExplicandum(), plannerContext);
203+
RelRoot relRoot = compileQuery(explain.getExplicandum(), plannerContext);
204204
if (explain instanceof SqlPhysicalExplain) {
205205
// get the physical plan for query.
206-
DispatchableSubPlan dispatchableSubPlan = planQueryDispatchable(relRoot, plannerContext, requestId);
207-
return new QueryPlannerResult(null, ExplainPlanPlanVisitor.explain(dispatchableSubPlan),
206+
DispatchableSubPlan dispatchableSubPlan = toDispatchableSubPlan(relRoot, plannerContext, requestId);
207+
return new QueryPlannerResult(null, PhysicalExplainPlanVisitor.explain(dispatchableSubPlan),
208208
dispatchableSubPlan.getTableNames());
209209
} else {
210210
// get the logical plan for query.
@@ -235,7 +235,7 @@ public List<String> getTableNamesForQuery(String sqlQuery) {
235235
if (sqlNode.getKind().equals(SqlKind.EXPLAIN)) {
236236
sqlNode = ((SqlExplain) sqlNode).getExplicandum();
237237
}
238-
RelRoot relRoot = planQueryLogical(sqlNode, plannerContext);
238+
RelRoot relRoot = compileQuery(sqlNode, plannerContext);
239239
Set<String> tableNames = RelToPlanNodeConverter.getTableNamesFromRelRoot(relRoot.rel);
240240
return new ArrayList<>(tableNames);
241241
} catch (Throwable t) {
@@ -277,7 +277,7 @@ public Set<String> getTableNames() {
277277
// --------------------------------------------------------------------------
278278

279279
@VisibleForTesting
280-
protected RelRoot planQueryLogical(SqlNode sqlNode, PlannerContext plannerContext)
280+
protected RelRoot compileQuery(SqlNode sqlNode, PlannerContext plannerContext)
281281
throws Exception {
282282
SqlNode validated = validate(sqlNode, plannerContext);
283283
RelRoot relation = toRelation(validated, plannerContext);
@@ -345,7 +345,7 @@ private SubPlan toSubPlan(RelRoot relRoot) {
345345
QueryPlan queryPlan = pinotLogicalQueryPlanner.planQuery(relRoot);
346346
return pinotLogicalQueryPlanner.makePlan(queryPlan);
347347
}
348-
private DispatchableSubPlan planQueryDispatchable(RelRoot relRoot, PlannerContext plannerContext,
348+
private DispatchableSubPlan toDispatchableSubPlan(RelRoot relRoot, PlannerContext plannerContext,
349349
long requestId) {
350350
SubPlan subPlanRoot = toSubPlan(relRoot);
351351

pinot-query-planner/src/main/java/org/apache/pinot/query/planner/ExplainPlanPlanVisitor.java renamed to pinot-query-planner/src/main/java/org/apache/pinot/query/planner/PhysicalExplainPlanVisitor.java

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,13 @@
4242
/**
4343
* A visitor that converts a {@code QueryPlan} into a human-readable string representation.
4444
*
45-
* <p>It is currently not used programmatically and cannot be accessed by the user. Instead,
46-
* it is intended for use in manual debugging (e.g. setting breakpoints and calling QueryPlan#explain()).
45+
* <p>It is getting used for getting the physical plan of the query.</p>
4746
*/
48-
public class ExplainPlanPlanVisitor implements PlanNodeVisitor<StringBuilder, ExplainPlanPlanVisitor.Context> {
47+
public class PhysicalExplainPlanVisitor implements PlanNodeVisitor<StringBuilder, PhysicalExplainPlanVisitor.Context> {
4948

5049
private final DispatchableSubPlan _dispatchableSubPlan;
5150

52-
public ExplainPlanPlanVisitor(DispatchableSubPlan dispatchableSubPlan) {
51+
public PhysicalExplainPlanVisitor(DispatchableSubPlan dispatchableSubPlan) {
5352
_dispatchableSubPlan = dispatchableSubPlan;
5453
}
5554

@@ -87,7 +86,7 @@ public static String explain(DispatchableSubPlan dispatchableSubPlan) {
8786
*/
8887
public static String explainFrom(DispatchableSubPlan dispatchableSubPlan, PlanNode node,
8988
QueryServerInstance rootServer) {
90-
final ExplainPlanPlanVisitor visitor = new ExplainPlanPlanVisitor(dispatchableSubPlan);
89+
final PhysicalExplainPlanVisitor visitor = new PhysicalExplainPlanVisitor(dispatchableSubPlan);
9190
return node
9291
.visit(visitor, new Context(rootServer, 0, "", "", new StringBuilder()))
9392
.toString();
@@ -199,7 +198,7 @@ private StringBuilder appendMailboxSend(MailboxSendNode node, Context context) {
199198
.getServerInstanceToWorkerIdMap();
200199
context._builder.append("->");
201200
String receivers = servers.entrySet().stream()
202-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
201+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
203202
.map(s -> "[" + receiverStageId + "]@" + s)
204203
.collect(Collectors.joining(",", "{", "}"));
205204
return context._builder.append(receivers);

‎pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/PlanNodeVisitor.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*/
1919
package org.apache.pinot.query.planner.plannode;
2020

21-
import org.apache.pinot.query.planner.ExplainPlanPlanVisitor;
21+
import org.apache.pinot.query.planner.PhysicalExplainPlanVisitor;
2222

2323

2424
/**
@@ -27,7 +27,7 @@
2727
* enforced traversal order, and should be implemented by subclasses.
2828
*
2929
* <p>It is recommended that implementors use private constructors and static methods to access main
30-
* functionality (see {@link ExplainPlanPlanVisitor#explain(org.apache.pinot.query.planner.DispatchableSubPlan)}
30+
* functionality (see {@link PhysicalExplainPlanVisitor#explain(org.apache.pinot.query.planner.DispatchableSubPlan)}
3131
* as an example of a usage of this pattern.
3232
*
3333
* @param <T> the return type for all visitsPlanNodeVisitor

‎pinot-query-planner/src/test/java/org/apache/pinot/query/QueryCompilationTest.java‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
import org.apache.calcite.rel.RelDistribution;
3232
import org.apache.pinot.query.planner.DispatchablePlanFragment;
3333
import org.apache.pinot.query.planner.DispatchableSubPlan;
34-
import org.apache.pinot.query.planner.ExplainPlanPlanVisitor;
34+
import org.apache.pinot.query.planner.PhysicalExplainPlanVisitor;
3535
import org.apache.pinot.query.planner.PlannerUtils;
3636
import org.apache.pinot.query.planner.plannode.AbstractPlanNode;
3737
import org.apache.pinot.query.planner.plannode.AggregateNode;
@@ -134,18 +134,21 @@ public void testQueryAndAssertStageContentForJoin()
134134
if (tableName != null) {
135135
// table scan stages; for tableA it should have 2 hosts, for tableB it should have only 1
136136
Assert.assertEquals(dispatchablePlanFragment.getServerInstanceToWorkerIdMap().entrySet().stream()
137-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry).collect(Collectors.toSet()),
137+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
138+
.collect(Collectors.toSet()),
138139
tableName.equals("a") ? ImmutableList.of("localhost@{1,1}|[1]", "localhost@{2,2}|[0]")
139140
: ImmutableList.of("localhost@{1,1}|[0]"));
140141
} else if (!PlannerUtils.isRootPlanFragment(stageId)) {
141142
// join stage should have both servers used.
142143
Assert.assertEquals(dispatchablePlanFragment.getServerInstanceToWorkerIdMap().entrySet().stream()
143-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry).collect(Collectors.toSet()),
144+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
145+
.collect(Collectors.toSet()),
144146
ImmutableSet.of("localhost@{1,1}|[1]", "localhost@{2,2}|[0]"));
145147
} else {
146148
// reduce stage should have the reducer instance.
147149
Assert.assertEquals(dispatchablePlanFragment.getServerInstanceToWorkerIdMap().entrySet().stream()
148-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry).collect(Collectors.toSet()),
150+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
151+
.collect(Collectors.toSet()),
149152
ImmutableSet.of("localhost@{3,3}|[0]"));
150153
}
151154
}
@@ -254,12 +257,14 @@ public void testQueryWithHint()
254257
if (tableName != null) {
255258
// table scan stages; for tableB it should have only 1
256259
Assert.assertEquals(dispatchablePlanFragment.getServerInstanceToWorkerIdMap().entrySet().stream()
257-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry).collect(Collectors.toSet()),
260+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
261+
.collect(Collectors.toSet()),
258262
ImmutableList.of("localhost@{1,1}|[0]"));
259263
} else if (!PlannerUtils.isRootPlanFragment(stageId)) {
260264
// join stage should have both servers used.
261265
Assert.assertEquals(dispatchablePlanFragment.getServerInstanceToWorkerIdMap().entrySet().stream()
262-
.map(ExplainPlanPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry).collect(Collectors.toSet()),
266+
.map(PhysicalExplainPlanVisitor::stringifyQueryServerInstanceToWorkerIdsEntry)
267+
.collect(Collectors.toSet()),
263268
ImmutableList.of("localhost@{1,1}|[1]", "localhost@{2,2}|[0]"));
264269
}
265270
}

‎pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@
4747
import org.apache.pinot.query.mailbox.MailboxService;
4848
import org.apache.pinot.query.planner.DispatchablePlanFragment;
4949
import org.apache.pinot.query.planner.DispatchableSubPlan;
50-
import org.apache.pinot.query.planner.ExplainPlanPlanVisitor;
50+
import org.apache.pinot.query.planner.PhysicalExplainPlanVisitor;
5151
import org.apache.pinot.query.planner.plannode.MailboxReceiveNode;
5252
import org.apache.pinot.query.routing.QueryServerInstance;
5353
import org.apache.pinot.query.routing.VirtualServerAddress;
@@ -100,7 +100,8 @@ public ResultTable submitAndReduce(long requestId, DispatchableSubPlan dispatcha
100100
traceEnabled);
101101
} catch (Exception e) {
102102
cancel(requestId, dispatchableSubPlan);
103-
throw new RuntimeException("Error executing query: " + ExplainPlanPlanVisitor.explain(dispatchableSubPlan), e);
103+
throw new RuntimeException("Error executing query: "
104+
+ PhysicalExplainPlanVisitor.explain(dispatchableSubPlan), e);
104105
}
105106
}
106107

0 commit comments

Comments
 (0)