Vamsi-klu commented on code in PR #19027:
URL: https://github.com/apache/pinot/pull/19027#discussion_r3725704035
##########
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:
Thanks, this is a fair concern and I agree the default should not change
behavior on upgrade. You are describing the current state accurately:
resolveAndValidateSqlQueryOptions throws for any key outside CONFIG_RESOLVER
plus a small hardcoded trace/database map, and CalciteSqlParser applies it
unconditionally on every DQL SET and legacy OPTION(...) path, with no config to
disable it and no way for plugins to register keys.
I will rework it as follows:
1. Default: unknown keys are preserved and propagated exactly as today, with
a warning log that includes the close-typo suggestion, so users still get
feedback without breakage.
2. Strict rejection becomes opt-in behind a broker config following the
existing toggle style, e.g. pinot.broker.enable.strict.sql.query.options
(default false), wired from the broker request handler since the parser has no
config access.
3. An extension hook (e.g. QueryOptionsUtils.registerAllowedOptionKeys) so
custom components can add their keys to the allowlist before enabling strict
mode.
That gives a staged rollout: warn by default now, and the default can be
revisited in a later release. If this direction works for you I will push the
change; happy to drop the registry piece if you prefer a smaller surface.
--
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]
