Bug 13294 - [PATCH] break should be continue in RowRecordsAggregate
Summary: [PATCH] break should be continue in RowRecordsAggregate
Alias: None
Product: POI
Classification: Unclassified
Component: HSSF (show other bugs)
Version: 2.0-dev
Hardware: All All
: P3 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2002-10-04 14:13 UTC by Paul Krause
Modified: 2004-11-16 19:05 UTC (History)
0 users


Note You need to log in before you can comment on or make changes to this bug.
Description Paul Krause 2002-10-04 14:13:11 UTC
As far as I can tell, this simply a typo.  Without this change, my spreadsheet 
is missing rows.

RCS file: /home/cvspublic/jakarta-
	retrieving revision 1.4
	diff -u -r1.4 RowRecordsAggregate.java
src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java5 Sep 
2002 00:26:26 -00001.4
src/java/org/apache/poi/hssf/record/aggregates/RowRecordsAggregate.java2 Oct 
2002 13:56:05 -0000
	@@ -145,7 +145,7 @@
	             if (!rec.isInValueSection() && !(rec instanceof 
	-                break;
	+                continue;
	             if (rec.getSid() == RowRecord.sid)
Comment 1 Andy Oliver 2002-10-14 20:13:10 UTC
please always put [PATCH] when sending patches...  Otherwise I generally will
miss it.  Also can you attach the patch?  Its horribly wrapped so I can't
actually apply it.  (click create attachment).  Full instructions are here:
Comment 2 Luc Girardin 2002-10-15 21:52:48 UTC
I can also confirm that RowRecordsAggregate is buggy (i.e. it corrupts files 
with page breaks). I tried this patch with no success (corruption got even 
worse). Understanding the concept behind the RowRecordsAggregate class is 
unfortunately beyond my abilities. Could someone shed light on this class?
Comment 3 Andy Oliver 2002-10-15 22:43:01 UTC
RowRecordsAggreagate collects RowRecord objects.  Its current purpose is for
performance (easy lookup of row records).  In the future it won't even store the
row records, just the data.....  

Aggregate records in general are conceptual records.  They aren't real records
but they support the interface.  So when asked to serialize, they generally
serialize more than one record (delegating the actual serialization to the
records they hold).

Consider it a Record collection + persistance.

Comment 4 Andy Oliver 2002-10-25 00:01:13 UTC
So should I apply this or not?  I heard both directions on the list?
Comment 5 Paul Krause 2002-10-25 14:59:54 UTC
I don't know whether this correct or not, just that it works for me.  What does 
Jason Height have to say about it?
Comment 6 Jason Height 2002-10-27 23:00:52 UTC
I only wrote the support for the sheet cloning mechanism. However just looking 
at the code, without properly understanding the why of it, seems to indicate 
that a continue is indeed correct.

However having said that, I think that you should attach a simple sheet and 
simple java source code example that exhibits the behaviour so that Andy can 
get a warm fuzzy feeling that this is indeed an error and the proposed solution 
is correct.

Even better would be a unit test that fails before your patch and then works 
after it.

Comment 7 Andy Oliver 2002-10-28 13:57:00 UTC
did we enter a mind meld at some point?   That sounds awesome.  Also please
attach the patch, (click create a new attachment and upload it) -- cut and
pasted patches can't be applied because they wrap and I'm too dull to figure out
how to unwrap them in a way the patch command likes.  ;-)
Comment 8 Paul Krause 2002-12-11 16:07:40 UTC
ValueRecordsAggregate does the same thing!
Comment 9 Avik Sengupta 2002-12-12 13:15:07 UTC
Andy, is this still relevant, after your changes? I believe the aggregate stuff
was part of the refactoring?
Comment 10 Amitabh 2003-01-30 02:46:40 UTC
Can any one please confirm whether this patch has been applied and on which 
nightly release.
Comment 11 Andy Oliver 2003-01-30 04:03:39 UTC
There was no unit test supplied and there have been several reports that it does
not solve a problem.  I prefer to see a more complete solution or to wait until
the performance branch re-merges with the head because it will solve this problem.
Comment 12 Danny Mui 2003-05-17 21:07:18 UTC
ended up aggregating all rows in a previous patch.  please verify it fixes your
issues.  These changes are in CVS head.