xiangfu0 commented on code in PR #20169:
URL: https://github.com/apache/pinot/pull/20169#discussion_r4487194919
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java:
##########
@@ -579,11 +589,48 @@ private void handleSubquery(ExpressionContext expression,
TableExecutionInfo exe
arguments.set(1,
ExpressionContext.forLiteral(RequestUtils.getLiteral(((IdSet)
result).toBase64String())));
} else {
for (ExpressionContext argument : arguments) {
- handleSubquery(argument, executionInfo, timerContext, executorService,
endTimeMs);
+ handleSubquery(argument, executionInfo, timerContext, executorService,
queryContext);
}
}
}
+ /// Adds the row-level security filter of the queried table to the subquery
of an `IN_PARTITIONED_SUBQUERY`.
+ ///
+ /// The broker adds the row filters of the principal to the WHERE clause of
the query it sends and stamps them in
+ /// the query options under the key of the table it authorized (see
[RlsUtils#buildRlsFilterKey(String)]), but the
+ /// subquery reaches the server as an opaque string literal that carries
neither, so without this it would build
+ /// its ID_SET from rows the principal is not allowed to see. The subquery
runs on the segments selected for the
+ /// outer query whatever table its FROM clause names, so the filter of the
outer table is the one that applies: the
+ /// data source is renamed to that table so that the rewriter finds the
filter even when the FROM clause names
+ /// another table. The filter is added to the options the subquery was
written with, overriding any row filter
+ /// smuggled into them, so that a subquery nested in it is filtered the same
way. A query that carries row filters
+ /// but none for the queried table, which an older broker produces when a
materialized view rewrite swaps the table
+ /// after the filters were stamped, fails rather than run the subquery
unfiltered.
+ private static void applyRowFilters(PinotQuery subquery, QueryContext
queryContext) {
+ Map<String, String> queryOptions = queryContext.getQueryOptions();
+ if (queryOptions.isEmpty()) {
+ return;
+ }
+ Map<String, String> rowFilters = RlsUtils.extractRlsFilters(queryOptions);
+ if (rowFilters.isEmpty()) {
+ return;
+ }
+ String tableName = queryContext.getTableName();
+ Preconditions.checkState(tableName != null, "Row-level security filters %s
are set, but the query has no table",
+ rowFilters.keySet());
+ String rowFilterKey =
RlsUtils.buildRlsFilterKey(TableNameBuilder.extractRawTableName(tableName));
+ String rowFilter = rowFilters.get(rowFilterKey);
+ Preconditions.checkState(!StringUtils.isEmpty(rowFilter),
Review Comment:
MINOR (testing): this fail-closed branch is the mixed-version safety net
(older broker plus an MV swap), but no test covers it. I checked it locally:
with only `rlsFilters-other` set, the query fails with this message. Consider
adding that case to `testInPartitionedSubqueryAppliesRowFilters` so a later
refactor can't turn it into a silent no-op.
---
_AI-generated review via IntentLab · Model: Claude Opus 5.5
(claude-opus-5-5)._
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -561,6 +560,8 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
TableRowColAccessResult rlsFilters =
accessControl.getRowColFilters(requesterIdentity, tableName);
List<String> rowFilters = rlsFilters.getRLSFilters().orElse(null);
if (rowFilters != null && !rowFilters.isEmpty()) {
+ validateCrossTableFunctionsUnderRowFilters(requestId, query,
requestContext, tableName, rawTableName,
Review Comment:
MINOR (scope, pre-existing): this check only runs in the physical-table
`else` branch. The logical-table branch above never calls `getRowColFilters`,
so it applies neither the WHERE-clause row filters nor this rejection. No new
leak comes from this PR, because the outer query there is unfiltered as well.
Still, "the broker never forwards the function for a principal with row
filters" holds only for physical tables. Please narrow that wording or open a
follow-up issue.
---
_AI-generated review via IntentLab · Model: Claude Opus 5.5
(claude-opus-5-5)._
--
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]
