Bug 58909 - [PATCH] Performance problem on cloneSheet/setSheetName
Summary: [PATCH] Performance problem on cloneSheet/setSheetName
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: XSSF (show other bugs)
Version: 3.13-FINAL
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2016-01-22 10:38 UTC by Luca Martini
Modified: 2016-03-31 16:14 UTC (History)
1 user (show)



Attachments
proposed patch (5.19 KB, patch)
2016-01-22 10:38 UTC, Luca Martini
Details | Diff
proposed patch in tar.gz.format (1.51 KB, application/gzip)
2016-01-22 10:55 UTC, Luca Martini
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Luca Martini 2016-01-22 10:38:42 UTC
Created attachment 33476 [details]
proposed patch

Hi all,
I noticed a performance problem using the cloneSheet method of the XSSFWorbook class.
Currently, the cloneSheet generates an unique name for the cloned sheet.
However, it is common that the generated name has to be changed later to something more meaningful.
So, a typical use is the following:

XSSFSheet cloned = wb.cloneSheet(idx);
int origCopyIndex = wb.getSheetIndex(cloned);
wb.setSheetName(origCopyIndex, "OtherName");

In our application we use this pattern and we have noticed a huge performance hit in the setSheetName method because the setSheetName triggers the check of all the formulas in the workbook to update references to the sheet whose name was changed. When the workbook has a lot of cells this operation can last a significant amount of time.

However, this operation on a cloned sheet is completely not necessary because the generated name won't be in any formula in the workbook.

What I suggest is to add another version of the clonedSheet method for creating a cloned version with a given name. In this way, it is possible to avoid the setSheetName.

Something like:

/**
  * Create an XSSFSheet from an existing sheet in the XSSFWorkbook.
  *  The cloned sheet is a deep copy of the original but with a new given
  *  name.
  *
  * @return XSSFSheet representing the cloned sheet.
  * @throws IllegalArgumentException if the sheet index or the sheet
  *         name is invalid
  * @throws POIXMLException if there were errors when cloning
  */
  public XSSFSheet cloneSheet(int sheetNum, String newName)


I attach a patch that includes the newly added method on the XSSFWorkbook class and some unit tests. I already checked that all existing tests still succeed after the application of the patch.

You can evaluate if this method should be pull up to the Workbook interface (in this case it is necessary to provide implementations for the other formats).
Comment 1 Luca Martini 2016-01-22 10:55:57 UTC
Created attachment 33477 [details]
proposed patch in tar.gz.format

I'm sorry but in my previous comment I uploaded a patch in the Eclipse format. This new attachment is in the requested format (generated by the patch.xml ant script).
Comment 2 Dominik Stadler 2016-03-31 13:33:37 UTC
This was applied via r1737237 in time for the upcoming 3.15-beta1. For now only XSSF has this method. I have adjusted and combined the unit-tests somewhat. 

Thanks for the Patch!
Comment 3 Javen O'Neal 2016-03-31 16:14:05 UTC
Either patch format (or github pull request) is acceptable.