clintropolis commented on a change in pull request #10253:
URL: https://github.com/apache/druid/pull/10253#discussion_r468813252
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -409,7 +412,10 @@ public boolean getContextSortByDimsFirst()
@JsonIgnore
public boolean isApplyLimitPushDown()
{
- return applyLimitPushDown;
+ if (forceLimitPushDown == null) {
+ forceLimitPushDown = validateAndGetForceLimitPushDown();
+ }
+ return forceLimitPushDown || canPushDownLimit;
Review comment:
super nit: it seems funny to have `...LimitPushDown` and
`...PushDownLimit`, but I'm not really sure what is the best way to square
these up. I guess `...LimitPushDown` is more prevalent, so maybe renaming to
`canDoLimitPushDown` to be more consistent?
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -121,6 +120,10 @@ public static Builder builder()
@Nullable
private final DateTime universalTimestamp;
+ private final boolean canPushDownLimit;
+ @Nullable
+ private Boolean forceLimitPushDown;
Review comment:
I think this would cause extra calls to
`validateAndGetForceLimitPushDown` whenever `isApplyLimitPushdown` is called
because of being unable to distinguish between an actual `false` and the value
not being computed yet. It's probably not especially harmful, but it also seems
unnecessary.
##########
File path:
processing/src/main/java/org/apache/druid/query/groupby/GroupByQuery.java
##########
@@ -474,14 +480,12 @@ private RowSignature computeResultRowSignature()
.build();
}
- private boolean determineApplyLimitPushDown()
+ private boolean canPushDown()
Review comment:
nit: this method name should probably include `LimitPushDown` or at
least `Limit` somehow to make it clearer that it applies to limits, maybe
`canDoLimitPushDown()` if you decide to rename the field as well?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
