Bug 89872 - EDITING: MEDIAN(IF in array formula returns #VALUE!
Summary: EDITING: MEDIAN(IF in array formula returns #VALUE!
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
(earliest affected) Master
Hardware: Other All
: high major
Assignee: Eike Rathke
Whiteboard: target:4.5.0 target:4.4.2
Keywords: regression
Depends on:
Reported: 2015-03-07 10:02 UTC by GerardF
Modified: 2016-11-13 19:09 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:
Regression By:

file with MEDIAN sample (9.43 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-03-07 10:02 UTC, GerardF
expanded file with MEDIAN sample (10.71 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-03-11 09:10 UTC, Winfried Donkers
expanded file with MEDIAN sample (corrected) (10.94 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-03-11 11:29 UTC, Winfried Donkers
expanded file with MEDIAN sample (14.64 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-04-21 08:47 UTC, Winfried Donkers

Note You need to log in before you can comment on or make changes to this bug.
Description GerardF 2015-03-07 10:02:08 UTC
Created attachment 113945 [details]
file with MEDIAN sample

MEDIAN combined with IF in array formula fails.
Example: [=MEDIAN(IF(range>0;range))} returns #VALUE!
other functions like MIN, MAX, AVERAGE are working fine.

with Version:
Build ID: e80d1cfcb75659eddfd7a29e45d9e6ee935668ac
TinderBox: Win-x86@39, Branch:master, Time: 2015-02-27_02:25:05

Works with
Comment 1 Julien Nabet 2015-03-07 12:34:14 UTC
On pc Debian x86-64 with master sources updated yesterday, I could reproduce this

Interestingly, if I replace "0" values by "1", the array formula works.
Comment 2 Julien Nabet 2015-03-07 18:49:50 UTC
Considering this bt:
#0  ScMatrixImpl::IsString (this=0x7071af0, nC=0, nR=3) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/scmatrix.cxx:679
#1  0x00002aaac9fd86da in ScMatrixImpl::IsString (this=0x7071af0, nIndex=3) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/scmatrix.cxx:669
#2  0x00002aaac9fdd216 in ScMatrix::IsString (this=0x729c030, nIndex=3) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/scmatrix.cxx:2319
#3  0x00002aaac9f54fd9 in ScInterpreter::GetNumberSequenceArray (this=0x7676da0, nParamCount=1 '\001', rArray=std::__debug::vector of length 3, capacity 10 = {...})
    at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/interpr3.cxx:3720
#4  0x00002aaac9f52989 in ScInterpreter::ScMedian (this=0x7676da0) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/interpr3.cxx:3350
#5  0x00002aaac9f6b58d in ScInterpreter::Interpret (this=0x7676da0) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/tool/interpr4.cxx:4158
#6  0x00002aaac9d108fe in ScFormulaCell::InterpretTail (this=0x2d41240, eTailParam=ScFormulaCell::SCITP_NORMAL)
    at /home/julien/compile-libreoffice/libreoffice/sc/source/core/data/formulacell.cxx:1736
#7  0x00002aaac9d0f6dc in ScFormulaCell::Interpret (this=0x2d41240) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/data/formulacell.cxx:1469
#8  0x00002aaac9d12eac in ScFormulaCell::MaybeInterpret (this=0x2d41240) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/data/formulacell.cxx:2354
#9  0x00002aaac9d13122 in ScFormulaCell::GetValue (this=0x2d41240) at /home/julien/compile-libreoffice/libreoffice/sc/source/core/data/formulacell.cxx:2417
#10 0x00002aaaca7608d8 in ScOutputData::FindChanged (this=0x7fffffff0c00) at /home/julien/compile-libreoffice/libreoffice/sc/source/ui/view/output.cxx:1933
#11 0x00002aaaca725b18 in ScGridWindow::UpdateFormulas (this=0x2f31c50) at /home/julien/compile-libreoffice/libreoffice/sc/source/ui/view/gridwin.cxx:4630
#12 0x00002aaaca7ddfb3 in ScTabView::UpdateFormulas (this=0x2f2bd58) at /home/julien/compile-libreoffice/libreoffice/sc/source/ui/view/tabview3.cxx:1921

It seems 0 is considered like empty which has for consequence to consider the value like a string:

    672 bool ScMatrixImpl::IsString( SCSIZE nC, SCSIZE nR ) const
    673 {
    674     ValidColRowReplicated( nC, nR );
    675     switch (maMat.get_type(nR, nC))
    676     {
    677         case mdds::mtm::element_empty:
    678         case mdds::mtm::element_string:
    679             return true;
    680         default:
    681             ;
    682     }
    683     return false;
    684 }
See http://opengrok.libreoffice.org/xref/core/sc/source/core/tool/scmatrix.cxx#672

I don't know if the problem is the fact that mdds indicates it's empty
   1437 template<typename _CellBlockFunc>
   1438 mtv::element_t multi_type_vector<_CellBlockFunc>::get_type(size_type pos) const
   1439 {
   1440     size_type start_row = 0;
   1441     size_type block_index = 0;
   1442     if (!get_block_position(pos, start_row, block_index))
   1443         detail::throw_block_position_not_found("multi_type_vector::get_type", __LINE__, pos, block_size(), size());
   1445     const block* blk = m_blocks[block_index];
   1446     if (!blk->mp_data)
   1447         return mtv::element_type_empty;
   1449     return mtv::get_block_type(*blk->mp_data);
   1450 }

Eike/Markus: thought you might be interested in this one.
Comment 3 Eike Rathke 2015-03-09 13:00:04 UTC
Tricky.. actually IF(range>0;range) in array context produces an array of logical values, with the special case here the FALSE-path being omitted. Asking only for ScMatrix::IsString() in ScInterpreter::GetNumberSequenceArray() is wrong as it doesn't catch the empty cell / empty path conditions. This seems to be a regression of 104a1e641554be2e789758ae67c0e24620df8035 in master and b9088afc9f4f93548c668d81dd55e9bb63f91d80 in 4-4, which introduced a special handling for the string case to fix bug 88547 that should not be triggered in this case.

@Winfried: thinking on this further, I reckon the change was wrong and should be reverted, sorry for accepting it first hand ;-)  The way how GetNumberSequenceArray() is used (and was implemented) it indeed should ignore all string and empty values like all spreadsheet functions do that expect a number sequence as parameter. Actually this may spoil all sorts of things.. Bug 88547 needs to be fixed differently. I'll revert the corresponding changes and reopen that bug.
Comment 5 Winfried Donkers 2015-03-11 09:09:56 UTC
(In reply to Eike Rathke from comment #4)
> Fixed on master with
> https://gerrit.libreoffice.org/gitweb?p=core.git;a=commit;
> h=18de9b1de1aa404b3ca12e2018e5e5e220dd9786 and on 4-4 with
> https://gerrit.libreoffice.org/gitweb?p=core.git;a=commit;
> h=3feb8d18cfc7620891976c7fe116988a57192b79

@Eike: Strange, I can't reproduce the problem with nor without commit 104a1e641554be2e789758ae67c0e24620df8035 in master (updated to commit 8332bc98fac35ada2337de032bebfdae82139882, with/without modifications to sc/source/core/too/interpr3.cxx).
The code for tdf 88547 is not called with MEDIAN(IF(...)), nor with MEDIAN(...)

I expanded the sample document with date ranges, which are handled by MEDIAN as well as by other functions, except for {DATE(...);...;DATE(...)}, but that has nothing to do with this bug report.

What am I missing here?
(Keeping status of bug report unchanged.)
Comment 6 Winfried Donkers 2015-03-11 09:10:48 UTC
Created attachment 114031 [details]
expanded file with MEDIAN sample
Comment 7 GerardF 2015-03-11 09:23:33 UTC
(In reply to Winfried Donkers from comment #6)
> Created attachment 114031 [details]
> expanded file with MEDIAN sample

Your sample do not contain Array formula.
Comment 8 Winfried Donkers 2015-03-11 11:29:31 UTC
Created attachment 114039 [details]
expanded file with MEDIAN sample (corrected)

gerardF thank you for pointing to my error, that explains part of my confusion.

This attachment should be better and will be useful for fixing bug 88547 without breaking this fixed bug.
Comment 9 Eike Rathke 2015-04-20 15:00:43 UTC
When creating such test documents, please add a column with the expected result of a formula, i.e. copy the calculated formula cell and paste as result value only, or enter the expected value manually if the current version doesn't calculate as expected, and then best also add another column where the actual formula result is compared to the fixed value, best done with rounding each value to 12 decimals to eliminated round-off errors between applications.
Without a fixed result value it is hard to determine whether the actual formula result is correct or not.
Comment 10 Winfried Donkers 2015-04-21 08:47:14 UTC
Created attachment 114983 [details]
expanded file with MEDIAN sample

(In reply to Eike Rathke from comment #9)

Improved test document.