Jackeyzhe commented on code in PR #27877:
URL: https://github.com/apache/flink/pull/27877#discussion_r3442569242
##########
flink-core/src/main/java/org/apache/flink/util/TimeUtils.java:
##########
@@ -79,30 +82,45 @@ public static Duration parseDuration(String text) {
throw new NumberFormatException("text does not start with a
number");
}
- final long value;
+ final BigInteger value;
try {
- value = Long.parseLong(number); // this throws a
NumberFormatException on overflow
+ value = new BigInteger(number); // this throws a
NumberFormatException
} catch (NumberFormatException e) {
throw new IllegalArgumentException(
- "The value '"
- + number
- + "' cannot be re represented as 64bit number
(numeric overflow).");
+ "The value '" + number + "' cannot be represented as an
integer number.", e);
}
+ final ChronoUnit unit;
if (unitLabel.isEmpty()) {
- return Duration.of(value, ChronoUnit.MILLIS);
- }
-
- ChronoUnit unit = LABEL_TO_UNIT_MAP.get(unitLabel);
- if (unit != null) {
- return Duration.of(value, unit);
+ unit = ChronoUnit.MILLIS;
} else {
+ unit = LABEL_TO_UNIT_MAP.get(unitLabel);
+ }
+ if (unit == null) {
throw new IllegalArgumentException(
"Time interval unit label '"
+ unitLabel
+ "' does not match any of the recognized units: "
+ TimeUnit.getAllUnits());
}
+
+ try {
+ return convertBigIntToDuration(value, unit);
+ } catch (ArithmeticException e) {
Review Comment:
nit: The new `ArithmeticException` catch seems to be exercised indirectly by
`testParseDurationNumberOverflow`, but that test only checks for
`IllegalArgumentException`. Could we strengthen it to assert the overflow
message or `ArithmeticException` cause, so the new wrapping behavior is covered
explicitly?
--
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]
