From e365e9cbacec917dcdde0c8db27cceb8b4dd798e Mon Sep 17 00:00:00 2001 From: Javen O'Neal Date: Sun, 31 Jan 2016 03:27:35 +0000 Subject: [PATCH] bug 58885: performance regression on XSSFSheet.addMergedRegion(CellRangeAddress). Add an unsafe version of addMergedRegion that doesn't check for overlapping merged regions git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1727776 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/poi/xssf/usermodel/XSSFSheet.java | 106 ++++++++++++++++-- .../poi/xssf/usermodel/TestXSSFSheet.java | 52 ++++++++- 2 files changed, 146 insertions(+), 12 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java index c0ea0681a..256ed3f5d 100644 --- a/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java +++ b/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFSheet.java @@ -295,24 +295,60 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } /** - * Adds a merged region of cells (hence those cells form one). + * Adds a merged region of cells on a sheet. * - * @param region (rowfrom/colfrom-rowto/colto) to merge + * @param region to merge * @return index of this region * @throws IllegalStateException if region intersects with a multi-cell array formula * @throws IllegalStateException if region intersects with an existing region on this sheet */ @Override public int addMergedRegion(CellRangeAddress region) { - region.validate(SpreadsheetVersion.EXCEL2007); + return addMergedRegion(region, true); + } - // throw IllegalStateException if the argument CellRangeAddress intersects with - // a multi-cell array formula defined in this sheet - validateArrayFormulas(region); + /** + * Adds a merged region of cells (hence those cells form one). + * Skips validation. It is possible to create overlapping merged regions + * or create a merged region that intersects a multi-cell array formula + * with this formula, which may result in a corrupt workbook. + * + * To check for merged regions overlapping array formulas or other merged regions + * after addMergedRegionUnsafe has been called, call {@link #validateArrayFormulas()} and {@link #validateMergedRegions()} + * + * @param region to merge + * @return index of this region + */ + public int addMergedRegionUnsafe(CellRangeAddress region) { + return addMergedRegion(region, false); + } + + /** + * Adds a merged region of cells (hence those cells form one). + * If validate is true, check to make sure adding the merged region to the sheet doesn't create a corrupt workbook + * If validate is false, skips the expensive merged region checks, but may produce a corrupt workbook. + * + * @param region to merge + * @param validate whether to validate merged region + * @return index of this region + * @throws IllegalStateException if region intersects with a multi-cell array formula + * @throws IllegalStateException if region intersects with an existing region on this sheet + */ + private int addMergedRegion(CellRangeAddress region, boolean validate) { + if (region.getNumberOfCells() < 2) { + throw new IllegalArgumentException("Merged region " + region.formatAsString() + " must contain 2 or more cells"); + } + region.validate(SpreadsheetVersion.EXCEL2007); - // Throw IllegalStateException if the argument CellRangeAddress intersects with - // a merged region already in this sheet - validateMergedRegions(region); + if (validate) { + // throw IllegalStateException if the argument CellRangeAddress intersects with + // a multi-cell array formula defined in this sheet + validateArrayFormulas(region); + + // Throw IllegalStateException if the argument CellRangeAddress intersects with + // a merged region already in this sheet + validateMergedRegions(region); + } CTMergeCells ctMergeCells = worksheet.isSetMergeCells() ? worksheet.getMergeCells() : worksheet.addNewMergeCells(); CTMergeCell ctMergeCell = ctMergeCells.addNewMergeCell(); @@ -331,6 +367,7 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { int firstColumn = region.getFirstColumn(); int lastRow = region.getLastRow(); int lastColumn = region.getLastColumn(); + // for each cell in sheet, if cell belongs to an array formula, check if merged region intersects array formula cells for (int rowIn = firstRow; rowIn <= lastRow; rowIn++) { for (int colIn = firstColumn; colIn <= lastColumn; colIn++) { XSSFRow row = getRow(rowIn); @@ -342,6 +379,7 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { if(cell.isPartOfArrayFormulaGroup()){ CellRangeAddress arrayRange = cell.getArrayFormulaRange(); if (arrayRange.getNumberOfCells() > 1 && + // region.intersects(arrayRange) is more concise and probably correct. Is it equivalent? ( arrayRange.isInRange(region.getFirstRow(), region.getFirstColumn()) || arrayRange.isInRange(region.getFirstRow(), region.getFirstColumn())) ){ String msg = "The range " + region.formatAsString() + " intersects with a multi-cell array formula. " + @@ -351,13 +389,26 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } } } - } + + /** + * Verify that none of the merged regions intersect a multi-cell array formula in this sheet + * + * @param region + * @throws IllegalStateException if candidate region intersects an existing array formula in this sheet + */ + private void checkForMergedRegionsIntersectingArrayFormulas() { + for (CellRangeAddress region : getMergedRegions()) { + validateArrayFormulas(region); + } + } + + /** * Verify that candidate region does not intersect with an existing merged region in this sheet * * @param candidateRegion - * @throws IllegalStateException if candidate region intersects an existing merged region in this sheet + * @throws IllegalStateException if candidate region intersects an existing merged region in this sheet (or candidateRegion is already merged in this sheet) */ private void validateMergedRegions(CellRangeAddress candidateRegion) { for (final CellRangeAddress existingRegion : getMergedRegions()) { @@ -368,6 +419,39 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet { } } + /** + * Verify that no merged regions intersect another merged region in this sheet. + * + * @throws IllegalStateException if at least one region intersects with another merged region in this sheet + */ + private void checkForIntersectingMergedRegions() { + final List regions = getMergedRegions(); + final int size = regions.size(); + for (int i=0; i < size; i++) { + final CellRangeAddress region = regions.get(i); + for (final CellRangeAddress other : regions.subList(i+1, regions.size())) { + if (region.intersects(other)) { + String msg = "The range " + region.formatAsString() + + " intersects with another merged region " + + other.formatAsString() + " in this sheet"; + throw new IllegalStateException(msg); + } + } + } + } + + /** + * Verify that merged regions do not intersect multi-cell array formulas and + * no merged regions intersect another merged region in this sheet. + * + * @throws IllegalStateException if region intersects with a multi-cell array formula + * @throws IllegalStateException if at least one region intersects with another merged region in this sheet + */ + public void validateMergedRegions() { + checkForMergedRegionsIntersectingArrayFormulas(); + checkForIntersectingMergedRegions(); + } + /** * Adjusts the column width to fit the contents. * diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheet.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheet.java index 2066d2d4a..158cc7501 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheet.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFSheet.java @@ -26,6 +26,8 @@ import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; import java.io.IOException; import java.util.Arrays; @@ -300,6 +302,54 @@ public final class TestXSSFSheet extends BaseTestSheet { workbook.close(); } + /** + * bug 58885: checking for overlapping merged regions when + * adding a merged region is safe, but runs in O(n). + * the check for merged regions when adding a merged region + * can be skipped (unsafe) and run in O(1). + */ + @Test + public void addMergedRegionUnsafe() throws IOException { + XSSFWorkbook wb = new XSSFWorkbook(); + XSSFSheet sh = wb.createSheet(); + CellRangeAddress region1 = CellRangeAddress.valueOf("A1:B2"); + CellRangeAddress region2 = CellRangeAddress.valueOf("B2:C3"); + CellRangeAddress region3 = CellRangeAddress.valueOf("C3:D4"); + CellRangeAddress region4 = CellRangeAddress.valueOf("J10:K11"); + assumeTrue(region1.intersects(region2)); + assumeTrue(region2.intersects(region3)); + + sh.addMergedRegionUnsafe(region1); + assertTrue(sh.getMergedRegions().contains(region1)); + + // adding a duplicate or overlapping merged region should not + // raise an exception with the unsafe version of addMergedRegion. + + sh.addMergedRegionUnsafe(region2); + + // the safe version of addMergedRegion should throw when trying to add a merged region that overlaps an existing region + assertTrue(sh.getMergedRegions().contains(region2)); + try { + sh.addMergedRegion(region3); + fail("Expected IllegalStateException. region3 overlaps already added merged region2."); + } catch (final IllegalStateException e) { + // expected + assertFalse(sh.getMergedRegions().contains(region3)); + } + // addMergedRegion should not re-validate previously-added merged regions + sh.addMergedRegion(region4); + + // validation methods should detect a problem with previously added merged regions (runs in O(n^2) time) + try { + sh.validateMergedRegions(); + fail("Expected validation to fail. Sheet contains merged regions A1:B2 and B2:C3, which overlap at B2."); + } catch (final IllegalStateException e) { + // expected + } + + wb.close(); + } + @Test public void setDefaultColumnStyle() throws IOException { XSSFWorkbook workbook = new XSSFWorkbook(); @@ -1901,4 +1951,4 @@ public final class TestXSSFSheet extends BaseTestSheet { assertEquals("B2:D4", ignoredErrors.get(IgnoredErrorType.EVALUATION_ERROR).iterator().next().formatAsString()); workbook.close(); } -} \ No newline at end of file +}