FrankChen021 commented on code in PR #20629:
URL: https://github.com/apache/druid/pull/20629#discussion_r4313933771
##########
processing/src/main/java/org/apache/druid/query/context/QueryContextParameter.java:
##########
@@ -338,16 +339,36 @@ static <T> T exactNumber(
{
final String expectedFormat = "in " + typeName + " format";
final BigDecimal decimal;
- try {
- if (value instanceof BigDecimal) {
- decimal = (BigDecimal) value;
- } else if (value instanceof Number || value instanceof String) {
+ if (value instanceof BigDecimal) {
+ decimal = (BigDecimal) value;
+ } else if (value instanceof Number) {
+ try {
decimal = new BigDecimal(value.toString());
- } else {
+ }
+ catch (NumberFormatException e) {
+ // Such as NaN or infinity
throw QueryContextParameters.invalidValueException(name,
expectedFormat, value);
}
- }
- catch (NumberFormatException e) {
+ } else if (value instanceof String) {
+ // A plain number, or a human-readable number with a unit suffix, such
as 10K or 256MiB
+ BigDecimal parsed;
+ try {
+ parsed = new BigDecimal((String) value);
+ }
+ catch (NumberFormatException e) {
+ try {
+ parsed = BigDecimal.valueOf(HumanReadableBytes.parse((String) value,
true));
Review Comment:
[P2] Reject unsupported negative maxParseExceptions values
**Finding:** The new human-readable fallback is applied to every long
parameter, so maxParseExceptions='-1K' is now parsed as -1000. That parameter
only supports -1 as its negative sentinel; its existing downstream
FaultsExceededChecker rejects other negative values with
Preconditions.checkArgument. The query therefore reaches MSQ controller setup
and fails as a generic runtime fault instead of being rejected as invalid query
context input.
**Suggestion:** Add a parameter-specific constraint or parser for
MAX_PARSE_EXCEPTIONS that permits only -1 or nonnegative values, and cover
suffixed negative input in the MSQ tests.
##########
processing/src/main/java/org/apache/druid/java/util/common/HumanReadableBytes.java:
##########
@@ -204,15 +230,17 @@ private static long parseInner(String rawNumber)
}
try {
- long value = Long.parseLong(number.substring(0, lastDigitIndex + 1)) *
base;
+ // Spaces are allowed between the number and the unit, such as "1 MiB"
+ final String digits = number.substring(start, lastDigitIndex + 1);
+ long value = Long.parseLong(base > 1 ? digits.stripTrailing() : digits)
* base;
Review Comment:
[P2] Human-readable unit multiplication can silently wrap
**Finding:** The parser multiplies the coefficient by the unit base in
signed long arithmetic, then only rejects results below the base. An
out-of-range coefficient can therefore wrap to a positive value at least as
large as the base; for example, 18014398509481985KiB wraps to 1024 and is
accepted. The new query-context and subquery-guardrail parsing paths can
consequently turn an out-of-range value into a small valid limit instead of
rejecting it.
**Suggestion:** Use overflow-checked signed multiplication before applying
the sign, and add boundary and multi-wrap tests for positive and negative
values.
--
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]
