Vamsi-klu opened a new pull request, #19027: URL: https://github.com/apache/pinot/pull/19027
## Summary Rejects **unknown** SQL query options on the DQL `SET` / legacy `OPTION(...)` path with a clear error (and close-typo suggestion), while keeping REST/JSON free-form options for backward compatibility. **Issue:** Fixes apache/pinot#7207 --- ## What is the problem? Unknown or misspelled query options in SQL were **preserved and silently ignored**. Users believed they had set timeouts, trim flags, null-handling, etc., but nothing happened — a production foot-gun. Note: the original issue example `timeoutMS` vs `timeoutMs` is **already** handled by case-insensitive canonicalization of known `QueryOptionKey` names. The **remaining** gap was truly unknown keys. --- ## Why did I do this? Silent ignore of control knobs causes hard-to-debug latency and correctness incidents. Failing fast on SQL-supplied unknown keys matches the issue title (“Throw exception”) and improves operator UX with Levenshtein “Did you mean …?” suggestions for near misses. --- ## What is the implementation edit? | File | Change | |------|--------| | `QueryOptionsUtils.java` | New `resolveAndValidateSqlQueryOptions`; allowlist only `trace` + `database` (canonicalized to lowercase constants); **reject** user-supplied `rlsFilters*` (broker injects RLS after parse); Levenshtein suggestion on error path only | | `CalciteSqlParser.java` | Wire validation for DQL SET options and legacy `OPTION(...)` map; DML keeps free-form SET (e.g. INSERT INTO FILE task props) | | `QueryOptionsUtilsTest.java` | Validate path: known keys, case aliases, canonical trace/database, reject RLS, reject unknown, empty map, suggestions | | `CalciteSqlCompilerTest.java` | Parser tests: known, wrong-case, unknown, suggestions, TRACE/DATABASE canonicalization, RLS reject | | `BrokerRequestOptionsTest.java` | SQL path uses known keys; JSON free-form still allowed | **API sketch:** ```java // SQL DQL — rejects unknown keys Map<String, String> opts = QueryOptionsUtils.resolveAndValidateSqlQueryOptions(raw); // REST/JSON / DML / internal — unknown keys preserved Map<String, String> opts = QueryOptionsUtils.resolveCaseInsensitiveOptions(raw); ``` **Design tradeoffs:** | Choice | Decision | Why | |--------|----------|-----| | Reject vs warn | **Reject** | Clear failure; matches issue | | SQL only vs all paths | **SQL DQL SET/OPTION only** | REST free-form BC; DML free-form task props | | Extra allowlist | `trace`, `database` | Request context / legacy flags not on `QueryOptionKey` | | RLS | **Reject on SQL path** | Broker injects after parse; must not be user-settable | | Typo UX | Levenshtein suggestion when close | e.g. `timoutMs` → `timeoutMs` | --- ## What is the impact? - **Breaking (SQL DQL only):** Queries that used arbitrary free-form `SET`/`OPTION` keys will **fail parse** after this change. - **Non-breaking:** REST/JSON `queryOptions` free-form keys still work; broker-injected options after parse unchanged; DML SET free-form preserved. - **Performance:** Validation is O(options) on parse; Levenshtein only on error path. No query execution hot-path change. - **Security/correctness:** Users can no longer inject `rlsFilters*` via SQL; `SET TRACE` / `SET DATABASE` canonicalize so broker lookups succeed. Please consider a release-note / backward-incompat mention for SQL free-form options. --- ## What is the testing edit? ```text ./mvnw -pl pinot-common -Dtest=QueryOptionsUtilsTest,CalciteSqlCompilerTest test # Tests run: 126, Failures: 0 ./mvnw -pl pinot-broker -Dtest=BrokerRequestOptionsTest test # Tests run: 1, Failures: 0 ``` Covers: canonicalization, unknown reject, typo suggestion, TRACE/DATABASE case aliases, RLS reject, REST free-form still allowed, empty options map. Not run: full integration suite / multi-node E2E. --- ## Checklist - [x] Draft - [x] BC risk called out (SQL-only) - [x] Unit + parser + broker wiring tests -- 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]
