MAGETWO-66331: Implement solution to update duplicate rows at once and add environment variable to control batch size

- implementing solution, updating tests

Author: Igor Melnikov

app/etc/di.xml

@@ -1239,4 +1239,9 @@
             </argument>
         </arguments>
     </type>
+    <type name="Magento\Framework\DB\FieldDataConverter">
+        <arguments>
+            <argument name="envBatchSize" xsi:type="init_parameter">Magento\Framework\DB\FieldDataConverter::BATCH_SIZE_VARIABLE_NAME</argument>
+        </arguments>
+    </type>
 </config>

dev/tests/integration/testsuite/Magento/Framework/DB/DataConverter/DataConverterTest.php

@@ -8,17 +8,16 @@
 use Magento\Framework\DB\Adapter\Pdo\Mysql;
 use Magento\Framework\DB\FieldDataConverter;
 use Magento\Framework\DB\Select;
+use Magento\Framework\DB\Select\QueryModifierInterface;
 use Magento\Framework\DB\Select\InQueryModifier;
 use Magento\Framework\Serialize\Serializer\Serialize;
 use Magento\TestFramework\Helper\Bootstrap;
+use Magento\Framework\DB\Query\Generator;
+use Magento\Framework\DB\Query\BatchIterator;
+use Magento\Framework\ObjectManagerInterface;
 
 class DataConverterTest extends \PHPUnit_Framework_TestCase
 {
-    /**
-     * @var \Magento\Framework\ObjectManagerInterface
-     */
-    private $objectManager;
-
     /**
      * @var InQueryModifier|\PHPUnit_Framework_MockObject_MockObject
      */
@@ -30,12 +29,12 @@ class DataConverterTest extends \PHPUnit_Framework_TestCase
     private $dataConverter;
 
     /**
-     * @var \Magento\Framework\DB\Query\BatchIterator|\PHPUnit_Framework_MockObject_MockObject
+     * @var BatchIterator|\PHPUnit_Framework_MockObject_MockObject
      */
     private $iteratorMock;
 
     /**
-     * @var \Magento\Framework\DB\Query\Generator|\PHPUnit_Framework_MockObject_MockObject
+     * @var Generator|\PHPUnit_Framework_MockObject_MockObject
      */
     private $queryGeneratorMock;
 
@@ -54,6 +53,11 @@ class DataConverterTest extends \PHPUnit_Framework_TestCase
      */
     private $fieldDataConverter;
 
+    /**
+     * @var ObjectManagerInterface
+     */
+    private $objectManager;
+
     /**
      * Set up before test
      */
@@ -62,23 +66,24 @@ protected function setUp()
         $this->objectManager = Bootstrap::getObjectManager();
 
         /** @var InQueryModifier $queryModifier */
-        $this->queryModifierMock = $this->getMockBuilder(Select\QueryModifierInterface::class)
+        $this->queryModifierMock = $this->getMockBuilder(QueryModifierInterface::class)
             ->disableOriginalConstructor()
             ->setMethods(['modify'])
             ->getMock();
 
         $this->dataConverter = $this->objectManager->get(SerializedToJson::class);
 
-        $this->iteratorMock = $this->getMockBuilder(\Magento\Framework\DB\Query\BatchIterator::class)
+        $this->iteratorMock = $this->getMockBuilder(BatchIterator::class)
             ->disableOriginalConstructor()
             ->setMethods(['current', 'valid', 'next'])
             ->getMock();
 
         $iterationComplete = false;
 
         // mock valid() call so iterator passes only current() result in foreach invocation
-        $this->iteratorMock->expects($this->any())->method('valid')->will(
-            $this->returnCallback(
+        $this->iteratorMock->expects($this->any())
+            ->method('valid')
+            ->willReturnCallback(
                 function () use (&$iterationComplete) {
                     if (!$iterationComplete) {
                         $iterationComplete = true;
@@ -87,10 +92,9 @@ function () use (&$iterationComplete) {
                         return false;
                     }
                 }
-            )
-        );
+            );
 
-        $this->queryGeneratorMock = $this->getMockBuilder(\Magento\Framework\DB\Query\Generator::class)
+        $this->queryGeneratorMock = $this->getMockBuilder(Generator::class)
             ->disableOriginalConstructor()
             ->setMethods(['generate'])
             ->getMock();
@@ -111,7 +115,7 @@ function () use (&$iterationComplete) {
 
         $this->adapterMock = $this->getMockBuilder(Mysql::class)
             ->disableOriginalConstructor()
-            ->setMethods(['fetchAll', 'quoteInto', 'update'])
+            ->setMethods(['fetchPairs', 'quoteInto', 'update'])
             ->getMock();
 
         $this->adapterMock->expects($this->any())
@@ -129,6 +133,7 @@ function () use (&$iterationComplete) {
 
     /**
      * Test that exception with valid text is thrown when data is corrupted
+     *
      * @expectedException \Magento\Framework\DB\FieldDataConversionException
      * @expectedExceptionMessage Error converting field `value` in table `table` where `id`=2 using
      */
@@ -140,18 +145,18 @@ public function testDataConvertErrorReporting()
         $serializedDataLength = strlen($serializedData);
         $brokenSerializedData = substr($serializedData, 0, $serializedDataLength - 6);
         $rows = [
-            ['id' => 1, 'value' => 'N;'],
-            ['id' => 2, 'value' => $brokenSerializedData],
+            1 => 'N;',
+            2 => $brokenSerializedData,
         ];
 
         $this->adapterMock->expects($this->any())
-            ->method('fetchAll')
+            ->method('fetchPairs')
             ->with($this->selectByRangeMock)
             ->will($this->returnValue($rows));
 
         $this->adapterMock->expects($this->once())
             ->method('update')
-            ->with('table', ['value' => 'null'], ['id = ?' => 1]);
+            ->with('table', ['value' => 'null'], ['id IN (?)' => [1]]);
 
         $this->fieldDataConverter->convert($this->adapterMock, 'table', 'id', 'value', $this->queryModifierMock);
     }
@@ -161,23 +166,23 @@ public function testDataConvertErrorReporting()
     public function testAlreadyConvertedDataSkipped()
     {
         $rows = [
-            ['id' => 2, 'value' => '[]'],
-            ['id' => 3, 'value' => '{}'],
-            ['id' => 4, 'value' => 'null'],
-            ['id' => 5, 'value' => '""'],
-            ['id' => 6, 'value' => '0'],
-            ['id' => 7, 'value' => 'N;'],
-            ['id' => 8, 'value' => '{"valid": "json value"}'],
+            2 => '[]',
+            3 => '{}',
+            4 => 'null',
+            5 => '""',
+            6 => '0',
+            7 => 'N;',
+            8 => '{"valid": "json value"}',
         ];
 
         $this->adapterMock->expects($this->any())
-            ->method('fetchAll')
+            ->method('fetchPairs')
             ->with($this->selectByRangeMock)
             ->will($this->returnValue($rows));
 
         $this->adapterMock->expects($this->once())
             ->method('update')
-            ->with('table', ['value' => 'null'], ['id = ?' => 7]);
+            ->with('table', ['value' => 'null'], ['id IN (?)' => [7]]);
 
         $this->fieldDataConverter->convert($this->adapterMock, 'table', 'id', 'value', $this->queryModifierMock);
     }

lib/internal/Magento/Framework/DB/FieldDataConverter.php

@@ -5,7 +5,6 @@
  */
 namespace Magento\Framework\DB;
 
-use Magento\Framework\App\ObjectManager;
 use Magento\Framework\DB\Adapter\AdapterInterface;
 use Magento\Framework\DB\DataConverter\DataConversionException;
 use Magento\Framework\DB\Query\Generator;
@@ -17,6 +16,26 @@
  */
 class FieldDataConverter
 {
+    /**
+     * Batch size env variable name
+     */
+    const BATCH_SIZE_VARIABLE_NAME = 'DATA_CONVERTER_BATCH_SIZE';
+
+    /**
+     * Default batch size
+     */
+    const DEFAULT_BATCH_SIZE = 50000;
+
+    /**
+     * Min batch size
+     */
+    const MIN_BATCH_SIZE = 25000;
+
+    /**
+     * Max batch size
+     */
+    const MAX_BATCH_SIZE = 500000;
+
     /**
      * @var Generator
      */
@@ -32,25 +51,33 @@ class FieldDataConverter
      */
     private $selectFactory;
 
+    /**
+     * @var string|null
+     */
+    private $envBatchSize;
+
     /**
      * Constructor
      *
      * @param Generator $queryGenerator
      * @param DataConverterInterface $dataConverter
-     * @param SelectFactory $selectFactory
+     * @param SelectFactory|null $selectFactory
+     * @param string $envBatchSize
      */
     public function __construct(
         Generator $queryGenerator,
         DataConverterInterface $dataConverter,
-        SelectFactory $selectFactory = null
+        SelectFactory $selectFactory,
+        $envBatchSize
     ) {
         $this->queryGenerator = $queryGenerator;
         $this->dataConverter = $dataConverter;
-        $this->selectFactory = $selectFactory ?: ObjectManager::getInstance()->get(SelectFactory::class);
+        $this->selectFactory = $selectFactory;
+        $this->envBatchSize = $envBatchSize;
     }
 
     /**
-     * Convert table field data from one representation to another uses DataConverterInterface
+     * Convert table field data from one representation to another
      *
      * @param AdapterInterface $connection
      * @param string $table
@@ -73,18 +100,20 @@ public function convert(
         if ($queryModifier) {
             $queryModifier->modify($select);
         }
-        $iterator = $this->queryGenerator->generate($identifier, $select);
+        $iterator = $this->queryGenerator->generate($identifier, $select, $this->getBatchSize());
         foreach ($iterator as $selectByRange) {
-            $rows = $connection->fetchAll($selectByRange);
-            foreach ($rows as $row) {
+            $rows = $connection->fetchPairs($selectByRange);
+            $uniqueFieldDataArray = array_unique($rows);
+            foreach ($uniqueFieldDataArray as $uniqueFieldData) {
+                $ids = array_keys($rows, $uniqueFieldData);
                 try {
-                    $convertedValue = $this->dataConverter->convert($row[$field]);
-                    if ($row[$field] === $convertedValue) {
-                        // skip for data rows that have been already converted
+                    $convertedValue = $this->dataConverter->convert($uniqueFieldData);
+                    if ($uniqueFieldData === $convertedValue) {
+                        // Skip for data rows that have been already converted
                         continue;
                     }
                     $bind = [$field => $convertedValue];
-                    $where = [$identifier . ' = ?' => (int) $row[$identifier]];
+                    $where = [$identifier . ' IN (?)' => $ids];
                     $connection->update($table, $bind, $where);
                 } catch (DataConversionException $e) {
                     throw new \Magento\Framework\DB\FieldDataConversionException(
@@ -93,7 +122,7 @@ public function convert(
                             $field,
                             $table,
                             $identifier,
-                            $row[$identifier],
+                            implode(', ', $ids),
                             get_class($this->dataConverter),
                             $e->getMessage()
                         )
@@ -102,4 +131,24 @@ public function convert(
             }
         }
     }
+
+    /**
+     * Get batch size from environment variable or default
+     *
+     * @return int
+     */
+    private function getBatchSize()
+    {
+        if (null !== $this->envBatchSize) {
+            $batchSize = (int) $this->envBatchSize;
+            if ($batchSize < self::MIN_BATCH_SIZE || $batchSize > self::MAX_BATCH_SIZE) {
+                throw new \InvalidArgumentException(
+                    'Invalid value for environment variable ' . self::BATCH_SIZE_VARIABLE_NAME . '. '
+                    . 'Should be integer and be > ' . self::MIN_BATCH_SIZE . ', < ' . self::MAX_BATCH_SIZE
+                );
+            }
+            return $batchSize;
+        }
+        return self::DEFAULT_BATCH_SIZE;
+    }
 }