You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2026/09/16 01:13:08 UTC
[PR] Add SQL query option policies: legacy OPTION() cluster config and per-request sqlOptionsMode [pinot]
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add SQL query option policies: legacy OPTION() cluster config and per-request sqlOptionsMode [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19570:
URL: https://github.com/apache/pinot/pull/19570#issuecomment-5690778136
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19570?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.61905%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.94%. Comparing base ([`a979a6a`](https://app.codecov.io/gh/apache/pinot/commit/a979a6a0d8e173e887a68dd294e8013dc16ca43d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1718b4d`](https://app.codecov.io/gh/apache/pinot/commit/1718b4dd7767d928c17cf1a48ea2d6b3616b823b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19570?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...pache/pinot/common/utils/request/RequestUtils.java](https://app.codecov.io/gh/apache/pinot/pull/19570?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Futils%2Frequest%2FRequestUtils.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvcmVxdWVzdC9SZXF1ZXN0VXRpbHMuamF2YQ==) | 93.33% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19570?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (a979a6a) and HEAD (1718b4d). Click for more details.
>
> <details><summary>HEAD has 19 uploads less than BASE</summary>
>
>| Flag | BASE (a979a6a) | HEAD (1718b4d) |
>|------|------|------|
>|unittests|2|1|
>|java-25|5|1|
>|temurin|5|1|
>|unittests2|1|0|
>|lane-a|1|0|
>|integration|3|0|
>|integration2|2|0|
>|lane-b|2|0|
>|integration1|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19570 +/- ##
============================================
- Coverage 67.69% 57.94% -9.75%
+ Complexity 1444 1 -1443
============================================
Files 3499 2689 -810
Lines 225977 164823 -61154
Branches 35678 26769 -8909
============================================
- Hits 152974 95508 -57466
- Misses 60917 61260 +343
+ Partials 12086 8055 -4031
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.94% <97.61%> (-9.75%)` | :arrow_down: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.94% <97.61%> (-9.75%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.94% <97.61%> (-9.75%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.94% <97.61%> (+0.03%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19570/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19570?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add SQL query option policies: legacy OPTION() cluster config and per-request sqlOptionsMode [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #19570:
URL: https://github.com/apache/pinot/pull/19570
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add SQL query option policies: legacy OPTION() cluster config and per-request sqlOptionsMode [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #19570:
URL: https://github.com/apache/pinot/pull/19570#issuecomment-5707016810
Documentation follow-up: https://github.com/pinot-contrib/pinot-docs/pull/1050 (merged). It covers the live cluster policies for unknown SQL option keys and legacy OPTION syntax, the request-only sqlOptionsMode control, and rolling-upgrade guidance.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org