Bug 60131 - [PATCH] D* functions refactor, use OperandResolver
Summary: [PATCH] D* functions refactor, use OperandResolver
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: SS Common (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2016-09-14 09:14 UTC by Patrick Böker
Modified: 2016-09-14 14:59 UTC (History)
0 users


Attachments
dstar_OperandResolver_refactor.patch (12.20 KB, patch)
2016-09-14 09:14 UTC, Patrick Böker 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Böker 2016-09-14 09:14:38 UTC 
Created attachment 34246 [details]
dstar_OperandResolver_refactor.patch

Non functionality changing refactor of DStarRunner.java to not use custom implementations to resolve references or coerce to string, but use the OperandResolver static methods instead.
Tests still all pass.
Also removes an unused method.
Comment 1 Nick Burch 2016-09-14 11:52:47 UTC 
With this patch applied to trunk, but not your other patch, the unit tests fail with:

Testcase: processFunctionRow[strange types as headers - wrong number] took 0.001 sec
   Caused an ERROR
Error evaluating cell DGet!B98
org.apache.poi.ss.formula.eval.NotImplementedException: Error evaluating cell DGet!B98
   at org.apache.poi.ss.formula.WorkbookEvaluator.addExceptionInfo(WorkbookEvaluator.java:386)
   at org.apache.poi.ss.formula.WorkbookEvaluator.evaluateAny(WorkbookEvaluator.java:327)
   at org.apache.poi.ss.formula.WorkbookEvaluator.evaluate(WorkbookEvaluator.java:259)
   at org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator.evaluateFormulaCellValue(HSSFFormulaEvaluator.java:200)
   at org.apache.poi.ss.formula.BaseFormulaEvaluator.evaluate(BaseFormulaEvaluator.java:101)
   at org.apache.poi.ss.formula.functions.BaseTestFunctionsFromSpreadsheet.processFunctionRow(BaseTestFunctionsFromSpreadsheet.java:151)
Caused by: org.apache.poi.ss.formula.eval.NotImplementedException: D* function with formula conditions
   at org.apache.poi.ss.formula.functions.DStarRunner.fullfillsConditions(DStarRunner.java:227)
   at org.apache.poi.ss.formula.functions.DStarRunner.evaluate(DStarRunner.java:102)
   at org.apache.poi.ss.formula.functions.DStarRunner.evaluate(DStarRunner.java:55)
   at org.apache.poi.ss.formula.OperationEvaluatorFactory.evaluate(OperationEvaluatorFactory.java:132)
   at org.apache.poi.ss.formula.WorkbookEvaluator.evaluateFormula(WorkbookEvaluator.java:550)
   at org.apache.poi.ss.formula.WorkbookEvaluator.evaluateAny(WorkbookEvaluator.java:317)

Do we need to get your other patch fixed and applied first? Or do we need to fix the current tests for this patch?
Comment 2 Patrick Böker 2016-09-14 13:52:03 UTC 
(In reply to Nick Burch from comment #1)
> With this patch applied to trunk, but not your other patch, the unit tests
> fail with:
> 
> ...
> 
> Do we need to get your other patch fixed and applied first? Or do we need to
> fix the current tests for this patch?

I messed up a bit. Sorry for that.
Both patches together work fine.


There is a fix relating to error values in the D* database headers in here I overlooked while preparing the patches. Because of that the new DGet.xls (which tests that behavior) fails without this patch.

Also the DGet.xls has a false-positive test now <skip>ed which this patch relies on.

Easiest way forward: Just apply both patches and the DGet.xls.
Alternative: I prepare another intermediate DGet.xls so one can apply the patches one after the other.

What do you think?
Comment 3 Javen O'Neal 2016-09-14 13:53:28 UTC 
Together is fine.
Comment 4 Nick Burch 2016-09-14 14:59:29 UTC 
Patch applied in r1760717, thanks!