xiangfu0 commented on code in PR #20183: URL: https://github.com/apache/pinot/pull/20183#discussion_r4496598474
########## pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RowLevelSecurityFilterOperator.java: ########## @@ -0,0 +1,126 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.core.operator.filter; + +import java.util.List; +import javax.annotation.Nullable; +import org.apache.pinot.core.common.BlockDocIdSet; +import org.apache.pinot.core.common.Operator; +import org.apache.pinot.core.operator.docidsets.AndDocIdSet; +import org.apache.pinot.core.operator.docidsets.BitmapDocIdSet; +import org.apache.pinot.core.operator.docidsets.EmptyDocIdSet; +import org.apache.pinot.core.query.request.context.QueryContext; +import org.apache.pinot.spi.trace.Tracing; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; +import org.roaringbitmap.buffer.MutableRoaringBitmap; + + +/// Applies the row-level security (RLS) filter of the query before the user filter. +/// +/// On its first block, this operator materializes the result of the RLS filter into a bitmap, bounded by the number of +/// documents that the plan read. It then ANDs that bitmap with the user filter, with the AND restriction push-down on, +/// so the user filter only evaluates scans and transform functions on the documents that the RLS filter matches. When +/// the segment has a valid-document snapshot (upsert), the bitmap is also ANDed with that snapshot. +/// +/// The bitmap is materialized once and reused for every later block, for example when a filtered aggregation reads +/// this operator once per sub-filter. +/// +/// This operator never reports a match-all result, an optimized count, or bitmaps. Then the operators that read the +/// filter result through these short cuts (for example `FastFilteredCountOperator` or +/// `NonScanBasedAggregationOperator`) cannot skip the RLS bitmap. +/// +/// Not thread-safe: a segment plan runs on one thread. +public class RowLevelSecurityFilterOperator extends BaseFilterOperator { + private static final String EXPLAIN_NAME = "FILTER_ROW_LEVEL_SECURITY"; + + private final BaseFilterOperator _rowFilterOperator; + private final BaseFilterOperator _userFilterOperator; + @Nullable + private final ImmutableRoaringBitmap _validDocIds; + private final QueryContext _queryContext; + + @Nullable + private ImmutableRoaringBitmap _rowFilterDocIds; + + /// @param rowFilterOperator the operator of the RLS filter. It must not be empty or match all documents. + /// @param userFilterOperator the operator of the WHERE clause. It must not be empty. + /// @param validDocIds the valid-document snapshot of the segment, or `null` when the segment has none + /// @param numDocs the number of documents that the plan read + public RowLevelSecurityFilterOperator(BaseFilterOperator rowFilterOperator, BaseFilterOperator userFilterOperator, + @Nullable ImmutableRoaringBitmap validDocIds, QueryContext queryContext, int numDocs) { + super(numDocs, false); + assert !rowFilterOperator.isResultEmpty() && !rowFilterOperator.isResultMatchingAll() + && !userFilterOperator.isResultEmpty(); + _rowFilterOperator = rowFilterOperator; + _userFilterOperator = userFilterOperator; + _validDocIds = validDocIds; + _queryContext = queryContext; + } + + @Override + protected BlockDocIdSet getTrues() { + Tracing.activeRecording().setNumChildren(2); + ImmutableRoaringBitmap rowFilterDocIds = getRowFilterDocIds(); + if (rowFilterDocIds.isEmpty()) { + return EmptyDocIdSet.unscanned(); + } + // The cost of the RLS filter is not reported in numEntriesScannedInFilter: without an index it is the number of + // documents of the segment, which shows the size of the table to the user + BlockDocIdSet rowFilterDocIdSet = BitmapDocIdSet.create(rowFilterDocIds, _numDocs); + if (_userFilterOperator.isResultMatchingAll()) { + return rowFilterDocIdSet; + } + BlockDocIdSet userFilterDocIdSet = _userFilterOperator.nextBlock().getBlockDocIdSet(); + return new AndDocIdSet(List.of(rowFilterDocIdSet, userFilterDocIdSet), _queryContext.getQueryOptions(), true); + } + + /// Returns the documents that the RLS filter matches, ANDed with the valid-document snapshot. Materializes them on + /// the first call, and adds the time it takes to [QueryContext#addRowFilterTimeNs(long)]. + private ImmutableRoaringBitmap getRowFilterDocIds() { + if (_rowFilterDocIds == null) { + long startTimeNs = System.nanoTime(); + ImmutableRoaringBitmap docIds = _rowFilterOperator.getFilteredDocIds().getDocIds(); Review Comment: This always materializes the RLS filter over the whole segment, even when the user filter has a selective index. Local p50, 4×1M-doc segments, `tenant` without an index, RLS `tenant='t7'` (merge-base → PR), 2 runs × 200 iterations: - `AND userId = 1233` (inverted): 1.75–1.83 → 3.54–3.73 ms - `AND userId IN (…)` LIMIT 10: 1.07–1.15 → 2.54–2.73 ms - `AND metric > 900` (scan): 11.5–11.7 → 3.2–3.5 ms With an inverted index on `tenant`, the PR adds about 0.1–0.8 ms. Index-only user children run no user expression on rows, so they could seed or bound this bitmap. If you'd rather keep it simple, documenting that RLS columns should be indexed would also work. ########## pinot-core/src/main/java/org/apache/pinot/core/operator/filter/RowLevelSecurityFilterOperator.java: ########## @@ -0,0 +1,126 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.core.operator.filter; + +import java.util.List; +import javax.annotation.Nullable; +import org.apache.pinot.core.common.BlockDocIdSet; +import org.apache.pinot.core.common.Operator; +import org.apache.pinot.core.operator.docidsets.AndDocIdSet; +import org.apache.pinot.core.operator.docidsets.BitmapDocIdSet; +import org.apache.pinot.core.operator.docidsets.EmptyDocIdSet; +import org.apache.pinot.core.query.request.context.QueryContext; +import org.apache.pinot.spi.trace.Tracing; +import org.roaringbitmap.buffer.ImmutableRoaringBitmap; +import org.roaringbitmap.buffer.MutableRoaringBitmap; + + +/// Applies the row-level security (RLS) filter of the query before the user filter. +/// +/// On its first block, this operator materializes the result of the RLS filter into a bitmap, bounded by the number of +/// documents that the plan read. It then ANDs that bitmap with the user filter, with the AND restriction push-down on, +/// so the user filter only evaluates scans and transform functions on the documents that the RLS filter matches. When +/// the segment has a valid-document snapshot (upsert), the bitmap is also ANDed with that snapshot. +/// +/// The bitmap is materialized once and reused for every later block, for example when a filtered aggregation reads +/// this operator once per sub-filter. +/// +/// This operator never reports a match-all result, an optimized count, or bitmaps. Then the operators that read the +/// filter result through these short cuts (for example `FastFilteredCountOperator` or +/// `NonScanBasedAggregationOperator`) cannot skip the RLS bitmap. +/// +/// Not thread-safe: a segment plan runs on one thread. +public class RowLevelSecurityFilterOperator extends BaseFilterOperator { + private static final String EXPLAIN_NAME = "FILTER_ROW_LEVEL_SECURITY"; + + private final BaseFilterOperator _rowFilterOperator; + private final BaseFilterOperator _userFilterOperator; + @Nullable + private final ImmutableRoaringBitmap _validDocIds; + private final QueryContext _queryContext; + + @Nullable + private ImmutableRoaringBitmap _rowFilterDocIds; + + /// @param rowFilterOperator the operator of the RLS filter. It must not be empty or match all documents. + /// @param userFilterOperator the operator of the WHERE clause. It must not be empty. + /// @param validDocIds the valid-document snapshot of the segment, or `null` when the segment has none + /// @param numDocs the number of documents that the plan read + public RowLevelSecurityFilterOperator(BaseFilterOperator rowFilterOperator, BaseFilterOperator userFilterOperator, + @Nullable ImmutableRoaringBitmap validDocIds, QueryContext queryContext, int numDocs) { + super(numDocs, false); + assert !rowFilterOperator.isResultEmpty() && !rowFilterOperator.isResultMatchingAll() + && !userFilterOperator.isResultEmpty(); + _rowFilterOperator = rowFilterOperator; + _userFilterOperator = userFilterOperator; + _validDocIds = validDocIds; + _queryContext = queryContext; + } + + @Override + protected BlockDocIdSet getTrues() { + Tracing.activeRecording().setNumChildren(2); + ImmutableRoaringBitmap rowFilterDocIds = getRowFilterDocIds(); + if (rowFilterDocIds.isEmpty()) { + return EmptyDocIdSet.unscanned(); + } + // The cost of the RLS filter is not reported in numEntriesScannedInFilter: without an index it is the number of + // documents of the segment, which shows the size of the table to the user + BlockDocIdSet rowFilterDocIdSet = BitmapDocIdSet.create(rowFilterDocIds, _numDocs); Review Comment: Reporting 0 entries here hides the RLS cost from more than the response. `DocIdSetOperator` feeds `getNumEntriesScannedInFilter()` into `QueryScanCostContext`, so scan-based query killing (`maxEntriesScannedInFilter`) no longer sees a full-segment RLS scan. The server query log loses it too. Could the cost go to the scan-cost context and server-only metadata, and stay out of the response? ########## pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java: ########## @@ -116,8 +122,21 @@ public BaseFilterOperator run() { // Candidate generation and the outer valid-document AND must observe the same document set for this query. ImmutableRoaringBitmap outerDocIdsSnapshot = docIdsSnapshot; - if (_filter != null) { - BaseFilterOperator filterOperator = constructPhysicalOperator(_filter, numDocs); + BaseFilterOperator filterOperator = _filter != null ? constructPhysicalOperator(_filter, numDocs) : null; Review Comment: Vector top-K still ranks hidden rows. `_requiredVectorDocIds` (line 119) carries only the upsert snapshot candidate generation, and the RLS bitmap is applied after the search. So a tenant can get fewer than K hits, and the hit count depends on other tenants' vectors. This is from reading the source; I didn't run it. Worth listing as a known limitation, or a follow-up to pass the RLS docs as required candidates. ########## pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java: ########## @@ -116,8 +122,21 @@ public BaseFilterOperator run() { // Candidate generation and the outer valid-document AND must observe the same document set for this query. ImmutableRoaringBitmap outerDocIdsSnapshot = docIdsSnapshot; - if (_filter != null) { - BaseFilterOperator filterOperator = constructPhysicalOperator(_filter, numDocs); + BaseFilterOperator filterOperator = _filter != null ? constructPhysicalOperator(_filter, numDocs) : null; + if (_rowFilter != null && (filterOperator == null || !filterOperator.isResultEmpty())) { Review Comment: Not a regression, but a residual side channel that may belong under Known limitations. The user predicate's dictionary lookup still covers hidden values. With RLS `tenant='A'`, `secret='s7'` (exists only on a tenant-B row) returns 0 rows with `numEntriesScannedInFilter=40000`. `secret='s99999'` (exists nowhere) returns 0 rows with `numEntriesScannedInFilter=0`. The same split shows up on master (40012 vs 0). ########## pinot-core/src/main/java/org/apache/pinot/core/plan/FilterPlanNode.java: ########## @@ -116,8 +122,21 @@ public BaseFilterOperator run() { // Candidate generation and the outer valid-document AND must observe the same document set for this query. ImmutableRoaringBitmap outerDocIdsSnapshot = docIdsSnapshot; - if (_filter != null) { - BaseFilterOperator filterOperator = constructPhysicalOperator(_filter, numDocs); + BaseFilterOperator filterOperator = _filter != null ? constructPhysicalOperator(_filter, numDocs) : null; Review Comment: Vector top-K still ranks hidden rows. `_requiredVectorDocIds` (line 119) carries only the upsert snapshot into candidate generation, and the RLS bitmap is applied after the search. So a tenant can get fewer than K hits, and the hit count depends on other tenants' vectors. This is from reading the source; I didn't run it. Worth listing as a known limitation, or a follow-up to pass the RLS docs as required candidates. -- 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]
