xiangfu0 commented on PR #19664: URL: https://github.com/apache/pinot/pull/19664#issuecomment-6027506839
Addressed the review in d5065b2950; each inline thread has a reply with what changed. For the points from the summary that had no thread: - `backward-incompat` label: nothing in the diff is incompatible, so it should come off; I could not remove it from this session, it needs a click from a maintainer (or me from the UI). - `database` option vs the single-stage header-only resolution: noted in the `resolveTableName` Javadoc and in the description's compatibility section. - Controller legacy `OPTION(...)` mode: the controller now registers `QueryOptionConfigListener`, so `legacySyntaxMode=REJECT`/`IGNORE` applies to the DML it executes on `/sql`, with a `PinotQueryResourceTest` case for each mode. - `useMultistageEngine=true` on a controller without MSE: the engine gate moved into the DQL branch, so DML passes its options through; a test pins it. - `BrokerRequest`-only access controls: the `authorizeDelete` and `authorizeDeleteRows` Javadoc now name the exact calls made and say that restrictions living only in the `BrokerRequest` overload must be re-applied in `authorizeDeleteRows`. I did not add a test pinning the 500, since that is not an outcome worth locking in. - Broker and controller hooks: both `createSqlQueryExecutor()` Javadocs point at each other and say both must be overridden; `NOT_SUPPORTED_MESSAGE` is role-neutral now, and the description says "both" instead of "or". - Controller `authorizeDelete` Javadoc: reworded to state what is checked and that it is stricter than the query path, not equivalent. - The unreachable `IllegalArgumentException` catch in `resolveTableName` is gone, and the WHERE-required message now says what the guard is for. - The executor hooks are now exercised end to end: `BasicAuthBatchIntegrationTest` starts the broker and the controller with a recording executor through `createSqlQueryExecutor()` and asserts the authorized DELETE reaches it with the resolved table, predicate and headers, while denied and unknown-table DELETEs never do. Two more things came out of re-verifying the fixes. Relaxing the raw-name check for typed `tables` principals let them reach the row-level-security lookup, which failed on the raw name with a 500; `hasRowFilters` now checks the typed name first and the raw name only when the caller is authorized for it, with a test on the real `BasicAuthAccessControlFactory`. And under ZooKeeper basic auth the broker's first-step `authorize(identity)` never authenticates, so an unauthenticated DELETE could still tell logical tables apart; the broker now also calls `authorize(identity, Set.of())` before looking the table up. Validation on the pushed tree: `DeleteStatementTest`, `CalciteSqlCompilerTest`, `SqlOptionsModeTest`, `SqlQueryExecutorTest`, `PinotClientRequestTest`, `BasicAuthAccessControlTest`, `ZkBasicAuthAccessControlFactoryTest`, `BrokerRequestHandlerDelegateTest`, `PinotQueryResourceTest` and `BasicAuthBatchIntegrationTest` pass locally; spotless, checkstyle and license checks pass on the touched modules; the warning-enabled compile reports no warnings on added lines. -- 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]
