Cole-Greer commented on code in PR #3340:
URL: https://github.com/apache/tinkerpop/pull/3340#discussion_r2985037878
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/binary/internals/NumberSerializationStrategy.js:
##########
@@ -58,11 +58,10 @@ export default class NumberSerializationStrategy {
// INT32_MIN/MAX
return this.ioc.intSerializer.serialize(item, fullyQualifiedFormat);
}
- // eslint-disable-next-line no-loss-of-precision
- if (item >= -9223372036854775808 && item < 9223372036854775807) {
- // INT64_MIN/MAX
+ if (item >= Number.MIN_SAFE_INTEGER && item <= Number.MAX_SAFE_INTEGER) {
return this.ioc.longSerializer.serialize(item, fullyQualifiedFormat);
}
+ // Integers outside safe range are huge doubles that only appear
integral due to IEEE 754 precision loss
return this.ioc.doubleSerializer.serialize(item, fullyQualifiedFormat);
}
Review Comment:
There's an asymmetry between the type matching logic here and that in
GremlinLang. GremlinLang uses the switchover to scientific notation on string
representations as the cutoff from long to double, where this code uses
Number.MAX_SAFE_INTEGER as the threshold.
I'm not sure I necessarily favour either system as "more correct", but I
think they should be consistent. I might slightly prefer the "safe integer"
bounding used here simply because it is easier to document and communicate, any
thoughts?
--
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]
