gortiz commented on PR #20183: URL: https://github.com/apache/pinot/pull/20183#issuecomment-6738858021
Thanks for the second pass. I rebased on master and pushed c188b26ce4 on top. Going through your list: 1. H3: fixed. Under RLS, both H3 operators now return the lazy `OR(fullMatch, AND(partialMatch, expression))` instead of materializing it, so the RLS push-down limits the expression to the allowed rows. Without RLS they keep the eager bitmap, so nothing changes there. `RowLevelSecurityH3QueriesTest` compares a segment with hidden rows against one without them, for `ST_Distance` (upper, lower and both bounds) and `ST_Contains` (positive and negative). It fails without the fix. 2. LIMIT: added to the description. #19589 is my draft that streams the restriction instead of materializing it, which keeps the early termination. 3. Double evaluation: listed as a follow-up. 4. Client-supplied `rlsFilters-*`: I left this one out on purpose. The multi-stage leaf already applies whatever `rlsFilters-<table>` arrives in the request metadata, client-supplied ones included, and with this PR the single-stage server behaves the same. A client can only narrow its own result, so nothing leaks, and being able to set a policy from the client is handy for testing one. If we want to lock it down, I'd do it in a separate PR for both engines. 5. Multi-stage broker log: listed under Known limitations. 6. WHERE copy: fixed. The single-stage broker now optimizes the query again after the RLS rewrite, so both copies match the same rows on every table. Covered by `testRlsFilterInWhereClauseIsOptimized`. 7. Rebase: done. 8. The description now has a Release notes section. The docs update is a follow-up. Smaller ones: the fail-closed path now records `QUERY_VALIDATION_EXCEPTIONS`, and the test is renamed to `testMaterializedViewRewriteSkippedUnderRls`. Test gaps: added policies that are an expression, an OR and the `( f1 ) AND ( f2 )` shape; group-by, distinct and order-by over the whole user-filter matrix; the H3 test above; an `EXPLAIN` assertion; and `QueryRunnerTest#testUserFilterDoesNotRunOnRowsHiddenByRowLevelSecurity`, which checks that the multi-stage leaf applies the RLS filter before a user filter that throws on hidden rows. I didn't add a benchmark for item 2. -- 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]
