Bug 63302 - [PATCH] Formula evaluation of names with offset or row function is incorrect
Summary: [PATCH] Formula evaluation of names with offset or row function is incorrect
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: 4.0.x-dev
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-29 16:47 UTC by John Lincoln White
Modified: 2019-03-30 18:14 UTC (History)
0 users



Attachments
Proposed patch (1.10 KB, application/x-gzip)
2019-03-29 16:47 UTC, John Lincoln White
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Lincoln White 2019-03-29 16:47:56 UTC
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.
Comment 1 Greg Woolsey 2019-03-29 23:57:50 UTC
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.
Comment 2 Yegor Kozlov 2019-03-30 13:26:26 UTC
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.
Comment 3 Greg Woolsey 2019-03-30 18:14:56 UTC
Applied in r1856644.  Thanks for the patch, tests, and detailed explanation!