Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id D4313200D5D for ; Wed, 6 Dec 2017 01:15:57 +0100 (CET) Received: by cust-asf.ponee.io (Postfix) id D2B6D160C1C; Wed, 6 Dec 2017 00:15:57 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id C9FC1160C1B for ; Wed, 6 Dec 2017 01:15:56 +0100 (CET) Received: (qmail 64882 invoked by uid 500); 6 Dec 2017 00:15:56 -0000 Mailing-List: contact commits-help@poi.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@poi.apache.org Delivered-To: mailing list commits@poi.apache.org Received: (qmail 64873 invoked by uid 99); 6 Dec 2017 00:15:56 -0000 Received: from Unknown (HELO svn01-us-west.apache.org) (209.188.14.144) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 06 Dec 2017 00:15:56 +0000 Received: from svn01-us-west.apache.org (localhost [127.0.0.1]) by svn01-us-west.apache.org (ASF Mail Server at svn01-us-west.apache.org) with ESMTP id 7F4A93A015D for ; Wed, 6 Dec 2017 00:15:53 +0000 (UTC) Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit 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 -0000 To: commits@poi.apache.org From: gwoolsey@apache.org X-Mailer: svnmailer-1.0.9 Message-Id: <20171206001554.7F4A93A015D@svn01-us-west.apache.org> archived-at: Wed, 06 Dec 2017 00:15:58 -0000 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 _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