Copilot commented on code in PR #19027:
URL: https://github.com/apache/pinot/pull/19027#discussion_r3762695292
##########
pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java:
##########
@@ -381,6 +381,14 @@ public static class Broker {
"pinot.broker.query.log.sqlRedaction";
public static final String DEFAULT_BROKER_QUERY_LOG_SQL_REDACTION = "none";
public static final String CONFIG_OF_BROKER_QUERY_ENABLE_NULL_HANDLING =
"pinot.broker.query.enable.null.handling";
+ /// How query option keys supplied through SQL `SET` / `OPTION(...)` on
DQL queries are validated.
+ /// One of `QueryOptionsUtils.SqlQueryOptionValidationMode`: `NONE`
(default, unknown keys are
+ /// preserved silently, as they always have been), `WARN` (preserved,
logged once per distinct
+ /// unknown key) or `REJECT` (query fails). Plugins can allowlist their
own keys for `REJECT` via
+ /// `QueryOptionsUtils.registerSqlQueryOptionKey`.
+ public static final String CONFIG_OF_BROKER_QUERY_OPTION_VALIDATION_MODE =
+ "pinot.broker.query.sql.queryOptionValidationMode";
+ public static final String DEFAULT_BROKER_QUERY_OPTION_VALIDATION_MODE =
"NONE";
Review Comment:
The default `NONE` leaves every broker preserving unknown SQL keys, so this
PR does not actually fix the linked issue or match its stated SQL-DQL rejection
behavior unless operators discover and opt into the new setting. Since
validation is already scoped away from REST/JSON and DML, make `REJECT` the
default; otherwise the existing silent-ignore foot-gun remains after upgrade.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -75,6 +115,9 @@ private QueryOptionsUtils() {
: new RuntimeException("Failure to build case insensitive mapping.",
classLoadError);
}
+ /// Resolves known option keys case-insensitively to their canonical names.
Unknown keys are
+ /// preserved unchanged, including their key case. Used for every option
source: SQL, REST/JSON
+ /// `queryOptions` and broker-injected options, none of which reject unknown
keys here.
Review Comment:
`trace` and `database` are accepted case-insensitively but are absent from
`CONFIG_RESOLVER`, so this resolver preserves spellings such as `TRACE` and
`Database`. Exact-key consumers then miss them (`DatabaseUtils` reads
`database`; tracing reads `trace`), silently ignoring an option that validation
accepted. Canonicalize the additional allowlisted keys here as well, and update
the new test to assert lowercase keys.
--
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]
