xiangfu0 commented on code in PR #19027:
URL: https://github.com/apache/pinot/pull/19027#discussion_r3632343144
##########
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:
This makes the SQL option contract closed and default-on. Existing `SET` /
`OPTION(...)` queries with custom keys were previously accepted and propagated,
but every DQL parser call now rejects them. Custom components can consume
`BrokerRequest` options without any way to register keys in this allowlist, so
an upgrade can break deployed queries and plugin behavior immediately. Please
preserve unknown keys by default behind an opt-in strict mode, or provide an
extension registry and staged rollout.
--
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]
