ankitsultana commented on code in PR #18725:
URL: https://github.com/apache/pinot/pull/18725#discussion_r3393836381
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/v2/opt/rules/LiteModeSortInsertRule.java:
##########
@@ -81,6 +81,7 @@ public PRelNode onMatch(PRelOptRuleCall call) {
liteModeLimit);
return sort;
}
+ _context.setLiteModeImplicitSortApplied(liteModeLimit);
Review Comment:
nit: probably overkill, but it might be easy to miss calling
`setLiteModeImplicitSortApplied` as this code evolves. one way to mitigate that
could be add an `onMatchInternal(call) => Pair<PRelNode, Integer>` function.
but no one reads code anymore so should be okay
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/context/PhysicalPlannerContext.java:
##########
@@ -74,6 +74,8 @@ public Integer get() {
private final boolean _liteModeJoinsEnabled;
@Nullable
private final MultiClusterRoutingContext _multiClusterRoutingContext;
+ private boolean _liteModeImplicitSortApplied = false;
Review Comment:
nit: this boolean is not required and `isLiteModeImplicitSortApplied` can be
derived from `_liteModeEffectiveSortLimit`. can simplify code at other places
too
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -951,6 +951,7 @@ public static class QueryOptionKey {
// Server stage limit for lite mode queries.
public static final String LITE_MODE_LEAF_STAGE_LIMIT =
"liteModeLeafStageLimit";
public static final String LITE_MODE_LEAF_STAGE_FANOUT_ADJUSTED_LIMIT
= "liteModeLeafStageFanOutAdjustedLimit";
+ public static final String LITE_MODE_IMPLICIT_LEAF_STAGE_LIMIT =
"liteModeImplicitLeafStageLimit";
Review Comment:
nit: can you add a small comment here to explain this?
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/v2/opt/rules/LiteModeSortInsertRule.java:
##########
@@ -89,6 +90,9 @@ public PRelNode onMatch(PRelOptRuleCall call) {
Preconditions.checkState(aggregate.getLimit() <= liteModeLimit,
"Group trim limit={} exceeds server stage limit={}",
aggregate.getLimit(), liteModeLimit);
int limit = aggregate.getLimit() > 0 ? aggregate.getLimit() :
liteModeLimit;
Review Comment:
nit: might be cleaner to convert this ternary to if/else
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -96,6 +97,13 @@ protected InstanceResponseBlock
buildInstanceResponseBlock(BaseResultsBlock base
String.valueOf(_threadMemAllocatedBytes));
instanceResponseBlock.addMetadata(MetadataKey.SYSTEM_ACTIVITIES_CPU_TIME_NS.getName(),
String.valueOf(_systemActivitiesCpuTimeNs));
+ Integer implicitLimit =
QueryOptionsUtils.getLiteModeImplicitLeafStageLimit(
+ _queryContext.getQueryOptions());
+ // false-positive when table has exactly implicitLimit rows
+ if (implicitLimit != null && baseResultsBlock.getNumRows() >=
implicitLimit) {
+ instanceResponseBlock.addMetadata(
+ MetadataKey.LITE_MODE_LEAF_STAGE_LIMIT_REACHED.getName(), "true");
Review Comment:
would it better to log `baseResultsBlock.getNumRows()` instead of just a
true boolean?
also, afair, baseResultsBlock should ideally have at most implicitLimit rows
because both selection and group by operators will not allow returning more
than that. in that case i guess we can just keep this true
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -277,7 +277,15 @@ private QueryEnvironment.QueryPlannerResult
getQueryPlannerResult(PlannerContext
extraFields.put(RuleTimingPlannerListener.RULE_TIMINGS,
plannerContext.getPlannerOutput().get(RuleTimingPlannerListener.RULE_TIMINGS));
}
- return new QueryPlannerResult(dispatchableSubPlan, explainStr, tableNames,
extraFields);
+ boolean liteModeImplicitSortApplied = false;
+ int liteModeEffectiveSortLimit = -1;
+ PhysicalPlannerContext physicalPlannerContext =
plannerContext.getPhysicalPlannerContext();
+ if (physicalPlannerContext != null &&
physicalPlannerContext.isLiteModeImplicitSortApplied()) {
+ liteModeImplicitSortApplied = true;
+ liteModeEffectiveSortLimit =
physicalPlannerContext.getLiteModeEffectiveSortLimit();
+ }
+ return new QueryPlannerResult(dispatchableSubPlan, explainStr, tableNames,
extraFields,
Review Comment:
if we get rid of the boolean we can just pass
`physicalPlannerContext.getLiteModeEffectiveSortLimit()` here and avoid the 7
other lines above
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -641,6 +641,12 @@ private BrokerResponse
query(QueryEnvironment.CompiledQuery query, long requestI
DispatchableSubPlan dispatchableSubPlan = queryPlanResult.getQueryPlan();
+ // Inject the implicit leaf-stage limit as a query option so servers can
detect truncation
Review Comment:
you might also want to emit a metric when the limit is reached. also from
Cellar PoV, with the current change would you be able to identify exact queries
which hit this limit? you might want to ensure that broker event listener
captures this flag too
--
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]
