xiangfu0 commented on code in PR #19589:
URL: https://github.com/apache/pinot/pull/19589#discussion_r4568300790
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -266,35 +267,65 @@ public Plan makeInstancePlan(List<SegmentContext>
segmentContexts, QueryContext
new InstanceResponsePlanNode(combinePlanNode, fetchSegmentContexts,
fetchContexts, queryContext));
}
- /// Resolves [AndRestrictionPushdownMode] for a query.
+ /// Resolves [AndRestrictionPushdownMode] for a query into an
[AndRestrictionPushdown].
///
- /// The push-down materializes the filter result instead of streaming it, so
under AUTO it is only applied to a
- /// query that is going to read every matching document anyway. That is
exactly the aggregation and group-by
- /// queries:
+ /// - `NEVER`: no push-down.
+ /// - `ALWAYS`: the eager push-down, which materializes the filter result.
+ /// - `STREAMING`: the streaming push-down, which evaluates the filter
lazily, one chunk at a time.
+ /// - `AUTO`: picks by query shape, see [#resolveAutoAndRestrictionPushdown].
///
- /// - a selection query without ORDER BY stops as soon as it has LIMIT rows;
- /// - a DISTINCT query without ORDER BY stops the same way, in
`DistinctOperator#getNextBlock`;
- /// - whether a selection query *with* ORDER BY can stop early is decided
per segment, because
- /// [org.apache.pinot.core.plan.SelectionPlanNode] picks
`SelectionPartiallyOrderedBy*` from the segment's sorted
- /// column, so it cannot be ruled out here and is excluded conservatively;
- /// - LIMIT 0 reads nothing at all.
- ///
- /// Use ALWAYS to push down on a workload known not to terminate early.
- private static boolean
isAndRestrictionPushdownEnabled(AndRestrictionPushdownMode mode, QueryContext
queryContext) {
+ /// A query with a row-level security (RLS) filter never runs without the
push-down: every nested AND must restrict
+ /// its children to the documents the RLS filter matches. For it, `NEVER`
falls back to the `AUTO` choice.
+ @VisibleForTesting
+ static AndRestrictionPushdown
resolveAndRestrictionPushdown(AndRestrictionPushdownMode mode,
+ QueryContext queryContext) {
switch (mode) {
case ALWAYS:
- return true;
+ return AndRestrictionPushdown.EAGER;
case NEVER:
- return false;
+ return queryContext.getRowFilter() != null ?
resolveAutoAndRestrictionPushdown(queryContext)
+ : AndRestrictionPushdown.OFF;
+ case STREAMING:
+ return
AndRestrictionPushdown.streaming(getMaxDocsToRead(queryContext));
Review Comment:
Explicit `STREAMING` passes `limit + offset` as `maxDocsToRead` for every
kind of query. Aggregation, group-by, DISTINCT and ORDER BY queries read the
whole filter anyway. With the default `LIMIT 10`, the EAGER fallback in
`AndDocIdSet#shouldStream` never triggers for them. I confirmed this locally:
`COUNT(*) … AND (payload > 900 OR name = 'n7')` under `STREAMING` scans 800,000
entries, against 760,404 under `ALWAYS`. Consider passing `Integer.MAX_VALUE`
for anything that is not selection-only, or say in the docs that `STREAMING`
streams every query. The latency difference was within noise in my run, so this
does not block.
--
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]
