Bug 59814 - WorkbookEvaluator.clearAllCachedResultValues() needs to propagate the action to EvaluationWorkbook and EvaluationSheet instances
Summary: WorkbookEvaluator.clearAllCachedResultValues() needs to propagate the action ...
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.15-dev
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-07 02:42 UTC by Greg Woolsey
Modified: 2016-07-15 18:51 UTC (History)
0 users



Attachments
Fix and updated unit test resource (10.26 KB, patch)
2016-07-07 19:07 UTC, Greg Woolsey
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Woolsey 2016-07-07 02:42:17 UTC
Now that I've added more cached values to support Structured Reference syntax and performance improvements, I see that this method needs augmentation to clear the new caches down at the workbook and sheet level.

XSSFEvaluationWorkbook has _sheetCache, which needs to be set to null
BaseXSSFEvaluationWorkbook has _tableCache, which needs to be set to null
XSSFEvaluationSheet has _cellCache, which needs to be set to null

I'll work on a patch tomorrow, with some tests.  I have some other temp changes to strip out, though, to make current trunk work with another project that doesn't like some of the 3.15 API changes yet.

Motivation for this change is that in that library (Vaadin Spreadsheet) there isn't a way currently to reset the formula evaluator in use, so I need to make sure clear really clears state.
Comment 1 Greg Woolsey 2016-07-07 05:19:18 UTC
This may only be an issue for cells inside a Table, referenced by Structured Reference syntax in a formula - very narrow.  It appears in this new use case the cell values are calculated from the XSSFEvaluationSheet._cellCache and not from the WorkbookEvaluator cell cache, thus the need to push the clear down.  

Still a good idea though, to make it more obvious what needs changing when adding future cached data.

Building a test case will be more work than usual, as it requires a Table and a formula cell that references something in the table that can change.
Comment 2 Greg Woolsey 2016-07-07 19:07:49 UTC
Created attachment 34022 [details]
Fix and updated unit test resource

Attached a patch to propagate the cache clearing through the evaluator object tree.

Includes an updated Structured References unit test and source file.  The updated test fails before the fix and passes with the fix.
Comment 3 Javen O'Neal 2016-07-07 23:18:36 UTC
Applied in r1751836 without modification. Added @Overrides in r1751837.
Thanks for the patch.
Comment 4 Dominik Stadler 2016-07-15 18:51:46 UTC
This seems to be fixed based on the previous comment. Please reopen if not.