Bug 62993: XSSFEvaluationSheet now retrieves valid last row index from underlying XSSFSheet. Thanks to Axel Howind.

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1850212 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java

@@ -28,11 +28,9 @@ import org.apache.poi.util.Internal;
 final class HSSFEvaluationSheet implements EvaluationSheet {
 
     private final HSSFSheet _hs;
-    private int _lastDefinedRow = -1;
 
     public HSSFEvaluationSheet(HSSFSheet hs) {
         _hs = hs;
-        _lastDefinedRow = _hs.getLastRowNum();
     }
 
     public HSSFSheet getHSSFSheet() {
@@ -45,7 +43,7 @@ final class HSSFEvaluationSheet implements EvaluationSheet {
      */
     @Override
     public int getLastRowNum() {
-        return _lastDefinedRow;
+        return _hs.getLastRowNum();
     }
     
     @Override
@@ -66,6 +64,5 @@ final class HSSFEvaluationSheet implements EvaluationSheet {
      */    
     @Override
     public void clearAllCachedResultValues() {
-        _lastDefinedRow = _hs.getLastRowNum();
     }
 }

src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java

@@ -27,11 +27,9 @@ import org.apache.poi.util.Internal;
 @Internal
 final class SXSSFEvaluationSheet implements EvaluationSheet {
     private final SXSSFSheet _xs;
-    private int _lastDefinedRow = -1;
 
     public SXSSFEvaluationSheet(SXSSFSheet sheet) {
         _xs = sheet;
-        _lastDefinedRow = _xs.getLastRowNum();
     }
 
     public SXSSFSheet getSXSSFSheet() {
@@ -44,7 +42,7 @@ final class SXSSFEvaluationSheet implements EvaluationSheet {
      */
     @Override
     public int getLastRowNum() {
-        return _lastDefinedRow;
+        return _xs.getLastRowNum();
     }
     
     @Override
@@ -68,6 +66,5 @@ final class SXSSFEvaluationSheet implements EvaluationSheet {
      */
     @Override
     public void clearAllCachedResultValues() {
-        _lastDefinedRow = _xs.getLastRowNum();
     }
 }

src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java

@@ -34,11 +34,9 @@ final class XSSFEvaluationSheet implements EvaluationSheet {
 
     private final XSSFSheet _xs;
     private Map<CellKey, EvaluationCell> _cellCache;
-    private int _lastDefinedRow = -1;
 
     public XSSFEvaluationSheet(XSSFSheet sheet) {
         _xs = sheet;
-        _lastDefinedRow = _xs.getLastRowNum();
     }
 
     public XSSFSheet getXSSFSheet() {
@@ -51,7 +49,7 @@ final class XSSFEvaluationSheet implements EvaluationSheet {
      */
     @Override
     public int getLastRowNum() {
-        return _lastDefinedRow;
+        return _xs.getLastRowNum();
     }
     
     /* (non-JavaDoc), inherit JavaDoc from EvaluationWorkbook
@@ -60,15 +58,16 @@ final class XSSFEvaluationSheet implements EvaluationSheet {
     @Override
     public void clearAllCachedResultValues() {
         _cellCache = null;
-        _lastDefinedRow = _xs.getLastRowNum();
     }
     
     @Override
     public EvaluationCell getCell(int rowIndex, int columnIndex) {
         // shortcut evaluation if reference is outside the bounds of existing data
         // see issue #61841 for impact on VLOOKUP in particular
-        if (rowIndex > _lastDefinedRow) return null;
-        
+        if (rowIndex > getLastRowNum()) {
+            return null;
+        }
+
         // cache for performance: ~30% speedup due to caching
         if (_cellCache == null) {
             _cellCache = new HashMap<>(_xs.getLastRowNum() * 3);

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

@@ -27,7 +27,6 @@ import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
@@ -131,7 +130,6 @@ import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTSheetViews;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTTablePart;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTTableParts;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTWorksheet;
-import org.openxmlformats.schemas.spreadsheetml.x2006.main.CTWorksheetSource;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.STCalcMode;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.STCellFormulaType;
 import org.openxmlformats.schemas.spreadsheetml.x2006.main.STPane;
@@ -1211,6 +1209,10 @@ public class XSSFSheet extends POIXMLDocumentPart implements Sheet  {
 
     @Override
     public int getLastRowNum() {
+        // _rows.getLastKey() (O(logN)) or caching last row (O(1))?
+        // A test with 1_000_000 rows shows that querying getLastRowNum with lastKey() implementation takes ~40 ms,
+        // and ~1.2 ms with cached implementation. 40 ms is negligible compared to the time of evaluation a million
+        // cells, and the lastKey implementation is much more elegant and less error prone than caching.
         return _rows.isEmpty() ? 0 : _rows.lastKey();
     }
 

src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFEvaluationSheet.java

@@ -0,0 +1,32 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+package org.apache.poi.xssf.streaming;
+
+import org.apache.poi.ss.formula.EvaluationSheet;
+import org.apache.poi.ss.usermodel.BaseTestXEvaluationSheet;
+import org.apache.poi.ss.usermodel.Sheet;
+
+import java.util.AbstractMap;
+import java.util.Map;
+
+public class TestSXSSFEvaluationSheet extends BaseTestXEvaluationSheet {
+    @Override
+    protected Map.Entry<Sheet, EvaluationSheet> getInstance() {
+        SXSSFSheet sheet = new SXSSFWorkbook().createSheet();
+        return new AbstractMap.SimpleEntry<>(sheet, new SXSSFEvaluationSheet(sheet));
+    }
+}

src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFEvaluationSheet.java

@@ -16,11 +16,19 @@
 ==================================================================== */
 package org.apache.poi.xssf.usermodel;
 
+import org.apache.poi.ss.formula.EvaluationSheet;
+import org.apache.poi.ss.usermodel.BaseTestXEvaluationSheet;
+import org.apache.poi.ss.usermodel.Sheet;
 import org.junit.Test;
 
+import java.util.AbstractMap;
+import java.util.Map;
+
 import static org.junit.Assert.*;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
-public class TestXSSFEvaluationSheet {
+public class TestXSSFEvaluationSheet extends BaseTestXEvaluationSheet {
 
     @Test
     public void test() throws Exception {
@@ -52,4 +60,10 @@ public class TestXSSFEvaluationSheet {
         // other things
         assertEquals(sheet, evalsheet.getXSSFSheet());
     }
-}
+
+    @Override
+    protected Map.Entry<Sheet, EvaluationSheet> getInstance() {
+        XSSFSheet sheet = new XSSFWorkbook().createSheet();
+        return new AbstractMap.SimpleEntry<>(sheet, new XSSFEvaluationSheet(sheet));
+    }
+}

src/testcases/org/apache/poi/hssf/usermodel/TestHSSFEvaluationSheet.java

@@ -0,0 +1,33 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.hssf.usermodel;
+
+import org.apache.poi.ss.formula.EvaluationSheet;
+import org.apache.poi.ss.usermodel.BaseTestXEvaluationSheet;
+import org.apache.poi.ss.usermodel.Sheet;
+
+import java.util.AbstractMap;
+import java.util.Map;
+
+public class TestHSSFEvaluationSheet extends BaseTestXEvaluationSheet {
+    @Override
+    protected Map.Entry<Sheet, EvaluationSheet> getInstance() {
+        HSSFSheet sheet = new HSSFWorkbook().createSheet();
+        return new AbstractMap.SimpleEntry<>(sheet, new HSSFEvaluationSheet(sheet));
+    }
+}

src/testcases/org/apache/poi/ss/usermodel/BaseTestXEvaluationSheet.java

@@ -0,0 +1,49 @@
+/* ====================================================================
+   Licensed to the Apache Software Foundation (ASF) under one or more
+   contributor license agreements.  See the NOTICE file distributed with
+   this work for additional information regarding copyright ownership.
+   The ASF licenses this file to You under the Apache License, Version 2.0
+   (the "License"); you may not use this file except in compliance with
+   the License.  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+   Unless required by applicable law or agreed to in writing, software
+   distributed under the License is distributed on an "AS IS" BASIS,
+   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+   See the License for the specific language governing permissions and
+   limitations under the License.
+==================================================================== */
+
+package org.apache.poi.ss.usermodel;
+
+import org.apache.poi.ss.formula.EvaluationSheet;
+import org.junit.Test;
+
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+
+public abstract class BaseTestXEvaluationSheet {
+    /**
+     * Get a pair of underlying sheet and evaluation sheet.
+     */
+    protected abstract Map.Entry<Sheet, EvaluationSheet> getInstance();
+
+    @Test
+    public void lastRowNumIsUpdatedFromUnderlyingSheet_bug62993() {
+        Map.Entry<Sheet, EvaluationSheet> sheetPair = getInstance();
+        Sheet underlyingSheet = sheetPair.getKey();
+        EvaluationSheet instance = sheetPair.getValue();
+
+        assertEquals(0, instance.getLastRowNum());
+
+        underlyingSheet.createRow(0);
+        underlyingSheet.createRow(1);
+        underlyingSheet.createRow(2);
+        assertEquals(2, instance.getLastRowNum());
+
+        underlyingSheet.removeRow(underlyingSheet.getRow(2));
+        assertEquals(1, instance.getLastRowNum());
+    }
+}