Bug Hunting Session
Bug 97587 - Software interpreter for SUM treats empty cells as #VALUE!
Summary: Software interpreter for SUM treats empty cells as #VALUE!
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0 target:5.1.1
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-04 20:47 UTC by Tor Lillqvist
Modified: 2016-10-25 19:03 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Lillqvist 2016-02-04 20:47:43 UTC
(This bug report is mostly pro forma, in developer-speak, for people familiar with the code.)

When using the software interpreter for formula groups (and not OpenCL), empty cells are mishandled by the SUM function as implemented by ArraySumFunctor. Empty cells are represented in the mpNumericArray as NaN values (without any payload, i.e. not error codes). These NaN then take part in the summation (either using SSE2 or plain C++ code), and the NaN propagates to the sum, as it should. Until then in ScInterpreter::TreatDoubleError() and the GetDoubleErrorValue() it calls the NaN gets translated into errNoValue, and displayed as #VALUE!

This bug is not directly related to other serious issues in the software interpreter, see bug #97369.
Comment 1 Tor Lillqvist 2016-02-04 20:59:35 UTC
One way to fix this would be to go through the values to be summed in the ArraySumFunctor actor, check if each is a NaN, and change it to a 0. But I wonder if that extra code will then compensate any performance gain from using this code in the first place. Another possibility would be to make sure that if we know that it is the software interpreter and not OpenCL that will be used, then not store NaNs in the array, but zeros.
Comment 2 Tor Lillqvist 2016-02-04 20:59:55 UTC
s/actor/ctor
Comment 3 Tor Lillqvist 2016-02-04 21:20:36 UTC
If I only could find the place in the code where the NaNs are stored in the array. It isn't ToDoubleArray::operator() in scmatrix.cxx, as I would have assumed. Sigh.
Comment 4 Tor Lillqvist 2016-02-05 07:53:22 UTC
Sigh, and turning the NaNs into zeros in the ArraySumFunctor constructor isn't easy either because of constness: the mpNumericArray member of VectorRefArray is a pointer to const doubles. This is getting ridiculous.
Comment 5 Tor Lillqvist 2016-02-05 11:22:16 UTC
OK, the NaNs come from the code in column2.cxx that calls rtl::math::setNan() to get a NaN and then stores that in some array.
Comment 6 Tor Lillqvist 2016-02-05 13:20:39 UTC
https://gerrit.libreoffice.org/#/c/22152/
Comment 7 Commit Notification 2016-02-08 06:40:11 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4d67506dee347c3a3a290e1f52af91048c4318bc

tdf#97587: Treat plain NaNs as zero in the software interpreter for SUM

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 8 Commit Notification 2016-02-08 10:15:36 UTC
Tor Lillqvist committed a patch related to this issue.
It has been pushed to "libreoffice-5-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=aa064e8209e57ac91f16305d38e657d12a42093f&h=libreoffice-5-1

tdf#97587: Treat plain NaNs as zero in the software interpreter for SUM

It will be available in 5.1.1.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 9 Cor Nouws 2016-02-22 13:50:43 UTC
@Tor: shouldn't this one be marked as fixed, similar as bug 97369?
Comment 10 Commit Notification 2016-02-27 11:53:36 UTC
Marco Cecchetti committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=65694793e9588106e570d82b359c9c9e25a5cf0d

sc - unit tests for tdf#97369 and tdf#97587

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 11 Adolfo Jayme 2016-03-01 03:19:30 UTC
Also, http://cgit.freedesktop.org/libreoffice/core/commit/?id=cb01ea920550769e8a04b3cb809c70ed324ba342

tdf#97369/#97587 - Further fix SUMming in the software interpreter
Comment 12 Michael Meeks 2016-06-30 11:04:12 UTC
Right - lets mark this fixed =)