| Summary: | [PATCH] break should be continue in RowRecordsAggregate | ||
|---|---|---|---|
| Product: | POI | Reporter: | Paul Krause <pkrause> |
| Component: | HSSF | Assignee: | POI Developers List <dev> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | P3 | ||
| Version: | 2.0-dev | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
|
Description
Paul Krause
2002-10-04 14:13:11 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: http://jakarta.apache.org/poi/getinvolved/index.html 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? 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. -Andy So should I apply this or not? I heard both directions on the list? I don't know whether this correct or not, just that it works for me. What does Jason Height have to say about it? 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. Jason 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. ;-) ValueRecordsAggregate does the same thing! Andy, is this still relevant, after your changes? I believe the aggregate stuff was part of the refactoring? Hi, Can any one please confirm whether this patch has been applied and on which nightly release. Thanks Amitabh 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. ended up aggregating all rows in a previous patch. please verify it fixes your issues. These changes are in CVS head. |