Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-6386

compareBigDecimals in org.ofbiz.minilang.method.conditional.Compare does not compare certain values correctly

    Details

      Description

      Moving the conversation from OFBIZ-6291 to this issue.

      compareBigDecimals scales down and rounds up meaning you lose information and the comparison result is not as expected

      1. compareBigDecimals.png
        22 kB
        Gareth Carter
      2. v1.patch
        6 kB
        Gareth Carter
      3. v2.patch
        1.0 kB
        Gareth Carter
      4. OFBIZ-6386_1783753.patch
        6 kB
        Gareth Carter

        Activity

        Hide
        gareth.carter Gareth Carter added a comment -

        compareBigDecimals.png shows the output of 5 different methods. The first table shows the result from the code in ofbiz - basically it treats all those values as equal to each other

        The second table shows a slightly modified version where the BigDecimals are scaled and rounded up (eg 2.01 will become 2.010). The result for this scenario is the same as BigDecimal.compareTo where compareTo method ignores scale

        The 4th table (BigDecimal.equals) is just there as a comparison and proves the result is false because the scale is different for each value

        The 5th table, I have added only because <if-compare and <if-compare-field in minilang default to converting objects to their string representation before performing comparison (String.compareTo). The result is slightly different because compareTo compares lexicographically, 2.010 and 2.01 are the same numerically (ignoring scale), "2.010" and "2.01" are lexicographically different

        Show
        gareth.carter Gareth Carter added a comment - compareBigDecimals.png shows the output of 5 different methods. The first table shows the result from the code in ofbiz - basically it treats all those values as equal to each other The second table shows a slightly modified version where the BigDecimals are scaled and rounded up (eg 2.01 will become 2.010). The result for this scenario is the same as BigDecimal.compareTo where compareTo method ignores scale The 4th table (BigDecimal.equals) is just there as a comparison and proves the result is false because the scale is different for each value The 5th table, I have added only because <if-compare and <if-compare-field in minilang default to converting objects to their string representation before performing comparison (String.compareTo). The result is slightly different because compareTo compares lexicographically, 2.010 and 2.01 are the same numerically (ignoring scale), "2.010" and "2.01" are lexicographically different
        Hide
        gareth.carter Gareth Carter added a comment -

        Also, the method has a comment

        " // Developers: Do not change this. We are comparing two fixed-point decimal numbers,
        // not performing accounting calculations - so it is okay to specify the rounding mode."

        But searching ofbiz for <if-compare and <if-compare-field in minilang, you will find them in accounting component...

        Show
        gareth.carter Gareth Carter added a comment - Also, the method has a comment " // Developers: Do not change this. We are comparing two fixed-point decimal numbers, // not performing accounting calculations - so it is okay to specify the rounding mode." But searching ofbiz for <if-compare and <if-compare-field in minilang, you will find them in accounting component...
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        It doesn't matter where you find the comparisons, they are still making a comparison and NOT performing a calculation.

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - It doesn't matter where you find the comparisons, they are still making a comparison and NOT performing a calculation.
        Hide
        adrianc@hlmksw.com Adrian Crum added a comment -

        Do you have a patch for this issue?

        Show
        adrianc@hlmksw.com Adrian Crum added a comment - Do you have a patch for this issue?
        Hide
        gareth.carter Gareth Carter added a comment -

        It may not perform calculations directly but it is a control statement in minilang, therefore calculations (or other code) may occur that were not expected.

        I only analysed ofbiz for usage not impact. I noticed the accounting component used <if-compare and <if-compare-field with type="BigDecimal" and with accounting you generally want accuracy in terms of numbers, BigDecimal offers this
        but, to me compareBigDecimals does not. You lose information on comparison.

        Java code in ofbiz generally uses compareTo for comparison without modifying scale.

        Groovy code is the same as java (using compareTo), groovy also supports operator overloading which will use compareTo for relational operator overloading (eg, a == b)

        The compareBigDecimals method used in minilang behaves slightly different to those comparisons done in java or groovy but only for values like those in compareBigDecimals.png

        I will provide a patch for review as soon as I can

        Show
        gareth.carter Gareth Carter added a comment - It may not perform calculations directly but it is a control statement in minilang, therefore calculations (or other code) may occur that were not expected. I only analysed ofbiz for usage not impact. I noticed the accounting component used <if-compare and <if-compare-field with type="BigDecimal" and with accounting you generally want accuracy in terms of numbers, BigDecimal offers this but, to me compareBigDecimals does not. You lose information on comparison. Java code in ofbiz generally uses compareTo for comparison without modifying scale. Groovy code is the same as java (using compareTo), groovy also supports operator overloading which will use compareTo for relational operator overloading (eg, a == b) The compareBigDecimals method used in minilang behaves slightly different to those comparisons done in java or groovy but only for values like those in compareBigDecimals.png I will provide a patch for review as soon as I can
        Hide
        gareth.carter Gareth Carter added a comment -

        v1.patch removes the compareBigDecimals code entirely

        v2.patch updates the compareBigDecimals method to up the scale so its a smaller patch with minimal code change.

        Personally, I would use v1.patch and remove the code. compareBigDecimals calls BigDecimal.compareTo but it modifies the value before calling compareTo

        Show
        gareth.carter Gareth Carter added a comment - v1.patch removes the compareBigDecimals code entirely v2.patch updates the compareBigDecimals method to up the scale so its a smaller patch with minimal code change. Personally, I would use v1.patch and remove the code. compareBigDecimals calls BigDecimal.compareTo but it modifies the value before calling compareTo
        Hide
        mridul.pathak Mridul Pathak added a comment -

        I have reviewed both the patches and gone through whole conversation. Gareth's observations seems correct. "compareBigDecimal" method seems to be performing unnecessary rounding before comparison. BigDecimal.compareTo() already takes care of variable scale while comparing, so I don't think we need to explicitly set scale using rounding before comparison. I think it should be ok to commit v1.patch.

        Looking forward to opinion from other community members.

        Show
        mridul.pathak Mridul Pathak added a comment - I have reviewed both the patches and gone through whole conversation. Gareth's observations seems correct. "compareBigDecimal" method seems to be performing unnecessary rounding before comparison. BigDecimal.compareTo() already takes care of variable scale while comparing, so I don't think we need to explicitly set scale using rounding before comparison. I think it should be ok to commit v1.patch. Looking forward to opinion from other community members.
        Hide
        jacopoc Jacopo Cappellato added a comment -

        I have reviewed the comments, the code and the patches provided by Gareth Carter and I agree with him and with Mridul Pathak that we should commit the first patch.

        Show
        jacopoc Jacopo Cappellato added a comment - I have reviewed the comments, the code and the patches provided by Gareth Carter and I agree with him and with Mridul Pathak that we should commit the first patch.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I tend to agree with all of you, but what will happen to the 161 lines of code with
        <if-compare field*"BigDecimal"
        Disclaimer: It's late and I did not get further to see if removing has an impact or not. I guess there is one, else those removed lines in Compare.java would not make sense in 1st place.

        Show
        jacques.le.roux Jacques Le Roux added a comment - I tend to agree with all of you, but what will happen to the 161 lines of code with <if-compare field*"BigDecimal" Disclaimer: It's late and I did not get further to see if removing has an impact or not. I guess there is one, else those removed lines in Compare.java would not make sense in 1st place.
        Hide
        mridul.pathak Mridul Pathak added a comment -

        Hi Jacques Le Roux
        That should still work as per the written code through BigDecimal.compareTo() implementation of Comparable. compareBigDecimal() method was doing the same but with additional unnecessary rounding.

        Show
        mridul.pathak Mridul Pathak added a comment - Hi Jacques Le Roux That should still work as per the written code through BigDecimal.compareTo() implementation of Comparable. compareBigDecimal() method was doing the same but with additional unnecessary rounding.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Mridul,

        I was thinking "why Adrian complicated those things will always remain a mystery" then I got to OFBIZ-6291 by chance. After reading https://issues.apache.org/jira/browse/OFBIZ-6291?focusedCommentId=14510685, quoted here for convenience

        Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000.

        If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break.

        If you need to compare BigDecimal precision, then you can use a scriptlet: groovy:fooBigDecimal.equals(barBigDecimal).

        and reviewing the v1.patch I agree we should apply and cross fingers that Adrian was wrong about

        If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break.

        I guess he crossed an issue, at least once, and that's why he put this piece of code. Also maybe he did not see the issues Gareth raised and focused on the "2 == 2.0 == 2.000000." aspect. I hope it's the second but I doubt, anyway should not be a big deal

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Mridul, I was thinking "why Adrian complicated those things will always remain a mystery" then I got to OFBIZ-6291 by chance. After reading https://issues.apache.org/jira/browse/OFBIZ-6291?focusedCommentId=14510685 , quoted here for convenience Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000. If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break. If you need to compare BigDecimal precision, then you can use a scriptlet: groovy:fooBigDecimal.equals(barBigDecimal). and reviewing the v1.patch I agree we should apply and cross fingers that Adrian was wrong about If we were to compare BigDecimal precision, then we lose the scripting abstraction, PLUS pretty much everything that uses Mini-language will break. I guess he crossed an issue, at least once, and that's why he put this piece of code. Also maybe he did not see the issues Gareth raised and focused on the "2 == 2.0 == 2.000000." aspect. I hope it's the second but I doubt, anyway should not be a big deal
        Hide
        jacopoc Jacopo Cappellato added a comment -

        Thanks Jacques Le Roux for the research you did; however I don't think that Adrian's comment is relevant for this case.
        After the patchv1 we will still have 2 == 2.0 == 2.00.
        With the added benefit that 2.01 will be different from 2.001 which is not the case with the current implementation.

        Show
        jacopoc Jacopo Cappellato added a comment - Thanks Jacques Le Roux for the research you did; however I don't think that Adrian's comment is relevant for this case. After the patchv1 we will still have 2 == 2.0 == 2.00. With the added benefit that 2.01 will be different from 2.001 which is not the case with the current implementation.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        I agree, so I think he was mistaken and that's all

        Show
        jacques.le.roux Jacques Le Roux added a comment - I agree, so I think he was mistaken and that's all
        Hide
        gareth.carter Gareth Carter added a comment -

        Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000.

        is not quite true in the my opinion. In minilang, field type conversion and comparison defaults to string representation unless you explicitly define the type, string comparison and numerical comparison are different. When it comes to comparing BigDecimals, information is lost (ie scale is changed, lowered) so values that are not the same can be interpreted to be the same (check attached image). Example, 2.01 == 2.001 when using <if-compare, value 2.001 is scaled down and rounded up yielding result 2.01.

        The patch does not compare precision but if the minilang does then yes it will be an issue. But I suspect 99% cases, do not care about it

        Show
        gareth.carter Gareth Carter added a comment - Mini-language is a scripting language. One of the nice things about scripting languages is you can compare things like decimal values without worrying about the underlying implementation. That is what the current implementation does: 2 == 2.0 == 2.000000. is not quite true in the my opinion. In minilang, field type conversion and comparison defaults to string representation unless you explicitly define the type, string comparison and numerical comparison are different. When it comes to comparing BigDecimals, information is lost (ie scale is changed, lowered) so values that are not the same can be interpreted to be the same (check attached image). Example, 2.01 == 2.001 when using <if-compare, value 2.001 is scaled down and rounded up yielding result 2.01. The patch does not compare precision but if the minilang does then yes it will be an issue. But I suspect 99% cases, do not care about it
        Hide
        gareth.carter Gareth Carter added a comment -

        Jacopo beat me to it!

        Show
        gareth.carter Gareth Carter added a comment - Jacopo beat me to it!
        Hide
        mridul.pathak Mridul Pathak added a comment -

        Thanks Jacques for the detailed input. What I see from Java API documentation, it seems to me that this particular abstraction won't be compromised if we apply this patch. It states that BigDecimal.compareTo() method treats two objects with different scale as equal (2.0 == 2.00), while BigDecimal.equals() do not. I am not sure why Adrian quoted that but seems like we shouldn't be facing any issue, and as you said, crossing my fingers already .

        Here is the link to java docs https://docs.oracle.com/javase/8/docs/api/java/math/BigDecimal.html#compareTo-java.math.BigDecimal-

        Show
        mridul.pathak Mridul Pathak added a comment - Thanks Jacques for the detailed input. What I see from Java API documentation, it seems to me that this particular abstraction won't be compromised if we apply this patch. It states that BigDecimal.compareTo() method treats two objects with different scale as equal (2.0 == 2.00), while BigDecimal.equals() do not. I am not sure why Adrian quoted that but seems like we shouldn't be facing any issue, and as you said, crossing my fingers already . Here is the link to java docs https://docs.oracle.com/javase/8/docs/api/java/math/BigDecimal.html#compareTo-java.math.BigDecimal-
        Hide
        mridul.pathak Mridul Pathak added a comment -

        Jacopo and Gareth both beat me to it . I should refresh page more often .

        Show
        mridul.pathak Mridul Pathak added a comment - Jacopo and Gareth both beat me to it . I should refresh page more often .
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Yes I checked the doc also, I often get caught by Jira not auto-refreshing

        Show
        jacques.le.roux Jacques Le Roux added a comment - Yes I checked the doc also, I often get caught by Jira not auto-refreshing
        Hide
        gareth.carter Gareth Carter added a comment -

        Updated patch (file location changed)

        Show
        gareth.carter Gareth Carter added a comment - Updated patch (file location changed)
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Gareth for you patch, Mridul and Jacopo for discussions,

        Fixed in
        trunk r1786079
        R16.11 r1786080
        R15.12 r1786082

        I did not backport further because of too much conflicts

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Gareth for you patch, Mridul and Jacopo for discussions, Fixed in trunk r1786079 R16.11 r1786080 R15.12 r1786082 I did not backport further because of too much conflicts
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Unfirtunately this breaks tests
        auto-invoice-tests.testGlPostingOnCancelCheckRun
        auto-payment-tests.testGlPostingOnCheckRun
        see https://ci.apache.org/projects/ofbiz/logs/15.12/html/

        Show
        jacques.le.roux Jacques Le Roux added a comment - Unfirtunately this breaks tests auto-invoice-tests.testGlPostingOnCancelCheckRun auto-payment-tests.testGlPostingOnCheckRun see https://ci.apache.org/projects/ofbiz/logs/15.12/html/
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        This is due to expressions like this

        <set field="totalPayableDebitAmount" value="${payableDebitTotal + 36.43}" type="BigDecimal"/>
        

        This kind of changes fixes it (local BigDecimal)

        Index: applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml
        ===================================================================
        --- applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml	(revision 1786324)
        +++ applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml	(working copy)
        @@ -295,9 +295,11 @@
                 <first-from-list list="paymentGroupMemberAndTransList" entry="paymentGroupMemberAndTrans"/>
                 <if-compare field="paymentGroupMemberAndTrans.finAccountTransStatusId" operator="not-equals" value="FINACT_TRNS_APPROVED">
         
        -        <set field="totalPayableDebitAmount" value="${payableDebitTotal + 82.86}" type="BigDecimal"/>
        -        <set field="totalPayableCreditAmount" value="${payableCreditTotal + 165.72}" type="BigDecimal"/>
        -        <set field="totalPayableDebitCreditDifference" value="${payableDebitCreditDifference - 82.86}" type="BigDecimal"/>
        +        <set field="tempBig1" value="82.86" type="BigDecimal"/>
        +        <set field="totalPayableDebitAmount" value="${payableDebitTotal + tempBig1}" type="BigDecimal"/>
        +        <set field="totalPayableCreditAmount" value="165.72" type="BigDecimal"/>
        +        <set field="totalPayableCreditAmount" value="${payableCreditTotal + totalPayableCreditAmount}" type="BigDecimal"/>
        +        <set field="totalPayableDebitCreditDifference" value="${payableDebitCreditDifference - tempBig1}" type="BigDecimal"/>
                 <set field="getAcctgTransEntriesAndTransTotalMap.glAccountId" value="210000"/>
                 <call-service service-name="getAcctgTransEntriesAndTransTotal" in-map-name="getAcctgTransEntriesAndTransTotalMap">
                     <result-to-field result-name="debitTotal" field="payableDebitTotal"/>
        @@ -311,8 +313,8 @@
                     <if-compare-field field="totalPayableDebitCreditDifference" operator="equals" to-field="payableDebitCreditDifference" type="BigDecimal"/>
                 </assert>
                 <check-errors/>
        -        <set field="totalUndepositedDebitAmount" value="${undepositedDebitTotal + 82.86}" type="BigDecimal"/>
        -        <set field="totalUndepositedDebitCreditDifference" value="${undepositedDebitCreditDifference + 82.86}" type="BigDecimal"/>
        +        <set field="totalUndepositedDebitAmount" value="${undepositedDebitTotal + tempBig1}" type="BigDecimal"/>
        +        <set field="totalUndepositedDebitCreditDifference" value="${undepositedDebitCreditDifference + tempBig1}" type="BigDecimal"/>
                 <set field="getAcctgTransEntriesAndTransTotalMap.glAccountId" value="111100"/>
                 <call-service service-name="getAcctgTransEntriesAndTransTotal" in-map-name="getAcctgTransEntriesAndTransTotalMap">
                     <result-to-field result-name="debitTotal" field="undepositedDebitTotal"/>
        

        I'll commit a fix soon

        Show
        jacques.le.roux Jacques Le Roux added a comment - This is due to expressions like this <set field= "totalPayableDebitAmount" value= "${payableDebitTotal + 36.43}" type= "BigDecimal" /> This kind of changes fixes it (local BigDecimal) Index: applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml =================================================================== --- applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml (revision 1786324) +++ applications/accounting/script/org/ofbiz/accounting/test/AutoInvoiceTests.xml (working copy) @@ -295,9 +295,11 @@ <first-from-list list= "paymentGroupMemberAndTransList" entry= "paymentGroupMemberAndTrans" /> < if -compare field= "paymentGroupMemberAndTrans.finAccountTransStatusId" operator = "not-equals" value= "FINACT_TRNS_APPROVED" > - <set field= "totalPayableDebitAmount" value= "${payableDebitTotal + 82.86}" type= "BigDecimal" /> - <set field= "totalPayableCreditAmount" value= "${payableCreditTotal + 165.72}" type= "BigDecimal" /> - <set field= "totalPayableDebitCreditDifference" value= "${payableDebitCreditDifference - 82.86}" type= "BigDecimal" /> + <set field= "tempBig1" value= "82.86" type= "BigDecimal" /> + <set field= "totalPayableDebitAmount" value= "${payableDebitTotal + tempBig1}" type= "BigDecimal" /> + <set field= "totalPayableCreditAmount" value= "165.72" type= "BigDecimal" /> + <set field= "totalPayableCreditAmount" value= "${payableCreditTotal + totalPayableCreditAmount}" type= "BigDecimal" /> + <set field= "totalPayableDebitCreditDifference" value= "${payableDebitCreditDifference - tempBig1}" type= "BigDecimal" /> <set field= "getAcctgTransEntriesAndTransTotalMap.glAccountId" value= "210000" /> <call-service service-name= "getAcctgTransEntriesAndTransTotal" in-map-name= "getAcctgTransEntriesAndTransTotalMap" > <result-to-field result-name= "debitTotal" field= "payableDebitTotal" /> @@ -311,8 +313,8 @@ < if -compare-field field= "totalPayableDebitCreditDifference" operator = "equals" to-field= "payableDebitCreditDifference" type= "BigDecimal" /> </ assert > <check-errors/> - <set field= "totalUndepositedDebitAmount" value= "${undepositedDebitTotal + 82.86}" type= "BigDecimal" /> - <set field= "totalUndepositedDebitCreditDifference" value= "${undepositedDebitCreditDifference + 82.86}" type= "BigDecimal" /> + <set field= "totalUndepositedDebitAmount" value= "${undepositedDebitTotal + tempBig1}" type= "BigDecimal" /> + <set field= "totalUndepositedDebitCreditDifference" value= "${undepositedDebitCreditDifference + tempBig1}" type= "BigDecimal" /> <set field= "getAcctgTransEntriesAndTransTotalMap.glAccountId" value= "111100" /> <call-service service-name= "getAcctgTransEntriesAndTransTotal" in-map-name= "getAcctgTransEntriesAndTransTotalMap" > <result-to-field result-name= "debitTotal" field= "undepositedDebitTotal" /> I'll commit a fix soon
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Fixed at r1786332 (all branches)

        Show
        jacques.le.roux Jacques Le Roux added a comment - Fixed at r1786332 (all branches)

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            gareth.carter Gareth Carter
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development