Cole-Greer commented on code in PR #3153:
URL: https://github.com/apache/tinkerpop/pull/3153#discussion_r2317038257


##########
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/NumberHelper.java:
##########
@@ -687,26 +689,69 @@ public static Integer compare(final Number a, final 
Number b) {
      * @throws IllegalArgumentException if the specified numeric type is 
unsupported
      */
     public static Number coerceTo(final Number a, final Class<? extends 
Number> clazz) {
+        try {
+            return performConversion(a, clazz);
+        } catch (ArithmeticException e) {
+            // return as-is since it didn't fit the type we wanted to coerce to
+            return a;
+        }
+    }
+
+    /**
+     * Casts the given number to the specified numeric type if it can fit into 
it.
+     * Otherwise, throw.
+     *
+     * @param a the number to be cast
+     * @param numberToken the number token denoting the desired type to cast
+     * @return the number cast to the specified type
+     * @throws IllegalArgumentException if the specified numeric type is 
unsupported
+     * @throws ArithmeticException if the number overflows
+     */
+    public static Number castTo(final Number a, final N numberToken) {
+        Class<? extends Number> clazz = numberToken.getType();
+        return performConversion(a, clazz);
+    }
+
+    /**
+     * Core conversion logic.
+     * Throws ArithmeticException when conversion would overflow.
+     */
+    private static Number performConversion(final Number a, final Class<? 
extends Number> clazz) {
         if (a.getClass().equals(clazz)) {
             return a;
-        } else if (clazz.equals(Integer.class)) {
-            if (a.longValue() >= Integer.MIN_VALUE && a.longValue() <= 
Integer.MAX_VALUE) {
+        }
+        if (clazz.equals(Integer.class)) {
+            Long val = longValue(a, clazz);
+            if (val >= Integer.MIN_VALUE && val <= Integer.MAX_VALUE) {
                 return a.intValue();
             }
         } else if (clazz.equals(Long.class)) {
-            return a.longValue();
+            return longValue(a, clazz);
         } else if (clazz.equals(Float.class)) {
+            // BigDecimal or BigInteger to double can overflow into Infinity, 
we want to prevent this
+            if (isInfinityOrNaN(a)) {
+                return a.floatValue();
+            }
             if (a.doubleValue() >= -Float.MAX_VALUE && a.doubleValue() <= 
Float.MAX_VALUE) {
                 return a.floatValue();
             }

Review Comment:
   I find it a bit odd that we throw exceptions for out of range float 
conversions instead of letting it go to `(float) Infinity`. I don't want to 
block this work if I'm the only one who feels this way (as this is a small edge 
case), but I believe pushing to infinity is more aligned with IEEE 754.



-- 
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]

Reply via email to