navina opened a new pull request, #19008:
URL: https://github.com/apache/pinot/pull/19008
## Summary
Fixes an `UnsupportedOperationException` that a broker throws when Row-Level
Security (RLS) filters are applied to a query on a table that also has an
**expression-override map** configured.
```
java.lang.UnsupportedOperationException
at
java.util.ImmutableCollections$AbstractImmutableList.replaceAll(ImmutableCollections.java:261)
at BaseSingleStageBrokerRequestHandler.handleExpressionOverride(...:2019)
at BaseSingleStageBrokerRequestHandler.handleExpressionOverride(...:1992)
at BaseSingleStageBrokerRequestHandler.doHandleRequest(...)
```
## Root cause
`RlsFiltersRewriter` combines the RLS predicate with the query's existing
`WHERE` clause via
`RequestUtils.getFunctionExpression(AND, List.of(rlsExpr, existingFilter))`.
The `getFunction(String, List<Expression>)`
overload stored the caller's list **by reference** (no defensive copy),
unlike its sibling
`Expression` / `Expression...` overloads which both wrap in `new
ArrayList<>(...)`. So the `AND` function's operand
list was the immutable list returned by `List.of(...)`.
Later, `BaseSingleStageBrokerRequestHandler.handleExpressionOverride` walks
the filter tree and mutates operands in
place via `function.getOperands().replaceAll(...)` — which throws on the
immutable list. The crash only surfaces when
**both** an RLS filter and a table-level expression override are present,
which is why it went unnoticed.
`RequestUtils` never appears as the immediate caller in the stack trace: it
is the *producer* of the `AND` node
(parked in the `PinotQuery`), and `handleExpressionOverride` is the
*consumer* that mutates it later.
## Fix
Two levels:
1. **`RlsFiltersRewriter`** now uses the varargs `getFunctionExpression`
overload.
2. **`RequestUtils.getFunction(String, List<Expression>)`** now defensively
copies operands into a mutable
`ArrayList`, unifying the contract across all three overloads and
documenting that returned operand lists are
mutable. This removes the same landmine for any other caller passing an
immutable list (`List.of(...)`,
`Arrays.asList(...)`, `Stream.toList()`). At least four sites across
three modules rely on this invariant
(`handleExpressionOverride`, `PredicateComparisonRewriter`,
`MergeEqInFilterOptimizer`, `MergeRangeFilterOptimizer`).
**Note for reviewers:** this hardens a shared `pinot-common` factory with
many call sites across the query
parse/rewrite/optimize paths. The `List` overload changes from aliasing the
caller's list to copying it. The copy is
shallow, one-time, and per-query-node — off the per-row/per-segment hot path.
## Tests
- `RequestUtilsTest#testGetFunctionReturnsMutableOperandList` pins the
mutable-operand-list contract directly for all
three `getFunction` overloads with immutable inputs.
-
`BaseSingleStageBrokerRequestHandlerTest#testRlsFilterCombinedWithExpressionOverrideDoesNotThrow`
reproduces the exact
crash: RLS enabled on a query with an existing `WHERE` clause for a table
whose expression-override map is non-null.
It fails without the fix and, with it, asserts the request completes and
the server query carries both predicates
under a single `AND`.
--
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]
