Merge pull request #2791 from PHPOffice/Named-Range-Alias-Cell-Collection

Experiment #143 in memory saving/performance in the Cell Collection

phpstan-baseline.neon

@@ -4180,16 +4180,6 @@ parameters:
 			count: 1
 			path: src/PhpSpreadsheet/Worksheet/Worksheet.php
 
-		-
-			message: "#^Parameter \\#1 \\$row of method PhpOffice\\\\PhpSpreadsheet\\\\Collection\\\\Cells\\:\\:removeRow\\(\\) expects string, int given\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Worksheet/Worksheet.php
-
-		-
-			message: "#^Parameter \\#1 \\$row of method PhpOffice\\\\PhpSpreadsheet\\\\Collection\\\\Cells\\:\\:removeRow\\(\\) expects string, int\\<1, max\\> given\\.$#"
-			count: 1
-			path: src/PhpSpreadsheet/Worksheet/Worksheet.php
-
 		-
 			message: "#^Parameter \\#2 \\$format of static method PhpOffice\\\\PhpSpreadsheet\\\\Style\\\\NumberFormat\\:\\:toFormattedString\\(\\) expects string, string\\|null given\\.$#"
 			count: 2

src/PhpSpreadsheet/Cell/Coordinate.php

@@ -47,15 +47,17 @@ abstract class Coordinate
      *
      * @param string $coordinates eg: 'A1', '$B$12'
      *
-     * @return array{0: int, 1: int} Array containing column index and row index (indexes 0 and 1)
+     * @return array{0: int, 1: int, 2: string} Array containing column and row index, and column string
      */
     public static function indexesFromString(string $coordinates): array
     {
-        [$col, $row] = self::coordinateFromString($coordinates);
+        [$column, $row] = self::coordinateFromString($coordinates);
+        $column = ltrim($column, '$');
 
         return [
-            self::columnIndexFromString(ltrim($col, '$')),
+            self::columnIndexFromString($column),
             (int) ltrim($row, '$'),
+            $column,
         ];
     }
 

src/PhpSpreadsheet/Collection/Cells.php

@@ -12,6 +12,8 @@ use Psr\SimpleCache\CacheInterface;
 
 class Cells
 {
+    protected const MAX_COLUMN_ID = 16384;
+
     /**
      * @var CacheInterface
      */
@@ -46,9 +48,10 @@ class Cells
     private $currentCellIsDirty = false;
 
     /**
-     * An index of existing cells. Booleans indexed by their coordinate.
+     * An index of existing cells. int pointer to the coordinate (0-base-indexed row * 16,384 + 1-base indexed column)
+     *    indexed by their coordinate.
      *
-     * @var bool[]
+     * @var int[]
      */
     private $index = [];
 
@@ -93,12 +96,7 @@ class Cells
      */
     public function has($cellCoordinate)
     {
-        if ($cellCoordinate === $this->currentCoordinate) {
+        return ($cellCoordinate === $this->currentCoordinate) || isset($this->index[$cellCoordinate]);
-            return true;
-        }
-
-        // Check if the requested entry exists in the index
-        return isset($this->index[$cellCoordinate]);
     }
 
     /**
@@ -150,15 +148,9 @@ class Cells
      */
     public function getSortedCoordinates()
     {
-        $sortKeys = [];
+        asort($this->index);
-        foreach ($this->getCoordinates() as $coordinate) {
-            sscanf($coordinate, '%[A-Z]%d', $column, $row);
-            $key = (--$row * 16384) + Coordinate::columnIndexFromString($column);
-            $sortKeys[$key] = $coordinate;
-        }
-        ksort($sortKeys);
 
-        return array_values($sortKeys);
+        return array_keys($this->index);
     }
 
     /**
@@ -203,21 +195,17 @@ class Cells
     public function getHighestRowAndColumn()
     {
         // Lookup highest column and highest row
-        $columns = ['1A'];
+        $maxRow = $maxColumn = 1;
-        $rows = [1];
+        foreach ($this->index as $coordinate) {
-        foreach ($this->getCoordinates() as $coordinate) {
+            $row = (int) floor($coordinate / self::MAX_COLUMN_ID) + 1;
-            sscanf($coordinate, '%[A-Z]%d', $column, $row);
+            $maxRow = ($maxRow > $row) ? $maxRow : $row;
-            $rows[$row] = $rows[$row] ?? $row;
+            $column = $coordinate % self::MAX_COLUMN_ID;
-            $columns[$column] = $columns[$column] ?? strlen($column) . $column;
+            $maxColumn = ($maxColumn > $column) ? $maxColumn : $column;
         }
 
-        // Determine highest column and row
-        $highestRow = max($rows);
-        $highestColumn = substr((string) max($columns), 1);
-
         return [
-            'row' => $highestRow,
+            'row' => $maxRow,
-            'column' => $highestColumn,
+            'column' => Coordinate::stringFromColumnIndex($maxColumn),
         ];
     }
 
@@ -235,17 +223,23 @@ class Cells
             return $this->getHighestRowAndColumn()['column'];
         }
 
-        $maxColumn = '1A';
+        $row = (int) $row;
-        foreach ($this->getCoordinates() as $coord) {
+        if ($row <= 0) {
-            sscanf($coord, '%[A-Z]%d', $c, $r);
+            throw new PhpSpreadsheetException('Row number must be a positive integer');
-            if ($r != $row) {
-                continue;
-            }
-            $sortableColum = strlen($c) . $c;
-            $maxColumn = $maxColumn > $sortableColum ? $maxColumn : $sortableColum;
         }
 
-        return substr($maxColumn, 1);
+        $maxColumn = 1;
+        $toRow = $row * self::MAX_COLUMN_ID;
+        $fromRow = --$row * self::MAX_COLUMN_ID;
+        foreach ($this->index as $coordinate) {
+            if ($coordinate < $fromRow || $coordinate >= $toRow) {
+                continue;
+            }
+            $column = $coordinate % self::MAX_COLUMN_ID;
+            $maxColumn = $maxColumn > $column ? $maxColumn : $column;
+        }
+
+        return Coordinate::stringFromColumnIndex($maxColumn);
     }
 
     /**
@@ -263,12 +257,13 @@ class Cells
         }
 
         $maxRow = 1;
-        foreach ($this->getCoordinates() as $coord) {
+        $columnIndex = Coordinate::columnIndexFromString($column);
-            sscanf($coord, '%[A-Z]%d', $c, $r);
+        foreach ($this->index as $coordinate) {
-            if ($c != $column) {
+            if ($coordinate % self::MAX_COLUMN_ID !== $columnIndex) {
                 continue;
             }
-            $maxRow = ($maxRow > $r) ? $maxRow : $r;
+            $row = (int) floor($coordinate / self::MAX_COLUMN_ID) + 1;
+            $maxRow = ($maxRow > $row) ? $maxRow : $row;
         }
 
         return $maxRow;
@@ -297,29 +292,18 @@ class Cells
         $newCollection = clone $this;
 
         $newCollection->parent = $worksheet;
-        if (is_object($newCollection->currentCell)) {
-            $newCollection->currentCell->attach($this);
-        }
-
-        // Get old values
-        $oldKeys = $newCollection->getAllCacheKeys();
-        $oldValues = $newCollection->cache->getMultiple($oldKeys);
-        $newValues = [];
-        $oldCachePrefix = $newCollection->cachePrefix;
-
-        // Change prefix
         $newCollection->cachePrefix = $newCollection->getUniqueID();
-        foreach ($oldValues as $oldKey => $value) {
-            /** @var string $newKey */
-            $newKey = str_replace($oldCachePrefix, $newCollection->cachePrefix, $oldKey);
-            $newValues[$newKey] = clone $value;
-        }
 
-        // Store new values
+        foreach ($this->index as $key => $value) {
-        $stored = $newCollection->cache->setMultiple($newValues);
+            $newCollection->index[$key] = $value;
+            $stored = $newCollection->cache->set(
+                $newCollection->cachePrefix . $key,
+                clone $this->cache->get($this->cachePrefix . $key)
+            );
             if ($stored === false) {
                 $this->destructIfNeeded($newCollection, 'Failed to copy cells in cache');
             }
+        }
 
         return $newCollection;
     }
@@ -327,14 +311,23 @@ class Cells
     /**
      * Remove a row, deleting all cells in that row.
      *
-     * @param string $row Row number to remove
+     * @param int|string $row Row number to remove
      */
     public function removeRow($row): void
     {
-        foreach ($this->getCoordinates() as $coord) {
+        $this->storeCurrentCell();
-            sscanf($coord, '%[A-Z]%d', $c, $r);
+        $row = (int) $row;
-            if ($r == $row) {
+        if ($row <= 0) {
-                $this->delete($coord);
+            throw new PhpSpreadsheetException('Row number must be a positive integer');
+        }
+
+        $toRow = $row * self::MAX_COLUMN_ID;
+        $fromRow = --$row * self::MAX_COLUMN_ID;
+        foreach ($this->index as $coordinate) {
+            if ($coordinate >= $fromRow && $coordinate < $toRow) {
+                $row = (int) floor($coordinate / self::MAX_COLUMN_ID) + 1;
+                $column = Coordinate::stringFromColumnIndex($coordinate % self::MAX_COLUMN_ID);
+                $this->delete("{$column}{$row}");
             }
         }
     }
@@ -346,10 +339,14 @@ class Cells
      */
     public function removeColumn($column): void
     {
-        foreach ($this->getCoordinates() as $coord) {
+        $this->storeCurrentCell();
-            sscanf($coord, '%[A-Z]%d', $c, $r);
+
-            if ($c == $column) {
+        $columnIndex = Coordinate::columnIndexFromString($column);
-                $this->delete($coord);
+        foreach ($this->index as $coordinate) {
+            if ($coordinate % self::MAX_COLUMN_ID === $columnIndex) {
+                $row = (int) floor($coordinate / self::MAX_COLUMN_ID) + 1;
+                $column = Coordinate::stringFromColumnIndex($coordinate % self::MAX_COLUMN_ID);
+                $this->delete("{$column}{$row}");
             }
         }
     }
@@ -394,7 +391,8 @@ class Cells
         if ($cellCoordinate !== $this->currentCoordinate) {
             $this->storeCurrentCell();
         }
-        $this->index[$cellCoordinate] = true;
+        sscanf($cellCoordinate, '%[A-Z]%d', $column, $row);
+        $this->index[$cellCoordinate] = (--$row * self::MAX_COLUMN_ID) + Coordinate::columnIndexFromString($column);
 
         $this->currentCoordinate = $cellCoordinate;
         $this->currentCell = $cell;
@@ -422,7 +420,7 @@ class Cells
             return null;
         }
 
-        // Check if the entry that has been requested actually exists
+        // Check if the entry that has been requested actually exists in the cache
         $cell = $this->cache->get($this->cachePrefix . $cellCoordinate);
         if ($cell === null) {
             throw new PhpSpreadsheetException("Cell entry {$cellCoordinate} no longer exists in cache. This probably means that the cache was cleared by someone else.");

src/PhpSpreadsheet/Worksheet/Worksheet.php

@@ -181,13 +181,6 @@ class Worksheet implements IComparable
      */
     private $conditionalStylesCollection = [];
 
-    /**
-     * Is the current cell collection sorted already?
-     *
-     * @var bool
-     */
-    private $cellCollectionIsSorted = false;
-
     /**
      * Collection of breaks.
      *
@@ -1336,24 +1329,22 @@ class Worksheet implements IComparable
      */
     public function createNewCell($coordinate)
     {
+        [$column, $row, $columnString] = Coordinate::indexesFromString($coordinate);
         $cell = new Cell(null, DataType::TYPE_NULL, $this);
         $this->cellCollection->add($coordinate, $cell);
-        $this->cellCollectionIsSorted = false;
 
         // Coordinates
-        [$column, $row] = Coordinate::coordinateFromString($coordinate);
+        if ($column > $this->cachedHighestColumn) {
-        $aIndexes = Coordinate::indexesFromString($coordinate);
+            $this->cachedHighestColumn = $column;
-        if ($this->cachedHighestColumn < $aIndexes[0]) {
-            $this->cachedHighestColumn = $aIndexes[0];
         }
-        if ($aIndexes[1] > $this->cachedHighestRow) {
+        if ($row > $this->cachedHighestRow) {
-            $this->cachedHighestRow = $aIndexes[1];
+            $this->cachedHighestRow = $row;
         }
 
         // Cell needs appropriate xfIndex from dimensions records
         //    but don't create dimension records if they don't already exist
         $rowDimension = $this->rowDimensions[$row] ?? null;
-        $columnDimension = $this->columnDimensions[$column] ?? null;
+        $columnDimension = $this->columnDimensions[$columnString] ?? null;
 
         if ($rowDimension !== null && $rowDimension->getXfIndex() > 0) {
             // then there is a row dimension with explicit style, assign it to the cell

tests/PhpSpreadsheetTests/Collection/CellsTest.php

@@ -38,18 +38,21 @@ class CellsTest extends TestCase
         $cell2 = $sheet->getCell('A1');
         self::assertSame($cell2, $collection->add('A1', $cell2), 'adding a second cell should return the cell');
         self::assertEquals(['B2', 'A1'], $collection->getCoordinates(), 'cell list should contains the second cell');
+        // Note that sorts orders the collection itself, so these cells will aread be ordered for the subsequent asserions
         self::assertEquals(['A1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the second cell');
 
         // Add a third cell
         $cell3 = $sheet->getCell('AA1');
         self::assertSame($cell3, $collection->add('AA1', $cell3), 'adding a third cell should return the cell');
-        self::assertEquals(['B2', 'A1', 'AA1'], $collection->getCoordinates(), 'cell list should contains the third cell');
+        self::assertEquals(['A1', 'B2', 'AA1'], $collection->getCoordinates(), 'cell list should contains the third cell');
+        // Note that sorts orders the collection itself, so these cells will aread be ordered for the subsequent asserions
         self::assertEquals(['A1', 'AA1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the third cell');
 
         // Add a fourth cell
         $cell4 = $sheet->getCell('Z1');
         self::assertSame($cell4, $collection->add('Z1', $cell4), 'adding a fourth cell should return the cell');
-        self::assertEquals(['B2', 'A1', 'AA1', 'Z1'], $collection->getCoordinates(), 'cell list should contains the fourth cell');
+        self::assertEquals(['A1', 'AA1', 'B2', 'Z1'], $collection->getCoordinates(), 'cell list should contains the fourth cell');
+        // Note that sorts orders the collection itself, so these cells will aread be ordered for the subsequent asserions
         self::assertEquals(['A1', 'Z1', 'AA1', 'B2'], $collection->getSortedCoordinates(), 'sorted cell list contains the fourth cell');
 
         // Assert collection copy
@@ -65,7 +68,7 @@ class CellsTest extends TestCase
         // Assert deletion
         $collection->delete('B2');
         self::assertFalse($collection->has('B2'), 'cell should have been deleted');
-        self::assertEquals(['A1', 'AA1', 'Z1'], $collection->getCoordinates(), 'cell list should still contains the A1,Z1 and A11 cells');
+        self::assertEquals(['A1', 'Z1', 'AA1'], $collection->getCoordinates(), 'cell list should still contains the A1,Z1 and A11 cells');
 
         // Assert update
         $cell2 = $sheet->getCell('A1');
@@ -74,7 +77,7 @@ class CellsTest extends TestCase
 
         $cell3 = $sheet->getCell('C3');
         self::assertSame($cell3, $collection->update($cell3), 'should silently add non-existing C3 cell');
-        self::assertEquals(['A1', 'AA1', 'Z1', 'C3'], $collection->getCoordinates(), 'cell list should contains the C3 cell');
+        self::assertEquals(['A1', 'Z1', 'AA1', 'C3'], $collection->getCoordinates(), 'cell list should contains the C3 cell');
     }
 
     public function testCacheLastCell(): void

tests/data/Cell/IndexesFromString.php

@@ -5,6 +5,7 @@ return [
         [
             1,
             1,
+            'A',
         ],
         'A1',
     ],
@@ -12,6 +13,7 @@ return [
         [
             1,
             12,
+            'A',
         ],
         'A12',
     ],
@@ -19,6 +21,7 @@ return [
         [
             10,
             1,
+            'J',
         ],
         'J1',
     ],
@@ -26,6 +29,7 @@ return [
         [
             10,
             20,
+            'J',
         ],
         'J20',
     ],
@@ -33,6 +37,7 @@ return [
         [
             35,
             1,
+            'AI',
         ],
         'AI1',
     ],
@@ -40,6 +45,7 @@ return [
         [
             35,
             2012,
+            'AI',
         ],
         'AI2012',
     ],
@@ -47,6 +53,7 @@ return [
         [
             2,
             3,
+            'B',
         ],
         'B3',
     ],
@@ -54,6 +61,7 @@ return [
         [
             2,
             3,
+            'B',
         ],
         '$B3',
     ],
@@ -61,6 +69,7 @@ return [
         [
             2,
             3,
+            'B',
         ],
         'B$3',
     ],
@@ -68,6 +77,7 @@ return [
         [
             2,
             3,
+            'B',
         ],
         '$B$3',
     ],