Bug #61841 - Unnecessary long computation when evaluating VLOOKUP on all column reference

Found some optimizations in the general evaluation framework related to blank cells in rows beyond the last defined row of a sheet.

I don't see any issue with passing a bit of context down deeper into this framework, as it's all POI-internal and only had one calling path.

See the above bug for the performance analysis.  Not specifically related to VLOOKUP, but improves that case by more than 2/3 as well.

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

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

@@ -28,14 +28,25 @@ 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() {
         return _hs;
     }
+    
+    /* (non-Javadoc)
+     * @see org.apache.poi.ss.formula.EvaluationSheet#getlastRowNum()
+     * @since POI 4.0.0
+     */
+    public int getlastRowNum() {
+        return _lastDefinedRow;
+    }
+    
     @Override
     public EvaluationCell getCell(int rowIndex, int columnIndex) {
         HSSFRow row = _hs.getRow(rowIndex);
@@ -54,6 +65,6 @@ final class HSSFEvaluationSheet implements EvaluationSheet {
      */    
     @Override
     public void clearAllCachedResultValues() {
-        // nothing to do
+        _lastDefinedRow = _hs.getLastRowNum();
     }
 }

src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java

@@ -64,11 +64,11 @@ final class CellEvaluationFrame {
 		_sensitiveInputCells.toArray(result);
 		return result;
 	}
-	public void addUsedBlankCell(int bookIndex, int sheetIndex, int rowIndex, int columnIndex) {
+	public void addUsedBlankCell(EvaluationWorkbook evalWorkbook, int bookIndex, int sheetIndex, int rowIndex, int columnIndex) {
 		if (_usedBlankCellGroup == null) {
 			_usedBlankCellGroup = new FormulaUsedBlankCellSet();
 		}
-		_usedBlankCellGroup.addCell(bookIndex, sheetIndex, rowIndex, columnIndex);
+		_usedBlankCellGroup.addCell(evalWorkbook, bookIndex, sheetIndex, rowIndex, columnIndex);
 	}
 
 	public void updateFormulaResult(ValueEval result) {

src/java/org/apache/poi/ss/formula/EvaluationSheet.java

@@ -17,6 +17,7 @@
 
 package org.apache.poi.ss.formula;
 
+import org.apache.poi.ss.usermodel.Sheet;
 import org.apache.poi.util.Internal;
 
 /**
@@ -42,4 +43,10 @@ public interface EvaluationSheet {
      * @since POI 3.15 beta 3
      */
     public void clearAllCachedResultValues();
+    
+    /**
+     * @return last row index referenced on this sheet, for evaluation optimization
+     * @since POI 4.0.0
+     */
+    public int getlastRowNum();
 }

src/java/org/apache/poi/ss/formula/EvaluationTracker.java

@@ -131,7 +131,7 @@ final class EvaluationTracker {
 		}
 	}
 
-	public void acceptPlainValueDependency(int bookIndex, int sheetIndex,
+	public void acceptPlainValueDependency(EvaluationWorkbook evalWorkbook, int bookIndex, int sheetIndex,
 			int rowIndex, int columnIndex, ValueEval value) {
 		// Tell the currently evaluating cell frame that it has a dependency on the specified
 		int prevFrameIndex = _evaluationFrames.size() - 1;
@@ -140,7 +140,7 @@ final class EvaluationTracker {
 		} else {
 			CellEvaluationFrame consumingFrame = _evaluationFrames.get(prevFrameIndex);
 			if (value == BlankEval.instance) {
-				consumingFrame.addUsedBlankCell(bookIndex, sheetIndex, rowIndex, columnIndex);
+				consumingFrame.addUsedBlankCell(evalWorkbook, bookIndex, sheetIndex, rowIndex, columnIndex);
 			} else {
 				PlainValueCellCacheEntry cce = _cache.getPlainValueEntry(bookIndex, sheetIndex,
 						rowIndex, columnIndex, value);

src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java

@@ -57,13 +57,17 @@ final class FormulaUsedBlankCellSet {
 		private int _firstColumnIndex;
 		private int _lastColumnIndex;
 		private BlankCellRectangleGroup _currentRectangleGroup;
+        private int _lastDefinedRow;
 
-		public BlankCellSheetGroup() {
+		public BlankCellSheetGroup(int lastDefinedRow) {
 			_rectangleGroups = new ArrayList<>();
 			_currentRowIndex = -1;
+			_lastDefinedRow = lastDefinedRow;
 		}
 
 		public void addCell(int rowIndex, int columnIndex) {
+		    if (rowIndex > _lastDefinedRow) return;
+		    
 			if (_currentRowIndex == -1) {
 				_currentRowIndex = rowIndex;
 				_firstColumnIndex = columnIndex;
@@ -89,6 +93,8 @@ final class FormulaUsedBlankCellSet {
 		}
 
 		public boolean containsCell(int rowIndex, int columnIndex) {
+		    if (rowIndex > _lastDefinedRow) return true;
+		    
 			for (int i=_rectangleGroups.size()-1; i>=0; i--) {
 				BlankCellRectangleGroup bcrg = _rectangleGroups.get(i);
 				if (bcrg.containsCell(rowIndex, columnIndex)) {
@@ -167,17 +173,17 @@ final class FormulaUsedBlankCellSet {
 		_sheetGroupsByBookSheet = new HashMap<>();
 	}
 
-	public void addCell(int bookIndex, int sheetIndex, int rowIndex, int columnIndex) {
-		BlankCellSheetGroup sbcg = getSheetGroup(bookIndex, sheetIndex);
+	public void addCell(EvaluationWorkbook evalWorkbook, int bookIndex, int sheetIndex, int rowIndex, int columnIndex) {
+		BlankCellSheetGroup sbcg = getSheetGroup(evalWorkbook, bookIndex, sheetIndex);
 		sbcg.addCell(rowIndex, columnIndex);
 	}
 
-	private BlankCellSheetGroup getSheetGroup(int bookIndex, int sheetIndex) {
+	private BlankCellSheetGroup getSheetGroup(EvaluationWorkbook evalWorkbook, int bookIndex, int sheetIndex) {
 		BookSheetKey key = new BookSheetKey(bookIndex, sheetIndex);
 
 		BlankCellSheetGroup result = _sheetGroupsByBookSheet.get(key);
 		if (result == null) {
-			result = new BlankCellSheetGroup();
+			result = new BlankCellSheetGroup(evalWorkbook.getSheet(sheetIndex).getlastRowNum());
 			_sheetGroupsByBookSheet.put(key, result);
 		}
 		return result;

src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java

@@ -253,7 +253,7 @@ public final class WorkbookEvaluator {
         if (srcCell == null || srcCell.getCellType() != CellType.FORMULA) {
             ValueEval result = getValueFromNonFormulaCell(srcCell);
             if (shouldCellDependencyBeRecorded) {
-                tracker.acceptPlainValueDependency(_workbookIx, sheetIndex, rowIndex, columnIndex, result);
+                tracker.acceptPlainValueDependency(_workbook, _workbookIx, sheetIndex, rowIndex, columnIndex, result);
             }
             return result;
         }

src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java

@@ -42,6 +42,7 @@ import org.apache.poi.util.Internal;
 final class ForkedEvaluationSheet implements EvaluationSheet {
 
     private final EvaluationSheet _masterSheet;
+    
     /**
      * Only cells which have been split are put in this map.  (This has been done to conserve memory).
      */
@@ -52,6 +53,14 @@ final class ForkedEvaluationSheet implements EvaluationSheet {
         _sharedCellsByRowCol = new HashMap<>();
     }
     
+    /* (non-Javadoc)
+     * @see org.apache.poi.ss.formula.EvaluationSheet#getlastRowNum()
+     * @since POI 4.0.0
+     */
+    public int getlastRowNum() {
+        return _masterSheet.getlastRowNum();
+    }
+    
     @Override
     public EvaluationCell getCell(int rowIndex, int columnIndex) {
         RowColKey key = new RowColKey(rowIndex, columnIndex);

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

@@ -27,14 +27,25 @@ 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() {
         return _xs;
     }
+
+    /* (non-Javadoc)
+     * @see org.apache.poi.ss.formula.EvaluationSheet#getlastRowNum()
+     * @since POI 4.0.0
+     */
+    public int getlastRowNum() {
+        return _lastDefinedRow;
+    }
+    
     @Override
     public EvaluationCell getCell(int rowIndex, int columnIndex) {
         SXSSFRow row = _xs.getRow(rowIndex);
@@ -56,6 +67,6 @@ final class SXSSFEvaluationSheet implements EvaluationSheet {
      */
     @Override
     public void clearAllCachedResultValues() {
-        // nothing to do
+        _lastDefinedRow = _xs.getLastRowNum();
     }
 }

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

@@ -34,25 +34,40 @@ 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() {
         return _xs;
     }
 
+    /* (non-Javadoc)
+     * @see org.apache.poi.ss.formula.EvaluationSheet#getlastRowNum()
+     * @since POI 4.0.0
+     */
+    public int getlastRowNum() {
+        return _lastDefinedRow;
+    }
+    
     /* (non-JavaDoc), inherit JavaDoc from EvaluationWorkbook
      * @since POI 3.15 beta 3
      */
     @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;
+        
         // cache for performance: ~30% speedup due to caching
         if (_cellCache == null) {
             _cellCache = new HashMap<>(_xs.getLastRowNum() * 3);

src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java

@@ -0,0 +1,25 @@
+package org.apache.poi.ss.formula.functions;
+
+import org.apache.poi.ss.usermodel.CellType;
+import org.apache.poi.ss.usermodel.FormulaEvaluator;
+import org.apache.poi.ss.usermodel.Workbook;
+import org.apache.poi.xssf.XSSFTestDataSamples;
+import org.junit.Test;
+
+import junit.framework.TestCase;
+
+/**
+ * Test the VLOOKUP function
+ */
+public class TestVlookup extends TestCase {
+
+    @Test
+    public void testFullColumnAreaRef61841() {
+        final Workbook wb = XSSFTestDataSamples.openSampleWorkbook("VLookupFullColumn.xlsx");
+        FormulaEvaluator feval = wb.getCreationHelper().createFormulaEvaluator();
+        feval.evaluateAll();
+        assertEquals("Wrong lookup value",  "Value1", feval.evaluate(wb.getSheetAt(0).getRow(3).getCell(1)).getStringValue());
+        assertEquals("Lookup should return #N/A", CellType.ERROR, feval.evaluate(wb.getSheetAt(0).getRow(4).getCell(1)).getCellType());
+    }
+
+}