Jackie-Jiang commented on a change in pull request #7568:
URL: https://github.com/apache/pinot/pull/7568#discussion_r755656841
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -519,6 +543,18 @@ private BrokerResponseNative handleSQLRequest(long
requestId, String query, Json
return brokerResponse;
}
+ /** Set EXPLAIN PLAN query to route to only one server. */
+ private void setRoutingToOneServer(Map<ServerInstance, List<String>>
routingTable) {
+ Set<Map.Entry<ServerInstance, List<String>>> servers =
routingTable.entrySet();
+ // only send request to 1 server
+ Map.Entry<ServerInstance, List<String>> server = servers.iterator().next();
+ routingTable.clear();
+ List<String> segments = new ArrayList<>();
+ // only generate the plan for 1 segment
+ segments.add(server.getValue().get(0));
+ routingTable.put(server.getKey(), segments);
Review comment:
No need to create an extra list
```suggestion
routingTable.put(entry.getKey(), entry.getValue());
```
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java
##########
@@ -113,9 +116,34 @@ private IntermediateResultsBlock getCombinedResults() {
}
}
- @Override
- public String getOperatorName() {
- return OPERATOR_NAME;
+ /*
+ * Calculate totalThreadCpuTimeNs based on totalWallClockTimeNs,
multipleThreadCpuTimeNs, and numServerThreads.
+ * System activities time such as OS paging, GC, context switching are not
captured by totalThreadCpuTimeNs.
+ * For example, let's divide query processing into 4 phases:
+ * - phase 1: single thread preparing. Time used: T1
+ * - phase 2: N threads processing segments in parallel, each thread use
time T2
+ * - phase 3: GC/OS paging. Time used: T3
+ * - phase 4: single thread merging intermediate results blocks. Time used:
T4
+ *
+ * Then we have following equations:
+ * - singleThreadCpuTimeNs = T1 + T4
+ * - multipleThreadCpuTimeNs = T2 * N
+ * - totalWallClockTimeNs = T1 + T2 + T3 + T4 = singleThreadCpuTimeNs + T2 +
T3
+ * - totalThreadCpuTimeNsWithoutSystemActivities = T1 + T2 * N + T4 =
singleThreadCpuTimeNs + T2 * N
+ * - systemActivitiesTimeNs = T3 = (totalWallClockTimeNs -
totalThreadCpuTimeNsWithoutSystemActivities) + T2 * (N - 1)
+ *
+ * Thus:
+ * totalThreadCpuTimeNsWithSystemActivities =
totalThreadCpuTimeNsWithoutSystemActivities + systemActivitiesTimeNs
+ * = totalThreadCpuTimeNsWithoutSystemActivities + T3
+ * = totalThreadCpuTimeNsWithoutSystemActivities + (totalWallClockTimeNs -
+ * totalThreadCpuTimeNsWithoutSystemActivities) + T2 * (N - 1)
+ * = totalWallClockTimeNs + T2 * (N - 1)
+ * = totalWallClockTimeNs + (multipleThreadCpuTimeNs / N) * (N - 1)
+ */
+ public static long calTotalThreadCpuTimeNs(long totalWallClockTimeNs, long
multipleThreadCpuTimeNs,
Review comment:
Is this relevant to this PR?
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/common/Operator.java
##########
@@ -39,12 +40,20 @@
*/
T nextBlock();
+ /** @return Name of this operator */
+ String getOperatorName();
+
+ /** @return List of {@link Operator}s that this operator depends upon. */
+ List<Operator> getChildOperators();
+
/**
- * Returns the name of the operator.
- * NOTE: This method is called for tracing purpose. The sub-class should try
to return a constant to avoid the
- * unnecessary overhead.
+ * @return EXPLAIN PLAN name if one is defined; otherwise, null. null
EXPLAIN PLAN name means that this operator
+ * won't be included in EXPLAIN PLAN output.
*/
- String getOperatorName();
+ String getExplainPlanName();
Review comment:
I think we should be able to merge this with `toExplainString()`, and
annotate it with nullable. Don't see the value of keeping 2 separate methods
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -225,7 +225,13 @@ private BrokerResponseNative handleSQLRequest(long
requestId, String query, Json
if (isLiteralOnlyQuery(pinotQuery)) {
LOGGER.debug("Request {} contains only Literal, skipping server query:
{}", requestId, query);
try {
- return processLiteralOnlyQuery(pinotQuery, compilationStartTimeNs,
requestStatistics);
+ BrokerResponseNative responseForLiteralOnly =
+ processLiteralOnlyQuery(pinotQuery, compilationStartTimeNs,
requestStatistics);
Review comment:
For explain query, we should not process the query here. We can add a
BrokerResponseNative constant which represents the queries that can be solved
on the broker side.
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/PredicateEvaluator.java
##########
@@ -28,6 +28,16 @@
* APIs for both dictionary based and raw value based predicate evaluator
*/
+ /**
+ * Set the predicate
+ */
+ void setPredicate(Predicate predicate);
Review comment:
This should not be an interface method
##########
File path: pinot-core/src/main/java/org/apache/pinot/core/common/Operator.java
##########
@@ -39,12 +40,20 @@
*/
T nextBlock();
+ /** @return Name of this operator */
+ String getOperatorName();
+
+ /** @return List of {@link Operator}s that this operator depends upon. */
+ List<Operator> getChildOperators();
+
/**
- * Returns the name of the operator.
- * NOTE: This method is called for tracing purpose. The sub-class should try
to return a constant to avoid the
Review comment:
Can we keep the note?
Also suggest keeping the same convention for the javadoc. I know it can be
personal preference, but we should try to keep consistency within the same file
(especially interface)
##########
File path:
pinot-core/src/main/java/org/apache/pinot/core/operator/BaseOperator.java
##########
@@ -52,4 +54,25 @@ public final T nextBlock() {
// Make it protected because we should always call nextBlock()
protected abstract T getNextBlock();
+
+ // Enforcing sub-class to implement the getOperatorName(), as they can just
return a static final,
+ // as opposed to this super class calling getClass().getSimpleName().
+ public abstract String getOperatorName();
Review comment:
These methods are already included in the interface, no need to redefine
them within the base class
--
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
