Long, BigInteger, BigDecimal Operators Fail Due to Conversion to Double

[codesplode]

Problem

We came across an issue today and believe I have figured out why it occurs. When using pebble with id values that are longs we noticed odd behavior where pebble considered the values 325055142682428416 and 325055142682428417 to be equal. You can recreate this issue with any large longs.

After reviewing the code we realized that all number comparisons in Pebble are converted to Double values which we believe is the issue. Please see the following lines for reference:

The line that calls wideningConversionBinaryComparison from a comparison where the type is Number:

pebble/pebble/src/main/java/com/mitchellbosecke/pebble/utils/OperatorUtils.java

Line 69 in aa43888

return wideningConversionBinaryComparison(op1, op2, Comparison.EQUALS);

The line in wideningConversionBinaryComparison that actually performs the double comparison:

pebble/pebble/src/main/java/com/mitchellbosecke/pebble/utils/OperatorUtils.java

Line 191 in aa43888

return doubleComparison(num1.doubleValue(), num2.doubleValue(), comparison);

Edit : Same Problem Affects All Binary Operators

This line shows another use of doubleValue() in OperatorUtils.wideningConversionBinaryOperation:

pebble/pebble/src/main/java/com/mitchellbosecke/pebble/utils/OperatorUtils.java

Line 154 in aa43888

return bigDecimalOperation(BigDecimal.valueOf(num1.doubleValue()),

Solution

I would recommend adding a type check for Number to also see if the instance also implements Comparable as this should work for many numerics reliably without conversion. This would also give you support for BigDecimal and BigInteger which we also use a lot for their reliability in financial math. The downside of Comparable is it can be tricky if you have to handle the case of comparing two numbers of different types, where both implement Comparable<T> of their own type, but a class cast exception would be thrown with double.compareTo(long) for example.

Here to assist as needed if we can contribute, but this is obviously a sensitive low level operator for Pebble, so requires your input on implementation I believe. 👍

[codesplode]

I have created a branch with the equals operator corrected, though it could definitely use further cleanup since it was done quickly this morning. After seeing that all Binary operators have the same issue, I'm tempted to merge the comparison operators with the type specific operation functions, such as bigDecimalOperation etc.

Where your input would be very important is compatibility of these. It's ideal to implement this for operands of the same type (op1.getClass().equals(op2.getClass())), but currently if the operands are not the same type, the conversions would work in some cases (with compatible numbers for doubleValue()) and fail silently when the numbers overflow. I would prefer these cases throw an exception instead of dangerously resulting in incorrect values that could cause any number of effects in production code.

Worst case, if the operands are of different type, we could at least try to convert them to BigDecimal or do additional logic to use the more complex of the two values.