poi-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gwool...@apache.org
Subject svn commit: r1817252 - in /poi/trunk: src/java/org/apache/poi/hssf/usermodel/ src/java/org/apache/poi/ss/formula/ src/java/org/apache/poi/ss/formula/eval/forked/ src/ooxml/java/org/apache/poi/xssf/streaming/ src/ooxml/java/org/apache/poi/xssf/usermodel...
Date Wed, 06 Dec 2017 00:15:52 GMT
Author: gwoolsey
Date: Wed Dec  6 00:15:51 2017
New Revision: 1817252

URL: http://svn.apache.org/viewvc?rev=1817252&view=rev
Log:
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.

Added:
    poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java   (with
props)
    poi/trunk/test-data/spreadsheet/VLookupFullColumn.xlsx   (with props)
Modified:
    poi/trunk/src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java
    poi/trunk/src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java
    poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationSheet.java
    poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java
    poi/trunk/src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java
    poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java
    poi/trunk/src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java
    poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java
    poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java

Modified: poi/trunk/src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java (original)
+++ poi/trunk/src/java/org/apache/poi/hssf/usermodel/HSSFEvaluationSheet.java Wed Dec  6 00:15:51
2017
@@ -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 implemen
      */    
     @Override
     public void clearAllCachedResultValues() {
-        // nothing to do
+        _lastDefinedRow = _hs.getLastRowNum();
     }
 }

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/CellEvaluationFrame.java Wed Dec  6 00:15:51
2017
@@ -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) {

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationSheet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationSheet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationSheet.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationSheet.java Wed Dec  6 00:15:51
2017
@@ -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();
 }

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/EvaluationTracker.java Wed Dec  6 00:15:51
2017
@@ -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);

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/FormulaUsedBlankCellSet.java Wed Dec  6 00:15:51
2017
@@ -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;

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/WorkbookEvaluator.java Wed Dec  6 00:15:51
2017
@@ -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;
         }

Modified: poi/trunk/src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java (original)
+++ poi/trunk/src/java/org/apache/poi/ss/formula/eval/forked/ForkedEvaluationSheet.java Wed
Dec  6 00:15:51 2017
@@ -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).
      */
@@ -51,7 +52,15 @@ final class ForkedEvaluationSheet implem
         _masterSheet = masterSheet;
         _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);

Modified: poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java (original)
+++ poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFEvaluationSheet.java Wed Dec
 6 00:15:51 2017
@@ -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 impleme
      */
     @Override
     public void clearAllCachedResultValues() {
-        // nothing to do
+        _lastDefinedRow = _xs.getLastRowNum();
     }
 }

Modified: poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java?rev=1817252&r1=1817251&r2=1817252&view=diff
==============================================================================
--- poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java (original)
+++ poi/trunk/src/ooxml/java/org/apache/poi/xssf/usermodel/XSSFEvaluationSheet.java Wed Dec
 6 00:15:51 2017
@@ -34,25 +34,40 @@ final class XSSFEvaluationSheet implemen
 
     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);

Added: poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java
URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java?rev=1817252&view=auto
==============================================================================
--- poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java (added)
+++ poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java Wed
Dec  6 00:15:51 2017
@@ -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());
+    }
+
+}

Propchange: poi/trunk/src/ooxml/testcases/org/apache/poi/ss/formula/functions/TestVlookup.java
------------------------------------------------------------------------------
    svn:eol-style = native

Added: poi/trunk/test-data/spreadsheet/VLookupFullColumn.xlsx
URL: http://svn.apache.org/viewvc/poi/trunk/test-data/spreadsheet/VLookupFullColumn.xlsx?rev=1817252&view=auto
==============================================================================
Binary file - no diff available.

Propchange: poi/trunk/test-data/spreadsheet/VLookupFullColumn.xlsx
------------------------------------------------------------------------------
    svn:mime-type = application/octet-stream



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org


Mime
View raw message