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:
|
| { | ||
| $valueSet = []; | ||
| foreach ($values as $index => $value) { | ||
| $this->parameters[] = $value; // todo: allow expressions. |
There was a problem hiding this comment.
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.
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.
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| * @return \Doctrine\DBAL\Query\QueryException | ||
| */ | ||
| static public function unknownAlias($alias, $registeredAliases) | ||
| static public function unknownAlias(string $alias, array $registeredAliases): QueryException |
| */ | ||
| public function getInsertMaxRows() | ||
| { | ||
| return 10; |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.