From 07c12e09f0f74dc649bb84ded2eae12809170010 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Fri, 15 May 2009 21:36:25 +0000 Subject: [PATCH] Fix for bug noticed while investigating bugzilla 47048. Made INDEX() support missing args. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@775360 13f79535-47bb-0310-9956-ffa450edef68 --- .../hssf/record/formula/functions/Index.java | 128 +++++++++++------- .../hssf/data/IndexFunctionTestCaseData.xls | Bin 24576 -> 26112 bytes .../record/formula/functions/TestIndex.java | 55 ++++++-- 3 files changed, 123 insertions(+), 60 deletions(-) diff --git a/src/java/org/apache/poi/hssf/record/formula/functions/Index.java b/src/java/org/apache/poi/hssf/record/formula/functions/Index.java index 1c34322256..371a06e30a 100644 --- a/src/java/org/apache/poi/hssf/record/formula/functions/Index.java +++ b/src/java/org/apache/poi/hssf/record/formula/functions/Index.java @@ -18,9 +18,11 @@ package org.apache.poi.hssf.record.formula.functions; import org.apache.poi.hssf.record.formula.eval.AreaEval; +import org.apache.poi.hssf.record.formula.eval.BlankEval; import org.apache.poi.hssf.record.formula.eval.ErrorEval; import org.apache.poi.hssf.record.formula.eval.Eval; import org.apache.poi.hssf.record.formula.eval.EvaluationException; +import org.apache.poi.hssf.record.formula.eval.MissingArgEval; import org.apache.poi.hssf.record.formula.eval.OperandResolver; import org.apache.poi.hssf.record.formula.eval.RefEval; import org.apache.poi.hssf.record.formula.eval.ValueEval; @@ -28,7 +30,7 @@ import org.apache.poi.hssf.record.formula.eval.ValueEval; /** * Implementation for the Excel function INDEX *

- * + * * Syntax :
* INDEX ( reference, row_num[, column_num [, area_num]])
* INDEX ( array, row_num[, column_num]) @@ -40,12 +42,11 @@ import org.apache.poi.hssf.record.formula.eval.ValueEval; * area_numused when reference is a union of areas * *

- * + * * @author Josh Micich */ public final class Index implements Function { - // TODO - javadoc for interface method public Eval evaluate(Eval[] args, int srcCellRow, short srcCellCol) { int nArgs = args.length; if(nArgs < 2) { @@ -58,112 +59,141 @@ public final class Index implements Function { firstArg = ((RefEval)firstArg).offset(0, 0, 0, 0); } if(!(firstArg instanceof AreaEval)) { - + // else the other variation of this function takes an array as the first argument // it seems like interface 'ArrayEval' does not even exist yet - + throw new RuntimeException("Incomplete code - cannot handle first arg of type (" + firstArg.getClass().getName() + ")"); } AreaEval reference = (AreaEval) firstArg; - + int rowIx = 0; int columnIx = 0; - int areaIx = 0; - try { + boolean colArgWasPassed = false; + try { switch(nArgs) { case 4: - areaIx = convertIndexArgToZeroBase(args[3], srcCellRow, srcCellCol); throw new RuntimeException("Incomplete code" + " - don't know how to support the 'area_num' parameter yet)"); // Excel expression might look like this "INDEX( (A1:B4, C3:D6, D2:E5 ), 1, 2, 3) // In this example, the 3rd area would be used i.e. D2:E5, and the overall result would be E2 // Token array might be encoded like this: MemAreaPtg, AreaPtg, AreaPtg, UnionPtg, UnionPtg, ParenthesesPtg // The formula parser doesn't seem to support this yet. Not sure if the evaluator does either - + case 3: - columnIx = convertIndexArgToZeroBase(args[2], srcCellRow, srcCellCol); + columnIx = resolveIndexArg(args[2], srcCellRow, srcCellCol); + colArgWasPassed = true; case 2: - rowIx = convertIndexArgToZeroBase(args[1], srcCellRow, srcCellCol); + rowIx = resolveIndexArg(args[1], srcCellRow, srcCellCol); break; default: // too many arguments return ErrorEval.VALUE_INVALID; } - return getValueFromArea(reference, rowIx, columnIx, nArgs); + return getValueFromArea(reference, rowIx, columnIx, colArgWasPassed, srcCellRow, srcCellCol); } catch (EvaluationException e) { return e.getErrorEval(); } } - + /** - * @param nArgs - needed because error codes are slightly different when only 2 args are passed + * @param colArgWasPassed false if the INDEX argument list had just 2 items + * (exactly 1 comma). If anything is passed for the column_num argument + * (including {@link BlankEval} or {@link MissingArgEval}) this parameter will be + * true. This parameter is needed because error codes are slightly + * different when only 2 args are passed. */ - private static ValueEval getValueFromArea(AreaEval ae, int pRowIx, int pColumnIx, int nArgs) throws EvaluationException { + private static ValueEval getValueFromArea(AreaEval ae, int pRowIx, int pColumnIx, + boolean colArgWasPassed, int srcRowIx, int srcColIx) throws EvaluationException { + boolean rowArgWasEmpty = pRowIx == 0; + boolean colArgWasEmpty = pColumnIx == 0; int rowIx; int columnIx; - + // when the area ref is a single row or a single column, // there are special rules for conversion of rowIx and columnIx if (ae.isRow()) { if (ae.isColumn()) { - rowIx = pRowIx == -1 ? 0 : pRowIx; - columnIx = pColumnIx == -1 ? 0 : pColumnIx; + // single cell ref + rowIx = rowArgWasEmpty ? 0 : pRowIx-1; + columnIx = colArgWasEmpty ? 0 : pColumnIx-1; } else { - if (nArgs == 2) { - rowIx = 0; - columnIx = pRowIx; + if (colArgWasPassed) { + rowIx = rowArgWasEmpty ? 0 : pRowIx-1; + columnIx = pColumnIx-1; } else { - rowIx = pRowIx == -1 ? 0 : pRowIx; - columnIx = pColumnIx; + // special case - row arg seems to get used as the column index + rowIx = 0; + // transfer both the index value and the empty flag from 'row' to 'column': + columnIx = pRowIx-1; + colArgWasEmpty = rowArgWasEmpty; } } - if (rowIx < -1 || columnIx < -1) { - throw new EvaluationException(ErrorEval.VALUE_INVALID); - } } else if (ae.isColumn()) { - if (nArgs == 2) { - rowIx = pRowIx; - columnIx = 0; + if (rowArgWasEmpty) { + rowIx = srcRowIx - ae.getFirstRow(); } else { - rowIx = pRowIx; - columnIx = pColumnIx == -1 ? 0 : pColumnIx; + rowIx = pRowIx-1; } - if (rowIx < -1 || columnIx < -1) { - throw new EvaluationException(ErrorEval.VALUE_INVALID); + if (colArgWasEmpty) { + columnIx = 0; + } else { + columnIx = colArgWasEmpty ? 0 : pColumnIx-1; } } else { - if (nArgs == 2) { + // ae is an area (not single row or column) + if (!colArgWasPassed) { // always an error with 2-D area refs - if (pRowIx < -1) { - throw new EvaluationException(ErrorEval.VALUE_INVALID); - } - throw new EvaluationException(ErrorEval.REF_INVALID); + // Note - the type of error changes if the pRowArg is negative + throw new EvaluationException(pRowIx < 0 ? ErrorEval.VALUE_INVALID : ErrorEval.REF_INVALID); } // Normal case - area ref is 2-D, and both index args were provided - rowIx = pRowIx; - columnIx = pColumnIx; + // if either arg is missing (or blank) the logic is similar to OperandResolver.getSingleValue() + if (rowArgWasEmpty) { + rowIx = srcRowIx - ae.getFirstRow(); + } else { + rowIx = pRowIx-1; + } + if (colArgWasEmpty) { + columnIx = srcColIx - ae.getFirstColumn(); + } else { + columnIx = pColumnIx-1; + } } - + int width = ae.getWidth(); int height = ae.getHeight(); // Slightly irregular logic for bounds checking errors - if (rowIx >= height || columnIx >= width) { + if (!rowArgWasEmpty && rowIx >= height || !colArgWasEmpty && columnIx >= width) { + // high bounds check fail gives #REF! if arg was explicitly passed throw new EvaluationException(ErrorEval.REF_INVALID); } - if (rowIx < 0 || columnIx < 0) { + if (rowIx < 0 || columnIx < 0 || rowIx >= height || columnIx >= width) { throw new EvaluationException(ErrorEval.VALUE_INVALID); } return ae.getRelativeValue(rowIx, columnIx); } + /** - * takes a NumberEval representing a 1-based index and returns the zero-based int value + * @param arg a 1-based index. + * @return the resolved 1-based index. Zero if the arg was missing or blank + * @throws EvaluationException if the arg is an error value evaluates to a negative numeric value */ - private static int convertIndexArgToZeroBase(Eval arg, int srcCellRow, short srcCellCol) throws EvaluationException { - + private static int resolveIndexArg(Eval arg, int srcCellRow, short srcCellCol) throws EvaluationException { + ValueEval ev = OperandResolver.getSingleValue(arg, srcCellRow, srcCellCol); - int oneBasedVal = OperandResolver.coerceValueToInt(ev); - return oneBasedVal - 1; + if (ev == MissingArgEval.instance) { + return 0; + } + if (ev == BlankEval.INSTANCE) { + return 0; + } + int result = OperandResolver.coerceValueToInt(ev); + if (result < 0) { + throw new EvaluationException(ErrorEval.VALUE_INVALID); + } + return result; } } diff --git a/src/testcases/org/apache/poi/hssf/data/IndexFunctionTestCaseData.xls b/src/testcases/org/apache/poi/hssf/data/IndexFunctionTestCaseData.xls index 9dcde6177295a38037d74dbcf7ee9c5774c44086..bf0b23accb90886870920c5b5416b33f320a4216 100644 GIT binary patch delta 4815 zcmZ`-Yiu0V6+ScGr|s+}v1>cnY1SAQQs?D5PuGrj9p~ZLC5efXC{>y`*|oiXE%w?J z+A14B1y!UXbE z%f_P_?|k#!@0|0^Ju`RrLf*P?-MZ9cZF;r$eaq@pe*1-QbP&-7>>IIvRVWmA-?IFw zy~n!jORie|yRLVliEGxGFTQ-@xsR_|o7IxXP(!UnVY+G~C7W2Kq1L8G zx@2kKaLAFZk8H3k+F8o1S1*NDI3nu0mTOWsLONFUR-+ORHdXhOo^seeBX%a-+V*zP z+CyVBOJ`|;()iR#-=x2jMIpn3j=CB~faUkXAFQ|jz5HZXl@w)FJ?ijUeZ=P9ORVWY*l3h^U z-9OvaOjYnZijm1b{F(}d5ao$(`N?AVs&6eWhsm}nSVwILN5hygc^nGri|&V7n#rd} zw~VoHh}8=k^$$=GuhVc8IX%GN-2q8l_-P;ZV03kFY zKUP3aZVs;q0Tp-@RvN;s-fb4CuHA=ld~|mL$9Lrz?Bmaaa(tq%Z)-C(l!>f%L>Rv6 zYr^Gb_9+mdwkqwD`d42o(rYXm*i;;-I^%0W0D$y}exmQd8bip&?*;*Y5N>M19ft6h z5C90_C2cZNVF!?#s!6Z{0CB*z2v=Of@kB3X!8)z*XRUywnZPj5f_1c+t6POx;8y3% zf;SRfxn_##(O0fJbEwsf4%$ARoD;XG8nh~85k#qE$2wciX5DLB)hGwM%yvlTiN{*bD*yj|pvt zuwySzBmkoDtS;j&LwIa2D*zBey>_YH5Iz$E03poi9KK};JNK~y08b7uSo^G=+Q&KE zT}HUa5Iz?I0FS~3L)g5Z71nD4PbqGPZyUmSApj5sm(XDd9|{40hj79WS`Kgs#Pkq0 z8p5;?0C)(S4B-tS0PqkngIpiNgB$|$%|keD2qy+R(2re0AUdjV7K~#BhPfZR@Q%>^ z$nO+>ZmAasJD|K(53E1yJhHYG2de)JaM*gqSAmFocUj03gY` z?S@ozZtBy6-0}cC)gfz_aK}&=`mslc_(0DzB+UedxgUGzAXgVfKf2X9{g@r3QmhUr!^}|d6;He;P2#*Z&{Q)2f_iKfOA^d5$IWdjqZn1~Z z&_l90t?fNV=yb8McS&P2HzDnvQ1ktD%kl5EaA!kXVjb48`c16I4so^sgs?>u_87uv zLIB_?2R>>$)y~6Q&VVMkJ+api9v1=tj{+=q75*ZGePs&!4Wa!AhX5c7ZioYhuqXro z9s(NK4e^c;2Fr559B>J%M>qrkj{_wjuIfcQkU?>g^+BZhFl5C90_w>tiaAzTvz0I9(3 zocj#n&ZAr+01pAFbBXw;=7n%n6I`1|4IwWC0HWX$ju}F9ltTdU5Ky)o;s-*&v&fUf zDMR>;5a6Om0S_}*;pXTz-xa(a)V5;>d`Dq}didDuxq!S2o#|iRBr_FTD1y5)jET?h z!n3eEAn!tau%dz`D`2S#*f?XkvcVG-h?5ntbOmgx0ybR%n=Qd|Zq{=ppo?WHVDrHI zWl1koz_Jyv#jj&IS}Fs1xx%(aWubB2uBIfGF+62%^ab#WO?vJy);GArb=01sajZ)G z)Gz>gzQ1ZM`Rx$MyzYrwER*ucq(p{t^b2VS!fBZf_@blNQDdA~O7xHmIcGje%sunt zjJubkjeDYvJJIMxS)CAcbg-P8HTb2`&awWuMB`rk8AszU1{`g|6AhN&8t=Rg zA?TEq2D-K=7s6QM(PW9vMw%?sf%Sz(YERRYSUrG=na9D^3SoeeN_(WzI-R6;drC{? zTrEuK`~b$3njn^^MH7{X328*;vfQ&$%cndM5kZ*$)L*=&%aTX84`aziteksHx40D5T|V%%KOIG!OHEl6>0qKp>2u}0qyMBpwmiWxOORF}($ z2G40uk)s*&Gh&5%jh_oqtmB>Z7d+`Nlo^BQ94t$V5cHvCK`)xJAR(QW&1F40S)KKR zvhXDQNKmE&`qym87H!}Xvc(eElttGRVpA5i{2493L@VvDJb5mPDT^LcxCYbqOP++6 z$_hsT-oimI7QUnl{{&xECIduqD}t5FEVli456xp6!cy%Bt(wKZ{aC@Brrq{xdObku z!Q=Nt;5`2LcJQBi<@m_+r>x7T9)9h_7Tfxj>O9%DhF>JL*e_sTf&C)(I_yuY*^|vH b_=@|h5~-h`+~A)%fS;k%n6aesZZ{K4K-Jond+`4s-?&<8tCTxAJ+qy^Abq~fOYC#f$1`>%R5(rKN z6A}_8|BztRh++&FKPU}+FlZK09)XF87{6%Z2TjmVd@(8s%yZ5?EC0L1G`DMaem(d6 zU#Hv5En{ZRnC&teKJUI}7|qIMp7lLkSXiK|Dfgw>W4x6uJn7u=cfzN ze#D=RYM=8Bb3UW)I}aJx)qd9rCkpXXBbmB+`i zQuV96@}KlB<6rp;4e-Hb697!0EdD|hR>LgbfPXRSwbHIEYBUV!)a6Qb&b!=7qHamL zQgzQO_bO<6lqKdVE$GtU(ru%Ul4X5?s{02f!RY8ESIV{`wF&g`O_9D1J0g(a$BFV9X^P>(!r|_91r(AN|(4saxYN;x6F_ zx`<;K3$co!LOdFG6L$+Q?0yM3hHc!daSw5iaMfLn9K+049p=6z z*&oxmkGN0xsz|$0p@t%9e10bq$LC-8xjag*xAF7YXfzN6|B{r&n6e0}aE>AA2T=Z4 zU}q3FrhQnYn!?)W66ShvnO<}b9M1~lkXZnwn1Yt@Uepaid}qz!Wm)SoEdxr*ppM6C ztAl_M{#@sG)&i~3WSPj@EHT3J&J|0p)#MtH#c|4s5tMYmta-*!=SR6#)n2=V*J-?#c&+f)nPb?Ny;0+J#Os8A%{(NW)|n1yy~abt zL&6>XR1AhW`yy}9cs=oY;YsEgw(%_*Zy??v{3vs*)yA7N-blPr_*Lc@w((|-HxX|V z?nzPY7`AcT-uZcNCf+PO#k^$+pVD{>@fP97QVrN=!ps3aEwMHME*qUd`u-W~z;+em=8x~-Cmi-cY4=T{ID9H{ke!+oz?CV)b99U zq;qA%?bP+~wp_+5(w7oi8F4BjE>*ucLW9IG^KWEN^k_UmJR$rda}3+&1eYqmG(E(7 zEPgUg$0I3v@o4vII!QVy$>Yp1Y)g)dn@`?LyjS>P=6y@D_iMb5c%Seq%rVT_7mG=0 zyq|c#aOWu1j$s?eu9hz*MLZ?Em-)bw+6TdWxVUErhz|&VlR1WM*>NoS?1RJygUM4_Dynx_z0eea3+j(r-jD&@BoZzG)*)unGa`Z{V|M%;9nfP{s0+` zj}jjhKF=J(%y;;yB>{|SJVQJqymXARW0;3&bq9cPjgJu@6aK;&%{hi`9EVM_!owUV zJ}&$t<`crL6$O~o_yq9@;lDG-FlS%nn>9X3d{TJLI2D6o8^?*w7qgl8X5l-TV=uSW z{;bBQh))SW#~i=5Y}xUvA)kE<@h!st7;ksp!nIVD6Dj8rTz|D^;!=DW|8YcT-`*Y4 z`HrwOV)cy#M9gBi$SKZnX}jefYz^&fMD+vflP diff --git a/src/testcases/org/apache/poi/hssf/record/formula/functions/TestIndex.java b/src/testcases/org/apache/poi/hssf/record/formula/functions/TestIndex.java index 95355733d2..764e30cd99 100755 --- a/src/testcases/org/apache/poi/hssf/record/formula/functions/TestIndex.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/functions/TestIndex.java @@ -17,20 +17,28 @@ package org.apache.poi.hssf.record.formula.functions; +import junit.framework.AssertionFailedError; import junit.framework.TestCase; import org.apache.poi.hssf.record.formula.eval.AreaEval; import org.apache.poi.hssf.record.formula.eval.Eval; +import org.apache.poi.hssf.record.formula.eval.MissingArgEval; import org.apache.poi.hssf.record.formula.eval.NumberEval; import org.apache.poi.hssf.record.formula.eval.ValueEval; /** - * Tests for the INDEX() function - * + * Tests for the INDEX() function.

+ * + * This class contains just a few specific cases that directly invoke {@link Index}, + * with minimum overhead.
+ * Another test: {@link TestIndexFunctionFromSpreadsheet} operates from a higher level + * and has far greater coverage of input permutations.
+ * * @author Josh Micich */ public final class TestIndex extends TestCase { + private static final Index FUNC_INST = new Index(); private static final double[] TEST_VALUES0 = { 1, 2, 3, 4, @@ -39,44 +47,69 @@ public final class TestIndex extends TestCase { 9, 10, 11, 12, }; - + /** * For the case when the first argument to INDEX() is an area reference */ public void testEvaluateAreaReference() { - + double[] values = TEST_VALUES0; confirmAreaEval("C1:D6", values, 4, 1, 7); confirmAreaEval("C1:D6", values, 6, 2, 12); confirmAreaEval("C1:D6", values, 3, 1, 5); - + // now treat same data as 3 columns, 4 rows - confirmAreaEval("C10:E13", values, 2, 2, 5); + confirmAreaEval("C10:E13", values, 2, 2, 5); confirmAreaEval("C10:E13", values, 4, 1, 10); } - + /** * @param areaRefString in Excel notation e.g. 'D2:E97' * @param dValues array of evaluated values for the area reference * @param rowNum 1-based * @param colNum 1-based, pass -1 to signify argument not present */ - private static void confirmAreaEval(String areaRefString, double[] dValues, + private static void confirmAreaEval(String areaRefString, double[] dValues, int rowNum, int colNum, double expectedResult) { ValueEval[] values = new ValueEval[dValues.length]; for (int i = 0; i < values.length; i++) { values[i] = new NumberEval(dValues[i]); } AreaEval arg0 = EvalFactory.createAreaEval(areaRefString, values); - + Eval[] args; if (colNum > 0) { args = new Eval[] { arg0, new NumberEval(rowNum), new NumberEval(colNum), }; } else { args = new Eval[] { arg0, new NumberEval(rowNum), }; } - - double actual = NumericFunctionInvoker.invoke(new Index(), args); + + double actual = NumericFunctionInvoker.invoke(FUNC_INST, args); assertEquals(expectedResult, actual, 0D); } + + /** + * Tests expressions like "INDEX(A1:C1,,2)".
+ * This problem was found while fixing bug 47048 and is observable up to svn r773441. + */ + public void testMissingArg() { + ValueEval[] values = { + new NumberEval(25.0), + new NumberEval(26.0), + new NumberEval(28.0), + }; + AreaEval arg0 = EvalFactory.createAreaEval("A10:C10", values); + Eval[] args = new Eval[] { arg0, MissingArgEval.instance, new NumberEval(2), }; + Eval actualResult; + try { + actualResult = FUNC_INST.evaluate(args, 1, (short)1); + } catch (RuntimeException e) { + if (e.getMessage().equals("Unexpected arg eval type (org.apache.poi.hssf.record.formula.eval.MissingArgEval")) { + throw new AssertionFailedError("Identified bug 47048b - INDEX() should support missing-arg"); + } + throw e; + } + assertEquals(NumberEval.class, actualResult.getClass()); + assertEquals(26.0, ((NumberEval)actualResult).getNumberValue(), 0.0); + } } -- 2.39.5