Bug 54686 - [PATCH] Some Excel formats are formatted incorrectly by DataFormatter
Summary: [PATCH] Some Excel formats are formatted incorrectly by DataFormatter
Status: NEEDINFO
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.8-FINAL
Hardware: PC All
: P2 major (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2013-03-13 12:30 UTC by abramova151288
Modified: 2015-10-25 21:50 UTC (History)
1 user (show)



Attachments
DataFormatter changes (3.92 KB, patch)
2013-03-13 12:30 UTC, abramova151288
Details | Diff
3/4's of a patch, might be good enough (61.63 KB, patch)
2013-06-13 19:45 UTC, Tim Allison
Details | Diff
patch for fraction format (61.15 KB, patch)
2013-06-13 23:53 UTC, Tim Allison
Details | Diff
Fraction cells in 2003 format (17.00 KB, application/vnd.ms-excel)
2013-10-28 13:08 UTC, Chris Lott
Details

Note You need to log in before you can comment on or make changes to this bug.
Description abramova151288 2013-03-13 12:30:34 UTC
Created attachment 30044 [details]
DataFormatter changes

Some Excel formats are displayed incorrect in the application (fractional, accounting and currency formats).

For example, this formats displays incorrect:

Category - Fraction:
1/4   (Up to one digit (1/4) cell type)    --> 0/
21/25 (Up to two digits (21/25) cell type) --> 1/
1/2   (As halves (1/2) cell type)          --> 0/2
2/4   (As quarters (2/4) cell type)        --> 0/4

Category - Accounting:
0,001000000   (Decimal places - 9, Symbol - р.)               --> 0,00
$ 1,234567890 (Decimal places - 9, Symbol - $English(Canada)) --> 1,23

Category - Currency:
0,001000000   (Decimal places - 9, Symbol - р.)               --> 0,00
$ 1,234567890 (Decimal places - 9, Symbol - $English(Canada)) --> 1,23
Comment 1 Nick Burch 2013-03-13 13:15:30 UTC
Are you able to work up a quick unit test to go with this? Just something that tries to format a couple of different numbers with each of the formats. That way, we'll know it's fixed, and we'll also be able to make sure it doesn't get broken in future!
Comment 2 Tim Allison 2013-06-13 19:45:14 UTC
Created attachment 30430 [details]
3/4's of a patch, might be good enough

Thanks to Ryan Krueger and https://issues.apache.org/jira/browse/TIKA-1132 for the custom formatting test case on fractions.  I added some test cases, and offer a 3/4's patch.  This patch passes 3148 tests and fails on 38.  Without the patch, there are 3062 failurs and 124 successes.

I refactored the fraction calculator to avoid the n^2 calculations that could lead to 100,000,000 calculations if the format was ##/####.  By "refactored," I mean copied and pasted from org.apache.math.fraction.
Comment 3 Nick Burch 2013-06-13 21:31:47 UTC
We debated adding a dependency on commons math, but decided against it. Copying their code is probably more sensible, though someone came up with a contribution which we accepted in the mean time...

Hopefully someone with slightly stronger math skills can offer an opinion on your suggested fix!
Comment 4 Tim Allison 2013-06-13 23:53:47 UTC
Created attachment 30431 [details]
patch for fraction format

Completely agree.  Should have stepped away from the computer before submitting the last.  This passes all tests.  Still only handles fraction formatting.
Comment 5 Nick Burch 2013-06-20 13:38:17 UTC
Thanks for this patch Tim!

For the unit test, I renamed it to TestFractionFormat + changed package, and updated how it gets the test text file to be consistent with the xls, as well as re-enabling the single test

One thing I've just found - inner class Fraction of CellNumberFormat - seems to do something similar. Any chance you could cast an eye over it, and decide if we can rationalise / refactor to just have the one?

Patch applied (with tweaks described above) in r1494986, thanks
Comment 6 Tim Allison 2013-06-20 15:08:50 UTC
(In reply to Nick Burch from comment #5)
> Thanks for this patch Tim!
> 
> For the unit test, I renamed it to TestFractionFormat + changed package, and
> updated how it gets the test text file to be consistent with the xls, as
> well as re-enabling the single test
> 
> One thing I've just found - inner class Fraction of CellNumberFormat - seems
> to do something similar. Any chance you could cast an eye over it, and
> decide if we can rationalise / refactor to just have the one?
> 
> Patch applied (with tweaks described above) in r1494986, thanks

Thank you, Nick.  Will do.
Comment 7 Chris Lott 2013-10-28 13:08:37 UTC
Created attachment 30981 [details]
Fraction cells in 2003 format

Adding an excel sheet suitable for use as test of fraction formatting, HTH
Comment 8 Tim Allison 2013-10-28 18:27:02 UTC
Thank you, Chris.  POI-55419 covers the examples you offered.  Fraction formatting is still not perfect...there are some rounding differences btwn Java and MSExcel...but I think the fraction component of this issue should be closed.  

I haven't had a chance to look into the currency and accounting items that are raised by this issue.  If you have a need and POI is failing for you in those areas, please submit a file that shows those problems.

Thank you, again!
Comment 9 Nick Burch 2014-07-30 16:16:42 UTC
Just in case it isn't clear - we're still waiting on some new tests (unit test + sample file) that shows up the last remaining parts of this issue around currenct / accounting formatting

With those, it should be possible to investigate what logic changes are still needed, then work on those.
Comment 10 Nick Burch 2015-10-25 21:50:37 UTC
The currency / accounting pieces may have been solved by the work for #58536, or at least partially, would be great if someone could check and write a unit test for any parts that remain outstanding!