Vamsi-klu commented on code in PR #19027:
URL: https://github.com/apache/pinot/pull/19027#discussion_r3634664556
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -95,6 +112,100 @@ public static Map<String, String>
resolveCaseInsensitiveOptions(Map<String, Stri
return resolved;
}
+ /**
+ * Resolves known option keys case-insensitively and rejects unsupported
keys.
+ * <p>
+ * Intended for SQL-supplied {@code SET} / {@code OPTION(...)} options only.
Do not use for
+ * REST/JSON {@code queryOptions} (kept free-form for backward compatibility
— unknown keys are
+ * still silently preserved there) or broker-injected options. DML
statements that intentionally
+ * carry free-form task properties should continue to use
+ * {@link #resolveCaseInsensitiveOptions(Map)}.
+ * <p>
+ * Additional SQL keys ({@code trace}, {@code database}) are accepted
case-insensitively and
+ * stored under their canonical lowercase names so broker lookups succeed.
+ *
+ * @throws IllegalArgumentException if an unsupported option key is present
+ */
+ public static Map<String, String>
resolveAndValidateSqlQueryOptions(Map<String, String> queryOptions) {
+ if (CLASS_LOAD_ERROR != null) {
+ throw CLASS_LOAD_ERROR;
+ }
+
+ Map<String, String> resolved = new HashMap<>();
+ for (Map.Entry<String, String> configEntry : queryOptions.entrySet()) {
+ String key = configEntry.getKey();
+ String lower = key.toLowerCase();
+ String canonical = CONFIG_RESOLVER.get(lower);
+ if (canonical != null) {
+ resolved.put(canonical, configEntry.getValue());
+ continue;
+ }
+ String additionalCanonical = ADDITIONAL_SQL_OPTION_KEYS.get(lower);
+ if (additionalCanonical != null) {
+ resolved.put(additionalCanonical, configEntry.getValue());
+ continue;
+ }
+ throw new IllegalArgumentException(buildUnsupportedOptionMessage(key));
Review Comment:
Good catch - agreed this was too aggressive as a default-on closed allowlist.
Addressed as follows:
1. **Default preserves unknown keys** on SQL DQL `SET` / `OPTION(...)` so
custom/plugin option consumers keep working across upgrades.
2. **Opt-in strict mode**
- Per-query: `SET strictQueryOptions=true`
- Broker-wide: `pinot.broker.query.sql.strictQueryOptions` (default
`false`) via process-wide default set at broker startup
3. **Extension registry** for plugins that need known keys under strict mode:
`QueryOptionsUtils.registerSqlQueryOptionKey("myCustomKey")`
4. Still always reject user-supplied `rlsFilters*` on the SQL path (broker
injects RLS after parse).
5. Case-insensitive canonicalization for known keys (+ `trace` / `database`)
and Levenshtein "Did you mean ...?" on strict reject paths.
REST/JSON free-form `queryOptions` and DML free-form SET/OPTION are
unchanged.
Staged rollout: leave broker config false on upgrade -> enable after
validating clients -> optional future default flip.
PTAL when you get a chance.
--
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]
