gortiz commented on code in PR #16084:
URL: https://github.com/apache/pinot/pull/16084#discussion_r2144347755
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -657,6 +663,10 @@ public static class QueryOptionKey {
// When lite mode is enabled, if this flag is set, we will run all the
non-leaf stage operators within the
// broker itself. That way, the MSE queries will model the scatter
gather pattern used by the V1 Engine.
public static final String RUN_IN_BROKER = "runInBroker";
+
+ // For MSE queries, when this option is set to true, only use servers
for leaf stages as the workers for the
+ // intermediate stages. This is useful to control the fanout of the
query and reduce data shuffling.
+ public static final String USE_LEAF_SERVER_FOR_INTERMEDIATE_STAGE =
"useLeafServerForIntermediateStage";
Review Comment:
nit: can we use the three `///` to be recognized as Javadoc? I know some
other constants are not documented, but new ones should
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/DispatchablePlanContext.java:
##########
@@ -71,17 +88,32 @@ public long getRequestId() {
return _requestId;
}
+ public PlannerContext getPlannerContext() {
+ return _plannerContext;
+ }
+
+ public PairList<Integer, String> getResultFields() {
+ return _resultFields;
+ }
+
// Returns all the table names.
public Set<String> getTableNames() {
return _tableNames;
}
- public PairList<Integer, String> getResultFields() {
- return _resultFields;
+ /// Tracks non-lookup tables queried in leaf stages, which are used to
determine the servers to use for intermediate
+ /// stages. Returns `null` if leaf servers are directly used for
intermediate stages.
+ @Nullable
+ public Set<String> getNonLookupTables() {
+ return _nonLookupTables;
}
- public PlannerContext getPlannerContext() {
- return _plannerContext;
+ /// Tracks servers that are used for leaf stages, which are used to
determine the servers to use for intermediate
+ /// stages. Returns `null` if leaf servers are NOT directly used for
intermediate stages, and we need to calculate
+ /// the servers to use based on the non-lookup tables.
+ @Nullable
+ public Set<QueryServerInstance> getLeafServerInstances() {
+ return _leafServerInstances;
Review Comment:
Same as before, I think getLeafServerInstances may be used for other things
and a developer may blindly assume this method will return not null even if
leaf servers are not directly used for intermediate stages.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/DispatchablePlanContext.java:
##########
@@ -71,17 +88,32 @@ public long getRequestId() {
return _requestId;
}
+ public PlannerContext getPlannerContext() {
+ return _plannerContext;
+ }
+
+ public PairList<Integer, String> getResultFields() {
+ return _resultFields;
+ }
+
// Returns all the table names.
public Set<String> getTableNames() {
return _tableNames;
}
- public PairList<Integer, String> getResultFields() {
- return _resultFields;
+ /// Tracks non-lookup tables queried in leaf stages, which are used to
determine the servers to use for intermediate
+ /// stages. Returns `null` if leaf servers are directly used for
intermediate stages.
+ @Nullable
+ public Set<String> getNonLookupTables() {
+ return _nonLookupTables;
Review Comment:
I think it is unexpected that a getter method like this one returns null
under some distribution conditions. Shouln't that be another getter method to
be called?
The current semantic seems error prone
--
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
