-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Bulk inserts #2762
Bulk inserts #2762
Conversation
…t values excesses database limit
@deeky666 this patch introduces some fixups on your existing bulk-insert patch. Specifically, it will force transactions when an We should consider further scenarios too:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edefimov thank you for the patch. Please rebase it and make sure the most generic cases are tested in a functional way.
{ | ||
$valueSet = []; | ||
foreach ($values as $index => $value) { | ||
$this->parameters[] = $value; // todo: allow expressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the semantics of $values
from values to expressions would be a breaking change. It would make sense to implement it to avoid the breakage and maintain compatibility with QueryBuilder
.
$valueSet = array(); | ||
foreach ($this->columns as $index => $column) { | ||
$namedValue = isset($values[$column]) || array_key_exists($column, $values); | ||
$positionalValue = isset($values[$index]) || array_key_exists($index, $values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to allow mixing named and positional elements in the same call? Instead, we could see if $values
is a numeric or associative array and act based on that.
$this->table->getQuotedName($platform), | ||
$columnList, | ||
implode( | ||
'), (', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax is not supported by Oracle. The following workaround could be used instead:
INSERT INTO MY_TABLE
SELECT 1, 2 FROM DUAL
UNION ALL
SELECT 3, 4 FROM DUAL
/** | ||
* @param string $alias | ||
* @param array $registeredAliases | ||
* | ||
* @return \Doctrine\DBAL\Query\QueryException | ||
*/ | ||
static public function unknownAlias($alias, $registeredAliases) | ||
static public function unknownAlias(string $alias, array $registeredAliases): QueryException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change.
*/ | ||
public function getInsertMaxRows() | ||
{ | ||
return 10; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could PHPUnit mocking API be used instead? Otherwise, there's a tight coupling between a globally used mock and the tests using it.
|
||
$query->addValues(array('bar', 'baz', 'named' => 'bloo')); | ||
|
||
$this->assertSame("INSERT INTO foo VALUES (?, ?, ?)", $query->getSQL()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the "named"
column is explicitly specified in the call above, how does this guarantee that the third value will be associated with that column? Looks like a dangerous feature.
|
||
$query = new BulkInsertQuery($this->connection, 'foo'); | ||
|
||
for ($i = 0; $i <= $insertMaxRows; $i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this expected to work with the platforms which don't have the max rows limitation?
for ($i = 0; $i < $parameterCount; $i++) { | ||
$valueSet[] = "\\(\\?\\)"; | ||
} | ||
$sqlRegExp = "INSERT\\s+INTO\\s+foo\\s+\\(id\\)\s+VALUES\\s+" . implode("\\s*,\\s*", $valueSet); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This basically duplicates the implementation of building SQL and will have to be significantly reworked to implement compatibility with Oracle. It's enough to leave the functional part of the test. SQL is a platform-specific implementation detail.
|
||
$query = new BulkInsertQuery($this->connection, 'foo', ['id']); | ||
|
||
$numberOfRows = 4*$insertMaxRows + (int) ceil($insertMaxRows / 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this formula come from?
|
||
$numberOfRows = 4*$insertMaxRows + (int) ceil($insertMaxRows / 2); | ||
for ($i = 0; $i < $numberOfRows; $i++) { | ||
$query->addValues(array('id' => $i), array('id' => 'type' . $i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is a type equals to 'type' . $i
expected to work?
any news about this ? available in next major version(3) ? |
The change requests weren't covered yet, so there is no way to tell when this can be merged and introduced to a new version. |
Until this gets incorporated into the next major release, here's a temporary option: https://github.com/pharako/mysql-dbal |
i hope can use it. |
Closing as stale. |
This is an updated version of PR #682 solving issue #1392 refactored for php 7.1 and with small improvements like bulk insert execution even when the number of rows exceeds database limits.