Vamsi-klu commented on code in PR #19027:
URL: https://github.com/apache/pinot/pull/19027#discussion_r3755550157
##########
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:
Reworked in `94be569`, and the default is byte identical to master now.
Unknown `SET` and `OPTION` keys are preserved with no validation and no
logging on the default path. The three default-behavior test files,
`QueryOptionsUtilsTest`, `CalciteSqlCompilerTest` and
`BrokerRequestOptionsTest`, are restored to master's content exactly, so a diff
against master shows no change to them at all. The whole PR is now 408 added
lines and one deleted.
Validation moved behind a broker config with three modes: NONE by default,
WARN which logs the unknown key with a typo suggestion and dedupes per distinct
key so a high-QPS client with one misspelled option cannot flood the logs, and
REJECT which fails fast. `registerSqlQueryOptionKey` stays as the plugin
allowlist for REJECT. DML `SET` and REST or JSON `queryOptions` payloads stay
permissive in every mode.
@Jackie-Jiang that covers both of your asks, the unchanged default and the
reverted tests. One open question I did not want to decide silently:
user-supplied `rlsFilters` keys currently only get rejected under REJECT mode,
because making that always-on would itself be a default behavior change. Happy
to make it unconditional if you would rather have the security guard always
active.
--
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]
