XSSFSheet.shiftRows: stop rebuilding every row from the XML on each shift (#1300)

https://bz.apache.org/bugzilla/show_bug.cgi?id=70139

Since the fix for bug 64516, rebuildRows() reordered the CTRow array of the
sheet (an XmlBeans deep copy of every row) and recreated every XSSFRow and
XSSFCell, twice per shiftRows call. Inserting single rows into a sheet with a
few thousand rows became orders of magnitude slower than POI 4, and the
XSSFRow/XSSFCell instances held by the caller were silently invalidated.

Shifting only renumbers rows, so the CTRow elements normally stay in order
and rebuildRows() now just refreshes the keys of the _rows map, keeping the
existing row and cell instances. It is a no-op when nothing was renumbered
(shiftColumns). The XML reorder plus recreation is kept for the rare case
where rows jump over other rows (bug 64516).

Recreating the rows also re-registered shared and array formulas via
onReadCell; that is now done explicitly by rebuildFormulaBookkeeping() at
the end of shiftRows/shiftColumns. It also uncovered that updateRowFormulas
shifted a shared formula ref twice when the sheet's registry holds the live
formula of a re-homed master (after the old master cell was deleted) rather
than a detached copy - the recreation used to paper over that.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: pjfanning

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java

@@ -3109,6 +3109,7 @@ public void shiftRows(int startRow, int endRow, final int n, boolean copyRowHeig
         rowShifter.updateHyperlinks(formulaShifter);
 
         rebuildRows();
+        rebuildFormulaBookkeeping();
 
         for (XSSFTable table : overlappingTables) {
             rebuildTableFormulas(table);
@@ -3147,6 +3148,7 @@ public void shiftColumns(int startColumn, int endColumn, final int n) {
         columnShifter.updateNamedRanges(formulaShifter);
 
         rebuildRows();
+        rebuildFormulaBookkeeping();
 
         for (XSSFTable table : overlappingTables) {
             rebuildTableFormulas(table);
@@ -3178,7 +3180,49 @@ private void rebuildTableFormulas(XSSFTable table) {
         }
     }
 
+    /**
+     * Brings the {@code _rows} map back in sync with the row numbers after rows were renumbered.
+     * <p>
+     * {@code _rows} is keyed by the row numbers the rows had before the shift, so it iterates the rows
+     * in the order of the CTRow elements in CTSheetData. Usually shifting keeps that order intact
+     * (rows are only renumbered), in which case only the keys of {@code _rows} need to be refreshed
+     * and the XSSFRow and XSSFCell instances stay valid. The XML only needs to be reordered (and the
+     * rows recreated) when rows jumped over other rows, see bug 64516.
+     */
     private void rebuildRows() {
+        XSSFRow[] rowArray = new XSSFRow[_rows.size()];
+        int[] rownums = new int[rowArray.length];
+        boolean renumbered = false;
+        boolean inOrder = true;
+        int i = 0;
+        for (Map.Entry<Integer, XSSFRow> entry : _rows.entrySet()) {
+            XSSFRow row = entry.getValue();
+            int rownum = row.getRowNum();
+            if (rownum != entry.getKey()) {
+                renumbered = true;
+            }
+            if (i > 0 && rownum <= rownums[i - 1]) {
+                inOrder = false;
+                break;
+            }
+            rowArray[i] = row;
+            rownums[i] = rownum;
+            i++;
+        }
+        if (!renumbered) {
+            return;
+        }
+        if (inOrder) {
+            _rows.clear();
+            for (i = 0; i < rowArray.length; i++) {
+                // Performance optimization: explicit boxing is slightly faster than auto-unboxing, though may use more memory
+                //noinspection UnnecessaryBoxing
+                final Integer rownumI = Integer.valueOf(rownums[i]); // NOSONAR
+                _rows.put(rownumI, rowArray[i]);
+            }
+            return;
+        }
+
         //rebuild the CTSheetData CTRow order
         SortedMap<Long, CTRow> ctRows = new TreeMap<>();
         CTSheetData sheetData = getCTWorksheet().getSheetData();
@@ -3200,6 +3244,18 @@ private void rebuildRows() {
         }
     }
 
+    /**
+     * The shared and array formula bookkeeping refers to cell addresses and formula ranges,
+     * so it is refreshed from the cells once they and their formulas were shifted.
+     */
+    private void rebuildFormulaBookkeeping() {
+        for (XSSFRow row : _rows.values()) {
+            for (Cell cell : row) {
+                onReadCell((XSSFCell) cell);
+            }
+        }
+    }
+
     // remove all rows which will be overwritten
     private void removeOverwritten(int startRow, int endRow, final int n) {
         XSSFVMLDrawing vml = getVMLDrawing(false);

poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/helpers/XSSFRowColShifter.java

@@ -113,8 +113,13 @@ static void updateNamedRanges(XSSFSheet sheet, FormulaShifter formulaShifter) {
                         if(f.getT() == STCellFormulaType.SHARED){
                             int si = Math.toIntExact(f.getSi());
                             CTCellFormula sf = sheet.getSharedFormula(si);
-                            sf.setStringValue(shiftedFormula);
-                            updateRefInCTCellFormula(row, formulaShifter, sf);
+                            // the sheet usually holds a detached copy of the master formula, but after
+                            // the master cell was deleted it holds the live formula of the new master,
+                            // which is this very formula and must not be shifted a second time
+                            if (sf != null && sf != f) {
+                                sf.setStringValue(shiftedFormula);
+                                updateRefInCTCellFormula(row, formulaShifter, sf);
+                            }
                         }
                     }
 

poi-ooxml/src/test/java/org/apache/poi/xssf/usermodel/TestXSSFSheetShiftRows.java

@@ -20,7 +20,9 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.util.stream.IntStream;
@@ -38,6 +40,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more
 import org.apache.poi.xssf.XSSFITestDataProvider;
 import org.apache.poi.xssf.XSSFTestDataSamples;
 import org.junit.jupiter.api.Test;
+import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTRow;
 
 public final class TestXSSFSheetShiftRows extends BaseTestSheetShiftRows {
 
@@ -525,6 +528,125 @@ void test60709() throws IOException {
         wb.close();
     }
 
+    // bug 70139: shifting rows rebuilt every row and cell from the XML, which was slow and
+    // invalidated the XSSFRow/XSSFCell instances the caller holds
+    @Test
+    void testShiftRowsKeepsRowAndCellInstances() throws IOException {
+        try (XSSFWorkbook wb = new XSSFWorkbook()) {
+            XSSFSheet sheet = wb.createSheet();
+            for (int r = 0; r < 10; r++) {
+                XSSFRow row = sheet.createRow(r);
+                row.createCell(0).setCellValue("r" + r);
+                row.setHeightInPoints(12 + r);
+            }
+            XSSFRow row3 = sheet.getRow(3);
+            XSSFCell cell3 = row3.getCell(0);
+            XSSFRow row9 = sheet.getRow(9);
+
+            sheet.shiftRows(3, 9, 2);
+
+            assertNull(sheet.getRow(3));
+            assertNull(sheet.getRow(4));
+            assertSame(row3, sheet.getRow(5));
+            assertSame(cell3, sheet.getRow(5).getCell(0));
+            assertEquals("r3", cell3.getStringCellValue());
+            assertEquals(5, cell3.getRowIndex());
+            assertEquals(15, row3.getHeightInPoints(), 0);
+            assertSame(row9, sheet.getRow(11));
+            assertEquals(11, sheet.getLastRowNum());
+            assertRowsInOrder(sheet);
+
+            // the shifted row can be used further
+            row3.createCell(1).setCellValue("added");
+            XSSFRow inserted = sheet.createRow(3);
+            inserted.createCell(0).setCellValue("inserted");
+            assertRowsInOrder(sheet);
+
+            try (XSSFWorkbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb)) {
+                XSSFSheet sheet2 = wb2.getSheetAt(0);
+                assertEquals("inserted", sheet2.getRow(3).getCell(0).getStringCellValue());
+                assertNull(sheet2.getRow(4));
+                assertEquals("r3", sheet2.getRow(5).getCell(0).getStringCellValue());
+                assertEquals("added", sheet2.getRow(5).getCell(1).getStringCellValue());
+                assertEquals(15, sheet2.getRow(5).getHeightInPoints(), 0);
+                assertEquals("r9", sheet2.getRow(11).getCell(0).getStringCellValue());
+                assertRowsInOrder(sheet2);
+            }
+        }
+    }
+
+    // rows jumping over other rows (bug 64516) still need the XML rows to be reordered
+    @Test
+    void testShiftRowsOverOtherRowsKeepsSheetDataInOrder() throws IOException {
+        try (XSSFWorkbook wb = new XSSFWorkbook()) {
+            XSSFSheet sheet = wb.createSheet();
+            for (int r = 0; r < 6; r++) {
+                sheet.createRow(r).createCell(0).setCellValue("r" + r);
+            }
+
+            // move row 5 on top of row 0, rows 1-4 are jumped over
+            sheet.shiftRows(5, 5, -5);
+
+            assertRowsInOrder(sheet);
+            assertEquals("r5", sheet.getRow(0).getCell(0).getStringCellValue());
+            assertEquals("r1", sheet.getRow(1).getCell(0).getStringCellValue());
+            assertNull(sheet.getRow(5));
+            sheet.removeRow(sheet.getRow(0));
+            assertEquals("r1", sheet.getRow(1).getCell(0).getStringCellValue());
+
+            try (XSSFWorkbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb)) {
+                XSSFSheet sheet2 = wb2.getSheetAt(0);
+                assertNull(sheet2.getRow(0));
+                assertEquals("r1", sheet2.getRow(1).getCell(0).getStringCellValue());
+                assertEquals("r4", sheet2.getRow(4).getCell(0).getStringCellValue());
+                assertRowsInOrder(sheet2);
+            }
+        }
+    }
+
+    @Test
+    void testShiftSharedFormulasTwice() throws Exception {
+        try (XSSFWorkbook wb = XSSFTestDataSamples.openSampleWorkbook("TestShiftRowSharedFormula.xlsx")) {
+            XSSFSheet sheet = wb.getSheetAt(0);
+            XSSFRow row5 = sheet.getRow(4);
+
+            sheet.shiftRows(3, sheet.getLastRowNum(), 1);
+            assertSame(row5, sheet.getRow(5));
+            assertEquals("SUM(C2:C5)", getCellFormula(sheet, "C6"));
+            assertEquals("SUM(E3:E6)", getCellFormula(sheet, "E7"));
+
+            sheet.shiftRows(3, sheet.getLastRowNum(), 2);
+            assertSame(row5, sheet.getRow(7));
+            assertEquals("SUM(C2:C7)", getCellFormula(sheet, "C8"));
+            assertEquals("SUM(D2:D7)", getCellFormula(sheet, "D8"));
+            assertEquals("SUM(E3:E8)", getCellFormula(sheet, "E9"));
+            assertRowsInOrder(sheet);
+
+            try (XSSFWorkbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb)) {
+                XSSFSheet sheet2 = wb2.getSheetAt(0);
+                assertEquals("SUM(C2:C7)", getCellFormula(sheet2, "C8"));
+                assertEquals("SUM(E3:E8)", getCellFormula(sheet2, "E9"));
+            }
+        }
+    }
+
+    private static void assertRowsInOrder(XSSFSheet sheet) {
+        long prev = 0;
+        int count = 0;
+        for (CTRow ctRow : sheet.getCTWorksheet().getSheetData().getRowList()) {
+            assertTrue(ctRow.getR() > prev, "row " + ctRow.getR() + " after row " + prev);
+            prev = ctRow.getR();
+            count++;
+        }
+        assertEquals(sheet.getPhysicalNumberOfRows(), count);
+        int i = 0;
+        for (Row row : sheet) {
+            assertSame(sheet.getRow(row.getRowNum()), row);
+            i++;
+        }
+        assertEquals(count, i);
+    }
+
     @Test
     public void testBug69154() throws Exception {
         // this does not appear to work for HSSF but let's get it working for XSSF anyway