| Summary: | [PATCH] Formula evaluation of names with offset or row function is incorrect | ||
|---|---|---|---|
| Product: | POI | Reporter: | John Lincoln White <jwhite> |
| Component: | SS Common | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | P2 | ||
| Version: | 4.0.x-dev | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | Proposed patch | ||
Is this patch against SVN trunk? I ask because I made a change the last couple weeks in this area that fixed a different area reference offset bug, and want to make sure this doesn't step on that. I'll likely be able to take a look at this in a couple days. This sounds like a bug patch I will want before we do a new release. A good catch and it is reproducible in trunk. WorkbookEvaluator#evaluateNameFormula was introduced in r910043 to support evaluation of indirect names in INDIRECT, e.g. =SUM(INDIRECT(A4)) where A4 is a string cell with a value referring to a named range. The logic supports plain cell references like Sheet1!A1:D1, but it was never tested with names referring to functions like in the attached patch. Another way to reproduce the problem is to modify TestIndirect#createWBA() to create a name with a formula instead of a cell reference: change HSSFName name2 = wb.createName(); name2.setNameName("sales2"); name2.setRefersToFormula("Sheet2!B1:C3"); to HSSFName name2 = wb.createName(); name2.setNameName("sales2"); name2.setRefersToFormula("ROW()"); and the test will fail with java.lang.RuntimeException: Unexpected ptg class (org.apache.poi.ss.formula.ptg.FuncVarPtg) at org.apache.poi.ss.formula.WorkbookEvaluator.getEvalForPtg(WorkbookEvaluator.java:750) at org.apache.poi.ss.formula.WorkbookEvaluator.evaluateNameFormula(WorkbookEvaluator.java:777) at org.apache.poi.ss.formula.OperationEvaluationContext.getDynamicReference(OperationEvaluationContext.java:225) at org.apache.poi.ss.formula.functions.Indirect.evaluateIndirect(Indirect.java:140) at org.apache.poi.ss.formula.functions.Indirect.evaluate(Indirect.java:81) at org.apache.poi.ss.formula.OperationEvaluatorFactory.evaluate(OperationEvaluatorFactory.java:155) The proposed patch fixes the problem and I'm +1 to applying it. |
Created attachment 36504 [details] Proposed patch Overview Formula evaluation of names with OFFSET or ROW function is incorrect Steps to Reproduce 1) In a new workbook, put values 2, 5, 3, 7 in cells Sheet1!A1:A4: | A ---+---- 1 | 2 2 | 5 3 | 3 4 | 7 2) Create a named range offsetFormula with the refers to formula =OFFSET(Sheet1!$A$1:$A$4,2,0,2,1). Create a named range rowFormula with the refers to formula =ROW(). 3) Set cell C5 to =SUM(offsetFormula). Set cell C6 to =rowFormula. Evaluate cells C5 and C6 with the workbook's FormulaEvaluator. Actual Results Cell C5 evaluates to 0. Evaluation of cell C6 throws a RuntimeException. Expected Results Cell C5 should be evaluated to 10, the sum of Sheet1!A3:A4. Cell C6 should be evaluated to 6, the row number of cell C6. Additional Information In the process of evaluating the formula =SUM(offsetFormula), when WorkbookEvaluator.evaluateFormula sees the named range, it calls getEvalForNameRecord to evaluate the named range, calling evaluateNameFormula on the refers to formula of the name. evaluateNameFormula calls getEvalForPtg when the formula has a single Ptg and calls WorkbookEvaluator.evaluateFormula when there is more than one Ptg. getEvalForPtg cannot evaluate a Ptg of type FuncVarPtg, causing the RuntimeException. Evaluating the Ptg in this case through WorkbookEvaluator.evaluateFormula correctly returns the row number of the cell whose formula is being evaluated. When the formula is evaluated by WorkbookEvaluator.evaluateFormula and OperationEvaluationContext.isSingleValue() is TRUE. evaluateFormula returns a single value "unwrapped" based on the context's row and column. In the case of an OFFSET formula, this returns some value in the range of the OFFSET formula result, or an error value if beyond the range. The correct evaluation of the formula should be a type such as LazyAreaEval, which IS the value before WorkbookEvaluator.dereferenceResult is applied. Calling WorkbookEvaluator.evaluateFormula with an OperationEvaluationContext.isSingleValue() FALSE fixes the issue of incorrect evaluation of offsetFormula. I've attached a patch that adds the reproduction case to the test TestWorkbookEvaluator.testNamesInFormulas and makes the changes to evaluateNameFormula as described.