FrankChen021 commented on code in PR #20629:
URL: https://github.com/apache/druid/pull/20629#discussion_r4314849739
##########
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:
This is not new to this PR. `maxParseExceptions` has no range on master, so
a plain `-1000` is already accepted there and fails later in the same way, in
MSQ's `FaultsExceededChecker`. `'-1K'` is just another way to write it.
The fix is a constraint that allows `-1` or any value of `0` or more. I'll
leave it for a follow-up that reviews the ranges of all numeric parameters.
##########
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:
Fixed in eee0486968 with `Math.multiplyExact`. Added tests for the
wrap-around case (`18014398509481985KiB` used to return `1024`) and for the
`8191PiB`/`8192PiB` boundary.
--
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]
