Apache OpenOffice (AOO) Bugzilla – Issue 46681
ODFF: Repeat values of single array vector if used in second array dimension
Last modified: 2013-08-07 15:14:39 UTC
Following test case: A1: 1 B1: 1 C1: 2 A2: 2 B2: 1 C2: 2 A3: =SUM(IF(A1:A2<2;B1:C2;0)) Result is 1 (B1 only), but should be 3 (sum of B1:C1).
Excel replicates a single column or row vector to cover further columns or rows of a matrix in calculations. This is independent of conditional array calculation and also done for ordinary operators and functions. The behavior is currently not implemented in Calc.
Need more time to investigate feasibility, retargeting to OOo2.0.2
Adapting summary to reality.
*** Issue 54581 has been marked as a duplicate of this issue. ***
*** Issue 54885 has been marked as a duplicate of this issue. ***
Created attachment 31857 [details] more examples
This will not be a small change and should not be done for OOo2.0.2.
*** Issue 59973 has been marked as a duplicate of this issue. ***
*** Issue 63166 has been marked as a duplicate of this issue. ***
In the ODFF draft 28Dec07 page 42 is: 2.2.3.2) If the argument data is 1 column wide the value in the corresponding row is used to evaluate all columns in the result matrix. = {1|2} + {10;20|30;40} => {11;21|32;42} whereas Calc returns {11|32} Is this the same issue? Meaning it needs flagging for ODFF compliance?
Yes, this is about that ;) Adding ODFF to summary.
Created attachment 53407 [details] Changed a few functions, just for checking.
Hi Eike, I have a question about my system, but I'm not in office, so I can only ask you here. :) I'm thinking about switching my operating system to Linux, and change the milestone together. But there are so many Linux, I wonder which is better for our development, and which one are you using. And about ODFF, which milestone should I use? I hope I didn't bring you many trouble. :P Thank you! :) Yue
Strange request ;-) anyway, replied per mail.
Created attachment 54198 [details] patch 2.
Hi Eike, In patch 2, I made some change for IF(). Applied the dynamic way to decide dimensions. the biggest change is in class ScJumpMatrix, I don't know if such change is acceptable. It works well in IF() now. :) Please check this patch. Thanks. Yue
Hi Yue, The approach looks viable in general. Some comments: In real life (see remarks further down about copying methods), the implementation of SetNewResMat() in the final version should not be in the header file, but in a newly to be created jumpmatrix.cxx instead. Header files should contain only relatively short inline methods. The nested for() loops wouldn't necessarily be inlined by most compilers anyway. The method is also quite rarely called, so we wouldn't gain anything but increase compile time instead each time the header file is parsed. Instead of ScMatrix* pNewMat = new ScMatrix(... [...] ScMatrix* p = pMat; pMat = pNewMat; p->Delete(); use ScMatrixRef xNewMat = new ScMatrix(... [...] pMat = xNewMat; Reason is that ScMatrixRef does refcount the ScMatrix, the pMat member variable is also a ScMatrixRef, and by directly calling Delete() on the pointer you interfere with the refcounting mechanism and may actually doubly delete the matrix that was already released during the assignment to pMat. The copying code for (SCSIZE nC = 0; nC < nResMatCols; nC++) for (SCSIZE nR = 0; nR < nResMatRows; nR++) is error prone; in case we added more type functionality to ScMatrix it would had to be adapted and might easily get overlooked. It already omits the IsEmptyPath() case ;-) Better create a ScMatrix member method that copies the desired portion similar to ScMatrix::MatCopy(), which would be faster as well. If you also add a method ScMatrix::CloneAndExtend(nNewCols,nNewRows) or so, similar to ScMatrix::Clone(), most code of ScJumpMatrix::SetNewResMat() could be moved into these two methods, and SetNewResMat() could remain inline in the header file. Btw, last nitpick for now is a spelling error, Ajust should be written with a 'd': Adjust ;) Hope that helps. Eike
This may need more time than available until QA for 3.0 code freeze, retargeting to 3.1
Created attachment 54383 [details] patch 3.
Hi Eike, This is the third patch, including all the changes I made. And, so far, I found these affected functions: +, -, * , /, ^, &, =, <>, >, <, >=, <=, TRANSPOSE(), and IF(). Please check this patch. Thanks. PS: I have add 'd' to 'Ajust'. :P Yue
Created attachment 54412 [details] patch4. Filled the regions that remain after copying.
Created attachment 54878 [details] patch 5. :)
*** Issue 92402 has been marked as a duplicate of this issue. ***
Issue 92402 has another fine test case example attached, http://www.openoffice.org/nonav/issues/showattachment.cgi/55506/Issue%2092402.ods
Created attachment 55731 [details] patch 6, slightly changed from patch 5.
@lvyue: I'll attach a patch created against CWS odff05 based on DEV300_m29, please checkout CWS odff05 and continue further work based on the new patch. As discussed on IRC, please investigate interpreter methods whether special handling similar to MatAdd() and such is needed for other functions as well. Changes I did in the patch: In ScInterpreter::JumpMatrix() the conditions to check for replication were overcomplicated, at least in the case of svMatrix that was wrong and lead to access of invalid matrix positions. Btw, this was caught by an assertion about a matrix dimension error, I strongly suggest you use a non-product build to get assertions displayed, please invoke configure with --enable-dbgutil before a clean build from scratch. I changed the condition, for example, to if ((nCols <= nC && nCols != 1) || (nRows <= nR && nRows != 1)) to catch the case for NOTAVAILABLE, also changed errNoValue to NOTAVAILABLE for these cases to be consistent with MatRef() and Excel. Similar for svDoubleRef, also the original range must not be changed by using ScAddress& rAdr = aRange.aStart; rAdr.SetCol(...); see also the comments I added there. Introduced ScMatrix::ValidColRowOrReplicated(nCol,nRow) to replace (ValidColRow(nCol,nRow) || ValidColRowReplicated(nCol,nRow)) for better readability and less typing effort ;-) Simplified the conditionals in ValidColRowReplicated(), some checks were done twice. Same in ScJumpMatrix::GetJump(), also added cleanup and an assertion there for the case that invalid nCol/nRow was passed, otherwise arbitrary memory could had been accessed in pJump[]. In interpr5.cxx introduced lcl_GetMinExtent() to not repeat all the conditionals for nMinC=..., nMinR=... over and over again in MatAdd() and the like. Looking forward to your findings whether further changes are necessary in other interpreter functions. Thanks Eike
Created attachment 56049 [details] fixes, enhancements and comments
@lvyue: forgot to mention that I also negated the logic in ScInterpreter::ScMatRef() to simplify the condition for the PushNA().
Hi Eike, I have checked all functions in ScInterpreter that call Put... and Get... and I did not find a function that need to be changed.
Hi Yue, great! I think I did not mention it explicitly, but did you also check for places that use IsEmpty(), IsEmptyPath(), IsString(), IsValue() and such , without Get...() being near around? Anyway, I applied the latest revision of the patch in cws odff05: sc/inc/scmatrix.hxx 1.11.148.1 sc/source/core/inc/jumpmatrix.hxx 1.6.148.1 sc/source/core/tool/interpr1.cxx 1.60.54.1 sc/source/core/tool/interpr5.cxx 1.33.36.1 sc/source/core/tool/scmatrix.cxx 1.17.136.1 maybe this will need some further correction for new findings. Please cvs update your cws odff05 and base new patches on the update. Thanks Eike
Hi Eike, yes, I checked those Is...(). since they are changed, I thought they should also be checked. don't worry. :) Yue
FYI: cleaned up naming, meaning and usage of ScMatrix::Is...Type(ScMatValType) sc/inc/scmatrix.hxx 1.11.148.2 sc/source/core/data/validat.cxx 1.24.110.1 sc/source/core/tool/interpr1.cxx 1.60.54.2 sc/source/core/tool/interpr4.cxx 1.57.98.1 sc/source/core/tool/interpr5.cxx 1.33.36.2 sc/source/core/tool/scmatrix.cxx 1.17.136.2 sc/source/filter/excel/xehelper.cxx 1.31.148.1 sc/source/filter/xml/XMLExportDDELinks.cxx 1.18.148.1 Please cvs update your CWS.
*** Issue 27656 has been marked as a duplicate of this issue. ***
Test cases from issue 27656: http://www.openoffice.org/nonav/issues/showattachment.cgi/14440/arraycalc.sxc http://www.openoffice.org/nonav/issues/showattachment.cgi/14443/arraycalc.xls
*** Issue 95176 has been marked as a duplicate of this issue. ***
Test case from issue 95176: http://www.openoffice.org/nonav/issues/showattachment.cgi/57284/Example.xls
Tested issue 95176, the result is right in the version with patch of i46681. :)
*** Issue 96682 has been marked as a duplicate of this issue. ***
Fixed in CWS odff05.
Reassigning to QA for verification.
Created attachment 58880 [details] TestCaseSpecification
Created attachment 58881 [details] Testdocuments for Test Case Specification
verified in internal build cws_odff05
Verified in Ooo310m9 on WinXP and Fedora Closing Li Meiying