xiangfu0 commented on code in PR #19944:
URL: https://github.com/apache/pinot/pull/19944#discussion_r4343865889
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -726,12 +728,26 @@ public static ImmutableQueryEnvironment.Config.Builder
configBuilder() {
return ImmutableQueryEnvironment.Config.builder();
}
- public boolean useSpools(Map<String, String> options) {
+ /// Returns the spool mode of the query: the
[CommonConstants.Broker.Request.QueryOptionKey#USE_SPOOLS] query option
+ /// when it is set, or [Config#defaultSpoolMode()] otherwise.
+ ///
+ /// @throws QueryException with [QueryErrorCode#QUERY_VALIDATION] if the
query option is not a valid [SpoolMode].
+ public SpoolMode getSpoolMode(Map<String, String> options) {
String optionValue =
options.get(CommonConstants.Broker.Request.QueryOptionKey.USE_SPOOLS);
if (optionValue == null) {
- return _envConfig.defaultUseSpools();
+ return _envConfig.defaultSpoolMode();
}
- return Boolean.parseBoolean(optionValue);
+ try {
+ return SpoolMode.fromString(optionValue);
+ } catch (IllegalArgumentException e) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException(e.getMessage(), e);
+ }
+ }
+
+ /// Whether the response must describe the spool decisions of the planner.
See
+ /// [CommonConstants.Broker.Request.QueryOptionKey#SPOOLS_FEEDBACK].
+ public static boolean isSpoolsFeedback(Map<String, String> options) {
+ return
Boolean.parseBoolean(options.get(CommonConstants.Broker.Request.QueryOptionKey.SPOOLS_FEEDBACK));
Review Comment:
MINOR: `isSpoolsFeedback` uses `Boolean.parseBoolean`, which silently
returns false for anything that isn't "true". That's fine for a feedback
toggle, but it's worth noting it diverges from the strict `useSpools` parsing
right above, where an invalid value now fails the query. Not a blocker, just a
small inconsistency a user might trip on if they type e.g. `spoolsFeedback=1`.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/QueryEnvironment.java:
##########
@@ -726,12 +728,26 @@ public static ImmutableQueryEnvironment.Config.Builder
configBuilder() {
return ImmutableQueryEnvironment.Config.builder();
}
- public boolean useSpools(Map<String, String> options) {
+ /// Returns the spool mode of the query: the
[CommonConstants.Broker.Request.QueryOptionKey#USE_SPOOLS] query option
+ /// when it is set, or [Config#defaultSpoolMode()] otherwise.
+ ///
+ /// @throws QueryException with [QueryErrorCode#QUERY_VALIDATION] if the
query option is not a valid [SpoolMode].
+ public SpoolMode getSpoolMode(Map<String, String> options) {
String optionValue =
options.get(CommonConstants.Broker.Request.QueryOptionKey.USE_SPOOLS);
if (optionValue == null) {
- return _envConfig.defaultUseSpools();
+ return _envConfig.defaultSpoolMode();
}
- return Boolean.parseBoolean(optionValue);
+ try {
+ return SpoolMode.fromString(optionValue);
+ } catch (IllegalArgumentException e) {
+ throw QueryErrorCode.QUERY_VALIDATION.asException(e.getMessage(), e);
+ }
+ }
+
+ /// Whether the response must describe the spool decisions of the planner.
See
+ /// [CommonConstants.Broker.Request.QueryOptionKey#SPOOLS_FEEDBACK].
+ public static boolean isSpoolsFeedback(Map<String, String> options) {
+ return
Boolean.parseBoolean(options.get(CommonConstants.Broker.Request.QueryOptionKey.SPOOLS_FEEDBACK));
Review Comment:
MINOR: `isSpoolsFeedback` uses `Boolean.parseBoolean`, which silently
returns false for anything that isn't "true". That's fine for a feedback
toggle, but it's worth noting it diverges from the strict `useSpools` parsing
right above, where an invalid value now fails the query. Not a blocker, just a
small inconsistency a user might trip on if they type e.g. `spoolsFeedback=1`.
--
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]
