Clojure

inconsistent numeric comparison semantics between BigDecimal and other Numerics

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.4, Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

BigDecimal does not have consistent comparison semantics with other numeric types.

user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false                  <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
user> (== 1.0M 1.00M)
false    ;; potentially surprising

Patch: clj-1118-v7.patch

Approach: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The javadoc for these methods explicitly states that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

Screened by: Alex Miller

Activity

Hide
Arthur Ulfeldt added a comment -

I understand that the definition of equality between bigDecimals is dependent on both value and scale as in this case:

user> (== 0.000000M 0.0M)
false

I just want to make sure the decission to propagate that semantic across types is intentional. If this is on purpose than this is not a bug.

Show
Arthur Ulfeldt added a comment - I understand that the definition of equality between bigDecimals is dependent on both value and scale as in this case: user> (== 0.000000M 0.0M) false I just want to make sure the decission to propagate that semantic across types is intentional. If this is on purpose than this is not a bug.
Hide
Arthur Ulfeldt added a comment - - edited

this could be fixed by calling stripTrailingZeros on bigDecimals before comparing them to Longs or BigInts.

(== 2 (double (. 2.0M stripTrailingZeros)))
true

Edited by Andy Fingerhut: Unfortunately that fails for BigDecimal values equal to 0, unless they happen to have a scale that matches what you are comparing it to.

I think a more complete solution is to use BigDecimal's compareTo method, e.g.:

(zero? (.compareTo 2.0M (bigdec 2)))
true

Show
Arthur Ulfeldt added a comment - - edited this could be fixed by calling stripTrailingZeros on bigDecimals before comparing them to Longs or BigInts. (== 2 (double (. 2.0M stripTrailingZeros))) true Edited by Andy Fingerhut: Unfortunately that fails for BigDecimal values equal to 0, unless they happen to have a scale that matches what you are comparing it to. I think a more complete solution is to use BigDecimal's compareTo method, e.g.: (zero? (.compareTo 2.0M (bigdec 2))) true
Hide
Timothy Baldridge added a comment -

It seems we need some more eyes on this issue, can you bring this up on clojure-dev and see what they think?

Show
Timothy Baldridge added a comment - It seems we need some more eyes on this issue, can you bring this up on clojure-dev and see what they think?
Hide
Andy Fingerhut added a comment -

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt dated Apr 14 2013 changes equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal.

They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

I'm not sure if this is the preferred behavior for Clojure, but if it is, this patch should do it.

Show
Andy Fingerhut added a comment - Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt dated Apr 14 2013 changes equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0. The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals. I'm not sure if this is the preferred behavior for Clojure, but if it is, this patch should do it.
Andy Fingerhut made changes -
Field Original Value New Value
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt [ 11957 ]
Hide
Andy Fingerhut added a comment -

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt dated Apr 14 2013 is same as clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt described in previous comment, except it also has some new tests included.

Show
Andy Fingerhut added a comment - clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt dated Apr 14 2013 is same as clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt described in previous comment, except it also has some new tests included.
Andy Fingerhut made changes -
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt [ 11959 ]
Andy Fingerhut made changes -
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt [ 11957 ]
Hide
Andy Fingerhut added a comment -

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt dated Apr 15 2013 is the same as the the previous patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt, except for the following:

By changing == behavior for BigDecimal by modifying the BigDecimalOps.equiv() method, that also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return same value for all numerically equal BigDecimal values. This patch should achieve that.

Show
Andy Fingerhut added a comment - clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt dated Apr 15 2013 is the same as the the previous patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt, except for the following: By changing == behavior for BigDecimal by modifying the BigDecimalOps.equiv() method, that also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return same value for all numerically equal BigDecimal values. This patch should achieve that.
Andy Fingerhut made changes -
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt [ 11960 ]
Andy Fingerhut made changes -
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt [ 11959 ]
Andy Fingerhut made changes -
Patch Code and Test [ 10002 ]
Hide
Andy Fingerhut added a comment -

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt dated Apr 15 2013 is simply updating the stale -v3.txt patch. The only reason it no longer applied cleanly with git is because a few lines of context changed in the test code.

Show
Andy Fingerhut added a comment - Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt dated Apr 15 2013 is simply updating the stale -v3.txt patch. The only reason it no longer applied cleanly with git is because a few lines of context changed in the test code.
Andy Fingerhut made changes -
Andy Fingerhut made changes -
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt [ 11960 ]
Alex Miller made changes -
Description user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true


It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?
{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?
Labels math
Andy Fingerhut made changes -
Description {code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?
{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?

Part of a message about this issue from the following thread is copied below: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1403d494668229d8

It'd be great if this got fixed - we met an ugly bug yesterday due to this on our project. Our system validates that the sum of two monetary fields A and B equals the sum of two monetary fields C and D. We parse the fields via Cheshire with conversion to BigdDecimal turned on - but any fields with no fractional part get parsed as Integers anyway. So if A=$1.50, B=$1.50, C=$2 and D=$1, (== (+ A B) (+ C D)) is false.
Alex Miller made changes -
Approval Triaged [ 10120 ]
Rich Hickey made changes -
Approval Triaged [ 10120 ] Vetted [ 10003 ]
Fix Version/s Release 1.6 [ 10157 ]
Andy Fingerhut made changes -
Description {code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?

Part of a message about this issue from the following thread is copied below: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1403d494668229d8

It'd be great if this got fixed - we met an ugly bug yesterday due to this on our project. Our system validates that the sum of two monetary fields A and B equals the sum of two monetary fields C and D. We parse the fields via Cheshire with conversion to BigdDecimal turned on - but any fields with no fractional part get parsed as Integers anyway. So if A=$1.50, B=$1.50, C=$2 and D=$1, (== (+ A B) (+ C D)) is false.
{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?

Part of a message about this issue from the following thread is copied below: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1403d494668229d8

It'd be great if this got fixed - we met an ugly bug yesterday due to this on our project. Our system validates that the sum of two monetary fields A and B equals the sum of two monetary fields C and D. We parse the fields via Cheshire with conversion to BigdDecimal turned on - but any fields with no fractional part get parsed as Integers anyway. So if A=$1.50, B=$1.50, C=$2 and D=$1, (== (+ A B) (+ C D)) is false.

*Patch*: clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt

*Approach*: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing.
Alex Miller made changes -
Assignee Alex Miller [ alexmiller ]
Hide
Andy Fingerhut added a comment -

Quick comment that some of the tests in patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt were written before CLJ-1036 was declined as out of scope for Clojure, and should probably be cut down a bit, at least to eliminate biginteger and float occurrences. I will wait and see if there are any other comments from screening before creating any new patches.

Show
Andy Fingerhut added a comment - Quick comment that some of the tests in patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt were written before CLJ-1036 was declined as out of scope for Clojure, and should probably be cut down a bit, at least to eliminate biginteger and float occurrences. I will wait and see if there are any other comments from screening before creating any new patches.
Hide
Alex Miller added a comment -

Andy, the tests in the patch don't actually pass for me?

Many failures in the generative tests, but for example:

[java] {:clojure.test/vars (equality-tests),
     [java]  :thread/name "main",
     [java]  :pid 5719,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :message
     [java]  "Test that 0 (class java.lang.Byte) #'clojure.core/== 0.0 (class java.math.BigDecimal)",
     [java]  :level :warn,
     [java]  :test/actual false,
     [java]  :test/expected (equal-var val1 val2),
     [java]  :line 58,
     [java]  :tstamp 1380298603830,
     [java]  :file "numbers.clj"}
Show
Alex Miller added a comment - Andy, the tests in the patch don't actually pass for me? Many failures in the generative tests, but for example:
[java] {:clojure.test/vars (equality-tests),
     [java]  :thread/name "main",
     [java]  :pid 5719,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :message
     [java]  "Test that 0 (class java.lang.Byte) #'clojure.core/== 0.0 (class java.math.BigDecimal)",
     [java]  :level :warn,
     [java]  :test/actual false,
     [java]  :test/expected (equal-var val1 val2),
     [java]  :line 58,
     [java]  :tstamp 1380298603830,
     [java]  :file "numbers.clj"}
Alex Miller made changes -
Approval Vetted [ 10003 ] Incomplete [ 10006 ]
Alex Miller made changes -
Assignee Alex Miller [ alexmiller ]
Hide
Andy Fingerhut added a comment -

Could you tell me the OS, version, and JDK version, and the command you used to run this test? The reason I ask is that this patch has been tested on 3 OS/JDK version combos via my prescreen testing, using 'ant', and it passes on all of them, so if it fails on some OS/JDK I am not testing I would like to figure out why.

Show
Andy Fingerhut added a comment - Could you tell me the OS, version, and JDK version, and the command you used to run this test? The reason I ask is that this patch has been tested on 3 OS/JDK version combos via my prescreen testing, using 'ant', and it passes on all of them, so if it fails on some OS/JDK I am not testing I would like to figure out why.
Hide
Alex Miller added a comment -

Sorry - user error. They're passing.

Show
Alex Miller added a comment - Sorry - user error. They're passing.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Vetted [ 10003 ]
Assignee Alex Miller [ alexmiller ]
Hide
Alex Miller added a comment -

Slightly tweaked patch to remove some of the float/double stuff that I'm not expecting to change.

Marking screened...

Show
Alex Miller added a comment - Slightly tweaked patch to remove some of the float/double stuff that I'm not expecting to change. Marking screened...
Alex Miller made changes -
Approval Vetted [ 10003 ] Screened [ 10004 ]
Description {code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 0, :qualifier "beta1"}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

It's not clear if this is a bug or an enhancement request, Should BigDecimal's be special in comparason to their smaller equivalents?

Part of a message about this issue from the following thread is copied below: https://mail.google.com/mail/u/0/?shva=1#label/clojure/1403d494668229d8

It'd be great if this got fixed - we met an ugly bug yesterday due to this on our project. Our system validates that the sum of two monetary fields A and B equals the sum of two monetary fields C and D. We parse the fields via Cheshire with conversion to BigdDecimal turned on - but any fields with no fractional part get parsed as Integers anyway. So if A=$1.50, B=$1.50, C=$2 and D=$1, (== (+ A B) (+ C D)) is false.

*Patch*: clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt

*Approach*: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing.
BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing.

*Screened by:* Alex Miller
Attachment clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt [ 12283 ]
Hide
Andy Fingerhut added a comment -

Patch clj-1118-v6.txt is tiny modification of Alex Miller's clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

It removes hash consistency tests for floats and BigIntegers, with a comment explaining why they should not be there.

Show
Andy Fingerhut added a comment - Patch clj-1118-v6.txt is tiny modification of Alex Miller's clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt It removes hash consistency tests for floats and BigIntegers, with a comment explaining why they should not be there.
Andy Fingerhut made changes -
Attachment clj-1118-v6.txt [ 12284 ]
Hide
Rich Hickey added a comment -

Please don't use .txt as an extension for diffs, it keeps them from loading with appropriate mode in various editors.

v5 was screened but v6 alters it?

Show
Rich Hickey added a comment - Please don't use .txt as an extension for diffs, it keeps them from loading with appropriate mode in various editors. v5 was screened but v6 alters it?
Rich Hickey made changes -
Approval Screened [ 10004 ] Incomplete [ 10006 ]
Hide
Alex Miller added a comment -

Andy's last patch just added a comment. I updated a new version that is identical to v6 but has a .patch extension instead. Marking Screened again.

Show
Alex Miller added a comment - Andy's last patch just added a comment. I updated a new version that is identical to v6 but has a .patch extension instead. Marking Screened again.
Alex Miller made changes -
Approval Incomplete [ 10006 ] Screened [ 10004 ]
Description BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing.

*Screened by:* Alex Miller
BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-v7.patch

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing.

*Screened by:* Alex Miller
Attachment clj-1118-v7.patch [ 12357 ]
Hide
Andy Fingerhut added a comment -

It was not only adding a comment, but removing tests that should not have been there and could potentially mislead people into thinking that Clojure guarantees hash being consistent with = for BigInteger and Float/Double, which by CLJ-1036 it does not.

Regarding the .txt file extensions, first I've heard of the preference. I can make sure future screened tickets don't have this, but changing currently screened tickets would likely lead to confusion about which ones are screened. M-x diff-mode in emacs will force the mode regardless of file name.

Show
Andy Fingerhut added a comment - It was not only adding a comment, but removing tests that should not have been there and could potentially mislead people into thinking that Clojure guarantees hash being consistent with = for BigInteger and Float/Double, which by CLJ-1036 it does not. Regarding the .txt file extensions, first I've heard of the preference. I can make sure future screened tickets don't have this, but changing currently screened tickets would likely lead to confusion about which ones are screened. M-x diff-mode in emacs will force the mode regardless of file name.
Hide
Alex Miller added a comment -

Oh right, that is ok so can stay screened.

Show
Alex Miller added a comment - Oh right, that is ok so can stay screened.
Alex Miller made changes -
Assignee Alex Miller [ alexmiller ]
Alex Miller made changes -
Description BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-v7.patch

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing.

*Screened by:* Alex Miller
BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-v7.patch

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

*Screened by:* Alex Miller
Alex Miller made changes -
Description BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
{code}

*Patch*: clj-1118-v7.patch

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

*Screened by:* Alex Miller
BigDecimal does not have consistent comparison semantics with other numeric types.

{code}
user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
user> (== 1.0M 1.00M)
false ;; potentially surprising
{code}

*Patch*: clj-1118-v7.patch

*Approach*: Change equiv for BigDecimals so that instead of using {{BigDecimal.equals()}}, it uses {{BigDecimal.compareTo()}} and checks the return value is equal to 0.

The javadoc for these methods explicitly states that {{BigDecimal.equals()}} will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that {{BigDecimal.compareTo()}} will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies {{hasheq()}} to return the same value for all numerically equal BigDecimal values, by calling {{BigDecimal.stripTrailingZeros()}} on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

*Screened by:* Alex Miller
Rich Hickey made changes -
Approval Screened [ 10004 ] Ok [ 10007 ]
Stuart Halloway made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Closed [ 6 ]

People

Vote (3)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: