walterddr commented on code in PR #9957:
URL: https://github.com/apache/pinot/pull/9957#discussion_r1054718358
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -209,27 +209,39 @@ private void applyQueryOptions(QueryContext queryContext)
{
// Set group-by query options
if (QueryContextUtils.isAggregationQuery(queryContext) &&
queryContext.getGroupByExpressions() != null) {
-
// Set maxInitialResultHolderCapacity
-
queryContext.setMaxInitialResultHolderCapacity(_maxInitialResultHolderCapacity);
-
+ Integer initResultCap =
QueryOptionsUtils.getMaxInitResultCap(queryOptions);
+ if (initResultCap != null) {
+ queryContext.setMaxInitialResultHolderCapacity(initResultCap);
+ } else {
+
queryContext.setMaxInitialResultHolderCapacity(_maxInitialResultHolderCapacity);
+ }
Review Comment:
nit: unboxing can be coupled with default.
```suggestion
Integer initResultCap =
QueryOptionsUtils.getMaxInitResultCap(queryOptions);
int maxInitialResultHolderCapacity = initResultCap == null ?
_maxInitialResultHolderCapacity : initResultCap;
queryContext.setMaxInitialResultHolderCapacity(maxInitialResultHolderCapacity);
```
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/MultiStageEngineIntegrationTest.java:
##########
@@ -99,6 +99,14 @@ public void testGeneratedQueries()
super.testGeneratedQueries(false, true);
}
+ @Test
+ public void testQueryOptions()
+ throws Exception {
+ String pinotQuery = "SET multistageLeafLimit = 1; SELECT * FROM mytable;";
+ String h2Query = "SELECT * FROM mytable";
Review Comment:
curious how did this test passed? don't you need to do `SELECT * FROM
mytable LIMIT 1` for H2 query?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -165,7 +165,8 @@ private BrokerResponseNative handleRequest(long requestId,
String query,
ResultTable queryResults;
try {
- queryResults = _queryDispatcher.submitAndReduce(requestId, queryPlan,
_mailboxService, queryTimeoutMs);
+ queryResults = _queryDispatcher.submitAndReduce(requestId, queryPlan,
_mailboxService, queryTimeoutMs,
+ sqlNodeAndOptions.getOptions());
Review Comment:
nit: please use pinot intellij checkstyle so that it doesn't automatically
reformat code like these changes
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/QueryDispatcher.java:
##########
@@ -89,7 +89,9 @@ public int submit(long requestId, QueryPlan queryPlan, long
timeoutMs)
Worker.QueryResponse response =
client.submit(Worker.QueryRequest.newBuilder().setStagePlan(
QueryPlanSerDeUtils.serialize(constructDistributedStagePlan(queryPlan, stageId,
serverInstance)))
.putMetadata(QueryConfig.KEY_OF_BROKER_REQUEST_ID,
String.valueOf(requestId))
- .putMetadata(QueryConfig.KEY_OF_BROKER_REQUEST_TIMEOUT_MS,
String.valueOf(timeoutMs)).build());
+ .putMetadata(QueryConfig.KEY_OF_BROKER_REQUEST_TIMEOUT_MS,
String.valueOf(timeoutMs))
+ .putAllMetadata(queryOptions).build());
Review Comment:
should we only filter out queryOptions and attach only those that is listed
in `QueryConfig.KEY_OF_*`?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -291,6 +291,21 @@ public static class QueryOptionKey {
public static final String ORDER_BY_ALGORITHM = "orderByAlgorithm";
+ // Number of rows limit to select in leaf stage.
+ // It has to be greater than 0.
+ // If it is invalid number format, or the number is smaller than or
equal to zero, we use
+ // DEFAULT_LEAF_NODE_LIMIT.
+ public static final String MS_LEAF_LIMIT = "multiStageLeafLimit";
+
+ // Override numGroupLimit used by v1 engine.
+ public static final String NUM_GROUP_LIMIT = "numGroupLimit";
+
+ // Override maxInitialResultCap used by v1 engine.
+ public static final String MAX_INITIAL_RESULT_HOLDER_CAPACITY =
"maxInitialResultCap";
+
+ // Override groupByTrimThreshold used by v1 engine.
+ public static final String GROUP_BY_TRIM_THRESHOLD =
"groupByTrimThreshold";
Review Comment:
sounds good. thanks for sharing your opinions
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
