Jackie-Jiang commented on code in PR #20169:
URL: https://github.com/apache/pinot/pull/20169#discussion_r4498574767
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -561,6 +567,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:
Could we inline the `if (compileResult._hasCrossTableFunction)` and
`throwAccessDeniedError(...)` here? `rejectCrossTableFunctionsUnderRowFilters`
has one caller and takes six arguments just to perform that check. Keeping it
inside the existing nonempty-row-filters block makes the policy easier to
follow and lets us remove the helper and its lengthy Javadoc.
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java:
##########
@@ -537,16 +538,18 @@ private void handleSubquery(FilterContext filter,
TableExecutionInfo executionIn
/// rewrites the expression with the subquery response.
///
/// Currently only supports ID_SET subquery within the
IN_PARTITIONED_SUBQUERY transform function, which will be
- /// rewritten to an IN_ID_SET transform function.
+ /// rewritten to an IN_ID_SET transform function. The function is not
allowed under row-level security: the broker
+ /// rejects it, and a server behind an older broker fails the query, see
[#checkNoRowFilters(QueryContext)].
private void handleSubquery(ExpressionContext expression, TableExecutionInfo
executionInfo, TimerContext timerContext,
- ExecutorService executorService, long endTimeMs)
+ ExecutorService executorService, QueryContext queryContext)
throws Exception {
FunctionContext function = expression.getFunction();
if (function == null) {
return;
}
List<ExpressionContext> arguments = function.getArguments();
if (StringUtils.remove(function.getFunctionName(),
'_').equalsIgnoreCase(IN_PARTITIONED_SUBQUERY)) {
+ checkNoRowFilters(queryContext);
Review Comment:
Could we inline `checkNoRowFilters` here as well? It has one caller, and the
restriction is specifically on `IN_PARTITIONED_SUBQUERY`. Putting the existing
`RlsUtils.extractRlsFilters(...)` and `Preconditions.checkState(...)` directly
at the start of this branch makes that relationship clearer. A short comment
such as `Reject under RLS even when an older broker allows the function
through` is sufficient; the standalone helper and its lengthy Javadoc can be
removed.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -1179,6 +1187,12 @@ private CompileResult compileRequest(long requestId,
String query, SqlNodeAndOpt
return new CompileResult(new
BrokerResponseNative(QueryErrorCode.QUERY_VALIDATION, "Table name not found"));
}
+ // Collected before handleSubquery() replaces each IN_SUBQUERY call with
IN_ID_SET on its result, for
+ // doHandleRequest to reject cross-table functions once it knows whether
row filters apply. For a gapfill query
+ // the two differ, and only the stripped one reaches a server.
+ boolean hasCrossTableFunction = hasCrossTableFunction(serverPinotQuery)
Review Comment:
Could we gate this scan with `_enableRowColumnLevelAuth`? The boolean is
only consumed inside that same gate, so queries can skip the extra traversal
when row/column authorization is disabled. Keep the capture here, before
`handleSubquery()` removes `IN_SUBQUERY` calls:
```java
boolean hasCrossTableFunction = _enableRowColumnLevelAuth
&& (hasCrossTableFunction(serverPinotQuery)
|| (serverPinotQuery != pinotQuery &&
hasCrossTableFunction(pinotQuery)));
```
--
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]
