-
Notifications
You must be signed in to change notification settings - Fork 394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decimal data type controls are not printing correctly - the data is getting cut off and rounded. #1089
Comments
Write me. I added an email to my profile. |
After debugging your example I draw this conclusion: |
Thanks for looking into that. In real life example we ran across this issue with total (aggregate) controls. And the totals (both table bindings and controls) are also defined as Decimal data type. Probably, the internal scripting is also doing double conversion in between and then BIRT classes convert the end result into BigDecimal. |
After "reading up" on numbers in javascript they seem to be double precision floats, which means that the Rhino implementation is correct. |
If you are calculating invoices, it is a mistake to use That's the reason why most programming languages (including ancient Cobol) also support data types like For example, take 31/10 = 3.1 (exactly). BTW: I debugged a simple report and can see that - unfortunately - BIRT converts excerpt from TotalSum.java:
excerpt from NumberCalculator.java:
In DataTypeUtil.java, the toDouble method calls an overloaded method of the same name.
which calls
To summarize (pun intended :-), BIRT will convert a Probably later this Thus in some cases you will face a small difference if you compare BIRT's output to what you get out if you very carefully compute everything manually in the decimal system. I suggest - if your data is from an SQL database - that you perform the calculations on the SQL side. |
Well, I understand the floating math and get it that in a lot of cases it is no good for financial calculations. But that is why you have Decimal (BigDecimal) data type. My understanding was that if you use Decimal control in BIRT it should display the number as it is w/o any internal conversion to double and then back to BigDecimal. That seems to be not the case and as the result you end with the wrong number. Each time decimal values are converted to double before any calculation and formatting the number may lose the precision aspect of the Decimal data type (which is really what Decimal type is designed for). We occasionally run across such issues in BIRT reports though we are using only Decimal data type for DataSource/DataSet columns, aggregates/totals and report controls. |
I wish the final conclusion of that discussion would be true... I agree this is a bug which is still not fixed.
To be precise: AFAIK it is not scripting done in Rhino which is causing this error. It's caused by the Java implementation in the source files I mentioned (and other source files for other aggregate functions).
As this is caused on the Java side, it should be possible to fix it (in the abovementioned source files). But I doubt that anyone will fix it (unless you fix it yourself by providing a PR), because it would be quite an effort: I think it would be necessary to support all the calculations in two different ways: Furthermore, if you actually do any financial computations in Javascript (not BIRT aggregates), you're probably doomed: A workaround which avoids most of these problems is to use CT (instead of EUR or USD) as your basis (see http://adripofjavascript.com/blog/drips/avoiding-problems-with-decimal-math-in-javascript.html). |
As a side note, the Classic Models example database built into BIRT is a classic example for wrong database model design: |
I looked at this again, and the original problem happens because the script is using a javascript number literal which cannot "translate" to a corresponding double value. If the one would need an exact value the script needs to create a BigDecimal instead. Javascript does not have a corresponding type so the way to do that is: With that said: There are bugs or at least lack of function in BIRTs calculations. These are highlighted by @hvbtup in one of the comments. The aggregator functions of BIRT does not use BigDecimal where appropriate. The aggregator functions (AggrFunction) that use decimals all has "DataType.DOUBLE_TYPE" set as datatype. This will select a the "NumberCalculator" and that calculator always converts numbers to doubles in calculations. There are currently no way for the report designer to define what precision that should be used. It is possible to define the datatype for a "computed column" in a dataset, but that information is not taken into account (nor available) when the ICalculator, that will perform the calculations, is selected. The datatype only affects the resulting datatype. This means that all calculations will be done with doubles and then converted to the selected datatype. I see three different ways forward: a) Change the aggregator functions that needs higher then double precision to "Decimal", so a BigDecimalCalculator is selected for calculations during aggregation. To solve the @hvbtup if I were to try to "close" https://bugs.eclipse.org/bugs/show_bug.cgi?id=155138 again could you provide me with a test report that reproduces all the known BigDecimal problems? I am aware of the aggregation functions and the Javascript literals, but are there others? |
I won't be able to create such a test report in November. The topic of proper calculation is actually quite complex. I'll try to collect some brief notes here about what can go wrong:
As a result of what I said above, your wording "higher precision than double" is not correct. One would use float (or double) variables if the values are usually not exact anyway, e.g. they are measures (in the sense that someone has actually measured something, e.g. the alcohol contrentration of a liquor). But when the calculations are about offers/invoices/taxes/money, then we must use decimal datatypes and (usually) round-half-up from the start to the end. That means all intermediate results must be stored in decimal form, too. This "commercial" kind of calculation avoids all conversion errors, but it still incurs rounding errors. So it's not "higher precision than double", but instead a slightliy different way of performing the calculation to avoid conversion errors. |
I'm going to see what I can do with this. At least the part about getting it to use BigDecimalCalculator in aggregates. The OP's example involving constant values is going to be a different solution. |
It is possible to change the calculator in the onRow method, once it's known that the incoming data is a BigDecimal, however the return datatype of SUM would still be float and the user would need to know to change the datatype of the aggregate column binding from float to decimal. I wonder if it would be better to have a parallel set of aggregators specifically for decimal return values, such as DECIMAL_SUM. Maybe as an optional plugin. Additionally, there are some things the user would need to know to do for commercial/financial reporting, such as using new BigDecimal() for constant values and always using BirtMath for computations. These could be documented in the plugin GitHub page. I think BIRT should support financial reporting, even if it requires users to do things a bit differently. You can't necessarily rely on a SQL backend to do it for you. |
E.g. for SUM: Shouldn't it be possible to let the internal calculations use something like And perhaps we should give an opportunity to explicitly specify a rounding mode for the calculations. If not specified, it should be inferred from the data type, e.g. float -> ROUND_HALF_EVEN, decimal -> ROUND_HALF_UP. +1 for BIRT should support financial reporting. Anyway, we kind of hijacked this ticket. Maybe it makes sense to change the title. |
@hvbtup In the column binding, there isn't an explicit declaration of the data type of an argument. There's just an expression. And javascript being what it is, you can't really know the data type of the expression until you execute it. That's why it's possible to change the calculator in onRow but not before that. And at that point it's too late to change the return type of the binding (unless the user does it manually). Even if you had a type-agnostic calculator you'd still have the same problem with the return type. That's what led me to the idea of a set of decimal aggregate functions. It actually looks to be trivial to implement a decimal sum. Just extend TotalSum and override getName and getDataType. I'm not sure if it can be implemented as an external plugin though because of "Discouraged Access" warnings, but it could certainly be a built-in aggregator. As for rounding, I confess to not being very familiar with it so I'll do some research to see if what you are suggesting is possible. |
I've added DECIMALSUM and DECIMALAVG to our birt-functions-lib plugin project: https://github.com/innoventsolutions/birt-functions-lib/tree/birt-4.14 |
I've added 19 aggregations that use decimal calculations to the Innovent Solutions birt.functions.lib plugin. The site URL is the same one we've been using for several years: https://innoventsolutions.github.io/2.5.2. The aggregations are in a separate category so they are optional. The wiki URL is https://innoventsolutions.github.io. (I'll be adding wiki content soon). The repo URL is https://github.com/innoventsolutions/birt-aggregations-lib. I believe that using these aggregations and using BirtMath for calculations and new BigDecimal for constant values should allow on to produce financial reports devoid of floating point errors. If you get a chance to use them, let me know what you think. Please post issues to the repo if you have any problems. |
Create a simple data control of Decimal type in a report. Set control value to "1111111111111112.12". Print/preview report in Eclipse/View Report/Web Browser. See simple report design and report output attached.
Note that the data is getting cut and rounded. If I remove 1 digit and make precision = 17 instead of 18, then it prints correctly. Is there as limit on BigDecimal precision data type in BIRT?
decimal_not_printing.zip
The issue happens in old BIRT 4.9 (Java 8) and new BIRT 4.9 (Java 11).
If you run/print the same number in Java 8 or 11 it prints correctly:
BigDecimal d = new BigDecimal("1111111111111112.12");
System.out.println(d);
The text was updated successfully, but these errors were encountered: