manner commented on code in PR #29323:
URL: https://github.com/apache/flink/pull/29323#discussion_r4123174937
##########
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariantInternalBuilder.java:
##########
@@ -271,6 +276,33 @@ public void appendDecimal(BigDecimal d) {
}
}
+ // The variant spec requires a scale in [0, 38] and a precision of at most
38.
+ private static BigDecimal toVariantDecimal(BigDecimal d) {
+ BigDecimal result = d;
+ if (d.scale() < 0) {
+ // A non-zero value with a scale below -38 has more than 38 digits
after rescaling.
+ // Reject it upfront because setScale is expensive for huge
exponents like 1e999999999.
+ if (d.signum() != 0 && d.scale() < -MAX_DECIMAL16_PRECISION) {
+ throw decimalOutOfRange(d);
+ }
+ result = d.setScale(0);
+ }
+ if (result.scale() > MAX_DECIMAL16_PRECISION
+ || result.precision() > MAX_DECIMAL16_PRECISION) {
+ throw decimalOutOfRange(d);
+ }
Review Comment:
There are some edge cases where this can still happen. For example,
`12345e34` has scale -34 and precision 5. After rescaling to scale 0, it has a
precision of 39, which the variant doesn't support.
But we can rewrite the check as:
```java
if (d.signum() != 0 && (long) d.precision() - d.scale() >
MAX_DECIMAL16_PRECISION) {
throw decimalOutOfRange(d);
}
```
Then we don't need another check after rescaling.
--
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]
