gortiz commented on code in PR #20183: URL: https://github.com/apache/pinot/pull/20183#discussion_r4512182800
########## 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: Thanks for measuring it. I kept this PR simple: the description now has a Performance section with your numbers and recommends indexing the columns that RLS policies use. Letting index-only user children seed or bound the RLS bitmap is listed as a follow-up. ########## 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: Good catch, fixed in c188b26ce4. The entries the RLS filter scans now go to `QueryScanCostContext`, so `maxEntriesScannedInFilter` sees them, and to the `numEntriesScannedInFilter` that the server query log and metrics read (`InstanceResponseBlock#putServerOnlyMetadata`). The response still leaves them out. `RowLevelSecurityFilterOperatorTest#testScanCost` and `QueryExecutorTest#testRowLevelSecurityHidesTableLayoutStats` cover both. ########## 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: Agreed. It is pre-existing and this PR doesn't change it, so I listed it under Known limitations and kept it as a follow-up. ########## 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: Agreed, listed under Known limitations. The follow-up is to pass the RLS documents as required candidates, the same way the upsert snapshot is passed today. -- 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]
