Fix support for widening on equals operator#4617
Conversation
only the equals operator in Expression Language was not supporting mixing integer and float. If the type does not match we are widening to BigDecimal for comparison
shatzi
left a comment
shatzi
left a comment
There was a problem hiding this comment.
Great progress. Dobule.compare have some side effects that I believe need to be refined.
| Arguments.of(new NumericValue(1), new NumericValue(2), LT, true, "1 < 2"), | ||
| Arguments.of(new NumericValue(2), new NumericValue(1), LT, false, "2 < 1"), | ||
| Arguments.of(new NumericValue(1.0), new NumericValue(1.1), LT, true, "1.0 < 1.1"), | ||
| Arguments.of(new NumericValue(1), new NumericValue(1.1), LT, true, "1 < 1.1"), |
There was a problem hiding this comment.
might worth also check the false state?
| Arguments.of(new NumericValue(1), new NumericValue(0.9), GE, true, "1 >= 0.9"), | ||
| Arguments.of(ValueExpression.NULL, new NumericValue(2), GE, false, "null >= 2"), | ||
| Arguments.of( | ||
| new NumericValue(Double.NaN), new NumericValue(Double.NaN), GE, true, "NaN >= NaN"), |
There was a problem hiding this comment.
why this is true? I thought NaN is not equal to anything, include NaN. or JVM treat NaN differently?
There was a problem hiding this comment.
found that Double.compare treat NaN differently.
Which is interesting as person.height == NaN would return false even if person.height is NaN. but our DSL return true - which make more sense for me.
However, Dobule.compare(NaN, any-other-number) == 1 (always greater) - which I find wrong; DSL of person.height > 10 will return true when height is NaN - not what I would expect.
Maybe worth refine those details and ensure our DSL return what is expected.
There was a problem hiding this comment.
well using Double.compare with NaN is not great because you need a result that is either 0, -1 or 1 while boolean comparison could be false which is another kind of result.
so we need to take care specially for NaN, I agree
| Arguments.of(new NumericValue(1L), new NumericValue(1L), EQ, true, "1 == 1"), | ||
| Arguments.of(new NumericValue(1.0F), new NumericValue(1.0F), EQ, true, "1.0 == 1.0"), | ||
| Arguments.of(new NumericValue(1.0), new NumericValue(1.0), EQ, true, "1.0 == 1.0"), | ||
| Arguments.of(new NumericValue(1), new NumericValue(1.0), EQ, true, "1 == 1.0"), |
There was a problem hiding this comment.
should we also have 1 == 2.0 is false?
add more tests
c1ac517 to
f5952cf
Compare
2 participants
What Does This Do
only the equals operator in Expression Language was not supporting mixing integer and float. If the type does not match we are widening to BigDecimal for comparison
Motivation
Additional Notes