Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When execute command seed:run, insert all data to a table in a bulk. #1148

Merged
merged 7 commits into from Aug 31, 2017

Conversation

kenzo0107
Copy link
Contributor

When execute command seed:run, insert all data to a table in a bulk.
It take much time to insert data to a table one by one until now.

An example is shown below:

$table = $this->table('users');
$table->bulk()
      ->insert($data)
      ->save();

It became more than triple as fast by Bulk Insert when inserting 1,000 records.
It is very effective when many data samples are required.

However, it is assumed that the keys of data to be set are unified.
The method bulk() are not available in the following example:

$data = array(
    array(
        'column1' => 'value1',
        'column2' => 1,
    ),
    array(
        'column1' => 'value2',
        'column3' => 'foo',
    ),
    array(
        'column1' => 'value3',
        'column2' => 3,
        'column3' => 'foo',
    )
);

$table = $this->table('users');
$table->bulk()
  ->insert($data)
  ->save();

The function of dynamically checking the key of the set data is not added,
because it will cause performance degradation.

It take much time to insert data to a table one by one until now.

An example is shown below:

```
$table = $this->table('users');
$table->bulk()
      ->insert($data)
      ->save();
```

It became more than triple as fast by Bulk Insert when inserting 1,000 records.
It is very effective when many data samples are required.
Copy link
Member

@lorenzo lorenzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way you can implement this without adding the bulk flag? I'm not sure it makes for a good API

@@ -75,6 +75,11 @@ class Table
protected $data = array();

/**
* @var boolean
*/
protected $bulk = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this flag is for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is prepared to judge whether to use the bulk insert function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not always use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
There is a way I can implement this without adding the bulk flag.
Thx.

$keys = array_keys($current);
$sql .= "(". implode(', ', array_map(array($this, 'quoteColumnName'), $keys)) . ") VALUES";

$objTmp = (object) array('aFlat' => array());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aFlat

Typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, typo !
fix it !

$sql .= "(". implode(', ', array_map(array($this, 'quoteColumnName'), $keys)) . ") VALUES";

$objTmp = (object) array('aFlat' => array());
array_walk_recursive($rows, create_function('&$v, $k, &$t', '$t->flat[] = $v;'), $objTmp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use a Closure? Phinx seems to require PHP >= 5.4. And if you use a closure, $objTmp is unnecessary, as it can inherit variables form the parent scope by reference. And $rows would be a two-dimensional array. You need not handle it recursively. If I were you, I would use foreach in this case:

$vals = [];
foreach ($rows as $row) {
   foreach ($row as $v) {
       $vals[] = $v;
   }
}

Also, I think you should make sure that all rows have the same keys. I think it doesn't make a big impact on performance, but dose prevent users' mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your review !

I tried to simply coded in one line.
But there was not enough consideration to Phinx.

Fix it !

Thx.

fallaten values in data simply.

And typo, sorry.
set prefix adapter to a table when bulk insert.
By checking the keys in data to be inserted, and when all the keys are matched, execute bulk insert.
By checking the keys in data to be inserted, and when all the keys are matched, execute bulk insert.
@kenzo0107
Copy link
Contributor Author

kenzo0107 commented Aug 17, 2017

@lorenzo

I could implement Bulk Insert without adding the bulk flag 🤓
Please review.

@lorenzo
Copy link
Member

lorenzo commented Aug 21, 2017

I think the only thing this is missing is tests showing that the method actually works

@kenzo0107
Copy link
Contributor Author

kenzo0107 commented Aug 21, 2017

@lorenzo

Thank you for review !

I added to test the new method with the following method.

  • tests/Phinx/Db/Adapter/TablePrefixAdapterTest::testInsertData
  • tests/Phinx/Db/TableTest::testInsertSaveData
  • tests/Phinx/Migration/AbstractMigrationTest::testInsert

Is it necessary elsewhere ?

@lorenzo
Copy link
Member

lorenzo commented Aug 30, 2017

@kenzo0107 I didn't see any tests where the method was actually used. Only mocked tests were used, so the actual implementation is not tested

add test of method bulkinsert to each adaptor.
@kenzo0107
Copy link
Contributor Author

@lorenzo Thank you for your review. I added some tests where the method is actually used.
Please review again.

@lorenzo
Copy link
Member

lorenzo commented Aug 31, 2017

Thanks @kenzo0107 !

@lorenzo
Copy link
Member

lorenzo commented Aug 31, 2017

@kenzo0107 Would you mind sending another pull request with a short example for the docs?

@lorenzo lorenzo merged commit 17ec982 into cakephp:master Aug 31, 2017
@kenzo0107
Copy link
Contributor Author

@lorenzo Thank you 😄 I will send another pull request soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants