Abacn commented on code in PR #24447:
URL: https://github.com/apache/beam/pull/24447#discussion_r1037409787
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/TableRowToStorageApiProto.java:
##########
@@ -681,7 +681,7 @@ private static void fieldDescriptorFromTableField(
new BigDecimal((String) value));
} else if (value instanceof BigDecimal) {
return
BigDecimalByteStringEncoder.encodeToBigNumericByteString(((BigDecimal) value));
- } else if (value instanceof Double || value instanceof Float) {
Review Comment:
Sorry, I meant the conversion of Long to double value
`BigDecimal.valueOf(((Number) value).doubleValue())` could lose precision here.
Long has max 2^63-1 up to 18 digit of significance, however Double has 16 digit
of significance. A safe way is something like this
```
else if (value instanceof Double || value instanceof Float) {
return BigDecimalByteStringEncoder.encodeToNumericByteString(
BigDecimal.valueOf(((Number) value).doubleValue()));
}
ekse if (value instanceof java.lang.Short || value instanceof
java.lang.Integer || value instanceof java.lang.Long) {
return BigDecimalByteStringEncoder.encodeToNumericByteString(
BigDecimal.valueOf(((Number) value).longValue()));
}
```
We may also want to add test to check Long value of 18 digits do not lose
precision. This use case is possible as raw timestamp can be backed by a Long.
my original comment about the unsupported Number refers to some known direct
subclasses of Number here:
https://docs.oracle.com/javase/8/docs/api/java/lang/Number.html
--
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]
