Jackie-Jiang opened a new pull request, #19570:
URL: https://github.com/apache/pinot/pull/19570
## Summary
Adds two policies for query options embedded in the SQL text, as opposed to
options passed through the request payload (`queryOptions`). Both are driven by
a shared enum `QueryOptionsUtils.SqlOptionsMode { ALLOW, IGNORE, REJECT }`.
### Cluster config: `pinot.query.legacy.option.syntax.mode`
Controls the legacy PQL-style `OPTION(key=value)` suffix (the regex-stripped
path, as opposed to the standard `SET key = value;` statements):
- `ALLOW` (default): options are applied as always.
- `IGNORE`: the suffix is stripped and its options dropped, without
extracting them.
- `REJECT`: the statement fails with an error pointing at `SET`.
It applies to every statement type (DML `INSERT ... OPTION(taskName=...)`
included), since `SET` covers them all. Meant to be set in the ZK cluster
config so brokers and controllers pick it up together; read once at startup
(same pattern as `pinot.insecure.mode`), so a restart is needed to change it.
### Query option: `sqlOptionsMode`
Controls all SQL-embedded options (`SET` statements and the legacy suffix)
for a single request:
- `ALLOW` (default): SQL options are merged with precedence over request
options, as always.
- `IGNORE`: SQL options are dropped so only the request options apply.
- `REJECT`: the query fails (`QUERY_VALIDATION`) listing the offending keys.
Only honored from the request payload (REST `queryOptions`, gRPC metadata),
never from the SQL itself, so a gateway that sets request options on behalf of
its users can guarantee the query text cannot override them. An invalid value
fails with `QUERY_VALIDATION` even on queries without SQL options, so typos do
not go unnoticed. Applied in `RequestUtils.setOptions`, the single merge point
for REST, gRPC and `IN_SUBQUERY` sub-parses.
### Controller `/sql` parity
`PinotQueryResource` used to parse the SQL three times and merge request
options inconsistently: `putIfAbsent` (SQL wins) when picking the engine but
`putAll` (request wins) when resolving `database` for broker selection. With
the new modes this would have let the controller route a query differently from
how the broker executes it. It now builds the exact JSON it forwards, parses
once with `RequestUtils.parseQuery(sql, requestJson)`, and threads the result
through engine choice, `database`, MSE table-name compilation and SSE table
lookup. Behavior changes:
- `database` precedence on the controller is now SQL over request options,
matching the broker.
- The dead `CalciteSqlCompiler.compileToBrokerRequest` fallback (`sqlNode`
was never null there) is removed.
- The payload forwarded to the broker is unchanged.
### Broker error codes
The four broker parse-catch sites (`BaseBrokerRequestHandler`,
`BrokerRequestHandlerDelegate`, `BrokerGrpcServer`, `PinotClientRequest`)
mapped every parse exception to `SQL_PARSING`, which would have made the same
rejected request return `SQL_PARSING` from the broker but `QUERY_VALIDATION`
from the controller. They now preserve a `QueryException`'s code with
`SQL_PARSING` as the fallback. No existing response changes: until now only
`SQL_PARSING`-coded exceptions ever left `RequestUtils.parseQuery`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
