Bug 21923 - [PATCH] Modify HSSFWorkbook, FormulaParser and SheetReferences to support 3DRefs
Summary: [PATCH] Modify HSSFWorkbook, FormulaParser and SheetReferences to support 3DRefs
Status: RESOLVED WONTFIX
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: 3.0-dev
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on: 34944
Blocks:
  Show dependency tree
 
Reported: 2003-07-28 03:42 UTC by Eric Ladner
Modified: 2008-12-16 10:01 UTC (History)
0 users



Attachments
Patch for 3DRef resolving in a formua (HSSFWorkbook, SheetReferences and FormulaParser patched). (4.47 KB, patch)
2003-07-28 03:50 UTC, Eric Ladner
Details | Diff
Test class that exhibits the problem (1.90 KB, text/plain)
2005-05-16 17:47 UTC, Eric Ladner
Details
spreadsheet that shows the problem (6.50 KB, application/octet-stream)
2005-05-16 17:48 UTC, Eric Ladner
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Ladner 2003-07-28 03:42:41 UTC
This patch modifies HSSFWorkbook, FormulaParser and SheetReferences to allow
sheet references to be captured from HSSFWorkbook (new method
getSheetReferences, which mirrors Workbook.getSheetReferences()) and passed into
the FormulaParser (new methods getSheetReferences() and setSheetReferences()). 
I purposefully didn't add a new constructor for FormulaParser(String,
SheetReferences) because of the common practice up to now of just calling 'new
FormulaParser(formula, null)' (null for the workbook).  That call produces an
ambiguous call error.  Instead I added the setSheetReferences, so the typical
way to call it now would be something like:

        POIFSFileSystem fs = new POIFSFileSystem(new FileInputStream(file));
        HSSFWorkbook wb    = new HSSFWorkbook(fs);
        HSSFSheet sheet    = wb.getSheetAt(0);
        HSSFRow row        = sheet.getRow(0);
        SheetReferneces refs = wb.getSheetReferences();

        FormulaParser fp = new FormulaParser(formula, null);
        fp.setSheetReferences(refs);
        fp.parse();

Also, modifications to FormulaParser were required to check whether to get the
sheet numbers from the workbook, if valid, or the sheet references, if supplied.
 This also required a new method getSheetIndex() in SheetReferences (the
counterpart to getSheetName()).

A unit test was added to TestSheetReferences to test the reverse lookups.

Unit tests weren't added yet to test the advanced formula parsing, as I'm not
that up on JUnit yet, but if anyone has an pointers, I'm all ears.
Comment 1 Eric Ladner 2003-07-28 03:50:01 UTC
Created attachment 7539 [details]
Patch for 3DRef resolving in a formua (HSSFWorkbook, SheetReferences and FormulaParser patched).
Comment 2 Andy Oliver 2003-07-28 04:24:31 UTC
com'on JUnit isn't hard to use.  go LOOK at the unit tests.  You're telling me you need pointers?  
The only pointer I have is write the unit test before the code.  

I have to admit I don't favor this patch for the same reason I don't want to open up the model.  
This smells like a hack and I think we can do something better.  I'm retargeting it to 3.0, but I 
favor a different approach possibly just refactoring where needed.  I do appreciate your 
contribution though.
Comment 3 Paul Krause 2003-07-28 04:37:49 UTC
Here's a pointer:  avoid assert and assertTrue.  If the test involves checking 
two values for equality, use assertEquals.  For example,

assertEquals("ref A", 0, refs.getSheetIndex("A"));

Put the expected result on the left and the actual result on the right.  This 
will give you much more descriptive error messages when something goes wrong.
Comment 4 Eric Ladner 2003-07-28 15:45:23 UTC
Ok, ok.. I'll poke around with JUnit.. :)

As for the assertTrue, I was just following the previous examlpe in
TestSheetReferehces.

Regardless of how it's refactored, somehow, the FormulaParser needs information
out of the workbook, or access to the workbook to resolve 3DRefs.  Getting a
copy of the SheetReferences seemed like less of a hack than exposing the
internal workbook to the outside world.  At least it can't be used in reverse to
mess up the workbook.  Admittedly, it's still a hack, though.
Comment 5 Andy Oliver 2003-07-28 15:56:00 UTC
Right, there is no need for dirty hacks in 3.0.  Play with the scaffolding a bit.  Try to fix the frame a 
bit.
Comment 6 Avik Sengupta 2003-10-30 19:08:00 UTC
This patch will now certainly not applied since FormulaParser etc has been
refactored by the macro function patch of paul krause. The functionality of this
patch may however still have to be added. To be investigated!
Comment 7 Avik Sengupta 2005-04-22 14:23:43 UTC
Ok, so formula parser has moved on, but without any testcases, I have no clue
what problem this patch is supposed to solve! So while this looks useful at
first glance, I am finding it diffcult to figure this out!

Can someone atleast give me a worksheet, or formulas, that this solves?
Comment 8 Eric Ladner 2005-04-29 05:53:22 UTC
The problem was that parsing a 3DRef requires the sheet references in the
formula parser, but there was no way to actually get the sheet references from
inside the parser.  The two solutions I toyed around with was this one (getting
the references from teh worksheet, then passing them to the parser) and making
the lower workbook available to HSSFWorkbook by making the
Workbook.getWorkbook() method public.  The second method was poo-pooed.

I can create some test cases in about a week.  I'm about to go AFK to a
conference and won't be able to get back to this in a while.
Comment 9 Eric Ladner 2005-05-16 17:47:32 UTC
Created attachment 15050 [details]
Test class that exhibits the problem

This java code exibits the problem with parsing 3d refs.
Comment 10 Eric Ladner 2005-05-16 17:48:46 UTC
Created attachment 15051 [details]
spreadsheet that shows the problem

this is a 3 sheet workbook that has a formula on sheet 3 that's a the sum of
sheet1!A1 and sheet2!A2.
Comment 11 Eric Ladner 2005-05-16 17:54:42 UTC
I attached two additional files that show the problem this patch is trying to
overcome.   If you run the TestClass on test.xls, it'll blow up in
FormulaParser, line 276.  This is because FormulaParser expects to be passed an
object of type Workbook so that it can resolve 3DRefs.  Line 276 is calling the
checkExternSheet() method of the Workbook class.  Since you can't get a Workbook
object from the HSSFWorkbook, it blows up.  The two solutions are (1) make
getWorkbook in Workbook public so you can get it and pass it to the
FormulaParser or (2) modify Formula parser to accept SheetReferences that are
obtained from the Workbook.

The patch attached to this bug implements option 2 above since exposing the
Workbook directly was generally regarded as a "bad idea".
Comment 12 Eric Ladner 2005-05-17 22:29:58 UTC
I've been trying to bring the patch foward and ran into an interesting
chicken/egg problem.  In every Ptg class, the toForumlaString method takes a
second parameter of type Workbook.  If there's no way to get a Workbook from an
HSSFWorkbook, none of those parameters can ever be filled with anything other
than null.  I was reworking the patch slightly from the mind-set that you can
never obtain an object of type Workbook to pass anywhere, so I was removing
references of Workbook from the FormulaParser.  Then I found all the Ptg's. 
From a quick analysis, it looks like it can safely be removed from all of them
(it's unused in 90% of them, only Area3DPtg and Ref3DPtg actually use it and
it's just to get the sheet references).  The only one I'm concerned about is the
NamePtg.

I'm kind of hesitant to produce a patch that touches that many files, but it
really does seem to me that the whole involvement of Workbook in the formula
related classes is completely useless and can never be utilized.  Period.

Should I continue?
Comment 13 Yegor Kozlov 2008-12-16 10:01:52 UTC
Try the latest POI-3.5-beta4, it supports 3DRefs.

Yegor