gortiz commented on code in PR #20169:
URL: https://github.com/apache/pinot/pull/20169#discussion_r4496220863
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -561,6 +565,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()) {
+ rejectCrossTableFunctionsUnderRowFilters(requestId, query,
requestContext, tableName, rawTableName,
Review Comment:
About 20 lines below this check, the `catch` around `RlsFiltersRewriter` is
still fail-open. It logs "Row-level security filtering will be disabled for
this query" and forwards the query without the row filter. That doesn't come
from this PR, but it is the one remaining way the broker sends an unfiltered
query for a principal with row filters, which is the contract this PR describes.
[apache/pinot#20183](https://github.com/apache/pinot/pull/20183) changes
that `catch` to fail the query with `QUERY_VALIDATION`, and it doesn't put the
filter or the parse error in the response. So there's nothing to change here.
The two PRs edit the same block of `doHandleRequest`, though, and both touch
`ServerQueryExecutorV1Impl`. I'd merge this one first and rebase #20183 on top.
--
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]
