Improve `Table#getColumns()` performance by reducing its runtime complexity #2724

evgpisarchik · 2017-05-14T03:07:41Z

Resolves #2722
Seems resonable proposal by @verheyenkoen
Could you review these changes please?

Ocramius · 2017-05-16T10:58:49Z

lib/Doctrine/DBAL/Schema/Table.php

-        });
-
-        return $columns;
+        return array_replace(array_flip($colNames), $columns);


I must say that I neither understand this nor the original code. What is the intent: sorting columns? Is there a corresponding test somewhere?

For some reasons PK columns are put first, then FK columns, then the other ones. No idea why that was implemented like this. No idea if anyone relies on that...

That is indeed the "standard approach" when writing DDL, as it otherwise is really hard to figure out what is going on. Still, is there no O(n * log(n)) or similar solution that doesn't make the code unreadable? Original code was heavy due to the repeated array_search() calls, which make this O(n * log(n) * n), so your approach is good, just not understandable at all.

You mean this may be better if the array_replace line is refactored in a separate private method with a meaningful name?

Is it more comprehensible?

/** * Returns ordered list of columns (primary keys are first, then foreign keys, then the rest) * @return Column[] */ public function getColumns() { return array_merge($this->getPrimaryKeyColumns(), $this->getForeignKeyColumns(), $this->_columns); } /** * Returns primary key columns * @return array */ public function getPrimaryKeyColumns() { $primaryKeyColumnNames = $this->getPrimaryKeyColumnNames(); return array_filter($this->_columns, function ($key) { return in_array($key, $primaryKeyColumnNames); }, ARRAY_FILTER_USE_KEY); } /** * Returns foreign key columns * @return array */ public function getForeignKeyColumns() { $foreignKeyColumnNames = $this->getForeignKeyColumnNames(); return array_filter($this->_columns, function ($key) { return in_array($key, $foreignKeyColumnNames); }, ARRAY_FILTER_USE_KEY); } /** * Returns primary key column names * @return array */ public function getPrimaryKeyColumnNames() { $primaryKeyColumnNames = []; if ($this->hasPrimaryKey()) { $primaryKeyColumnNames = $this->getPrimaryKey()->getColumns(); } return $primaryKeyColumnNames; } /** * Returns foreign key column names * @return array */ public function getForeignKeyColumnNames() { $foreignKeyColumnNames = []; foreach ($this->getForeignKeys() as $foreignKey) { /* @var $foreignKey ForeignKeyConstraint */ $foreignKeyColumnNames = array_merge($foreignKeyColumnNames, $foreignKey->getColumns()); } return $foreignKeyColumnNames; }

@evgpisarchik That seems not too efficient as calls to getPrimaryKeyColumnNames() and getForeignKeyColumnNames() are called multiple times. Am I right?

I'm using php 5.6 syntax since composer.json has

"require": { "php": "^7.0",

You can use PHP 7 syntax, as master requires PHP 7.x

@verheyenkoen you're right, I've updated the code

evgpisarchik · 2017-05-20T10:20:59Z

Updated previously pushed code, squashed both commits

Ocramius

squashed both commits

Urgh, now I can't compare it with the previous solution. Please don't squash commits next time :S

As far as I can see, this patch added Table#getForeignKeyColumns(), Table#getPrimaryKeyColumnNames(), Table#getForeignKeyColumnNames(), which are all now to be tested to prevent BC regressions.

I appreciate your effort, I really do, but the newly exposed public API is a problem.

Ocramius · 2017-05-20T11:08:03Z

lib/Doctrine/DBAL/Platforms/MySqlPlatform.php

@@ -742,7 +742,7 @@ private function getPreAlterTableAlterIndexForeignKeySQL(TableDiff $diff)
        foreach ($diff->changedIndexes as $changedIndex) {
            // Changed primary key
            if ($changedIndex->isPrimary() && $diff->fromTable instanceof Table) {
-                foreach ($diff->fromTable->getPrimaryKeyColumns() as $columnName) {
+                foreach ($diff->fromTable->getPrimaryKeyColumnNames() as $columnName) {


Why is this not simply $diff->fromTable->getPrimaryKeyColumns() and then on line 746 $columnName->getName() ?

Ocramius · 2017-05-20T11:08:19Z

lib/Doctrine/DBAL/Schema/Table.php

     * @return Column[]
     */
    public function getColumns()
    {
-        $columns = $this->_columns;
+        return array_merge($this->getPrimaryKeyColumns(), $this->getForeignKeyColumns(), $this->_columns);


This looks much cleaner 👍

Using $this->getPrimaryKeyColumns() in this method will fail if there are no primary keys, and the BC break in getPrimaryKeyColumns is reverted (exception throwing on empty PK)

Ocramius · 2017-05-20T11:08:40Z

lib/Doctrine/DBAL/Schema/Table.php

+    {
+        $primaryKeyColumnNames = $this->getPrimaryKeyColumnNames();
+        return array_filter($this->_columns, function ($key) use ($primaryKeyColumnNames) {
+            return in_array($key, $primaryKeyColumnNames);


Use strict comparisons at all times

Ocramius · 2017-05-20T11:10:54Z

lib/Doctrine/DBAL/Schema/Table.php

-    public function getPrimaryKeyColumns()
-    {
-        if ( ! $this->hasPrimaryKey()) {
-            throw new DBALException("Table " . $this->getName() . " has no primary key.");


This exception is gone - that is a BC break, as now an empty list of primary key columns is returned

Ocramius · 2017-05-20T11:11:17Z

lib/Doctrine/DBAL/Schema/Table.php

+     * Returns foreign key column names
+     * @return array
+     */
+    public function getForeignKeyColumnNames()


Doesn't need to be public

Ocramius · 2017-05-20T11:11:28Z

lib/Doctrine/DBAL/Schema/Table.php

+     * Returns foreign key columns
+     * @return Column[]
+     */
+    public function getForeignKeyColumns()


Doesn't need to be public

Ocramius · 2017-05-20T11:12:32Z

lib/Doctrine/DBAL/Schema/Table.php

+     * Returns primary key column names
+     * @return array
+     */
+    public function getPrimaryKeyColumnNames()


Doesn't need to be public

evgpisarchik · 2017-05-21T07:50:47Z

Reverted changes to public Api that were made before (now getPrimaryKeyColumns returns array of strings (not Column[]).

getColumns method is cleaner that it was before and has the same behavior as the original code (it doesn't throw exception if table doesn't have primary keys )

Ocramius · 2017-06-01T13:44:12Z

@evgpisarchik perfect, thanks for all the effort! \o/

🚢

evgpisarchik · 2017-06-01T18:17:03Z

my pleasure!

Ocramius suggested changes May 16, 2017

View reviewed changes

refactored getColumns method

e5c2695

evgpisarchik force-pushed the issue_2722 branch from 97f0db3 to e5c2695 Compare May 20, 2017 10:06

Ocramius suggested changes May 20, 2017

View reviewed changes

evgpisarchik force-pushed the issue_2722 branch from cddde1e to 009acec Compare May 21, 2017 07:29

reverted public api changes and removed not necessary public methods

7b53b5e

evgpisarchik force-pushed the issue_2722 branch from 009acec to 7b53b5e Compare May 21, 2017 07:50

evgpisarchik changed the title ~~uksort replaced by array_replace to improve performance~~ Table->getColumns() performance May 21, 2017

Ocramius added this to the 2.6 milestone Jun 1, 2017

Ocramius added Columns Improvement Schema Comparison Schema Definition labels Jun 1, 2017

Ocramius self-assigned this Jun 1, 2017

Ocramius merged commit 476c2f9 into doctrine:master Jun 1, 2017

Ocramius changed the title ~~Table->getColumns() performance~~ Improve Table#getColumns() performance Jul 22, 2017

Ocramius changed the title ~~Improve Table#getColumns() performance~~ Improve Table#getColumns() performance by reducing its runtime complexity Jul 22, 2017

github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve `Table#getColumns()` performance by reducing its runtime complexity #2724

Improve `Table#getColumns()` performance by reducing its runtime complexity #2724

evgpisarchik commented May 14, 2017 •

edited

Loading

Ocramius May 16, 2017

verheyenkoen May 16, 2017 •

edited

Loading

Ocramius May 16, 2017 •

edited

Loading

verheyenkoen May 16, 2017

evgpisarchik May 18, 2017 •

edited

Loading

verheyenkoen May 18, 2017

evgpisarchik May 18, 2017 •

edited

Loading

Ocramius May 18, 2017

evgpisarchik May 18, 2017

evgpisarchik commented May 20, 2017

Ocramius left a comment

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

Ocramius May 20, 2017

evgpisarchik commented May 21, 2017 •

edited

Loading

Ocramius commented Jun 1, 2017

evgpisarchik commented Jun 1, 2017

Improve Table#getColumns() performance by reducing its runtime complexity #2724

Improve Table#getColumns() performance by reducing its runtime complexity #2724

Conversation

evgpisarchik commented May 14, 2017 • edited Loading

Choose a reason for hiding this comment

verheyenkoen May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Ocramius May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgpisarchik May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgpisarchik May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgpisarchik commented May 20, 2017

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgpisarchik commented May 21, 2017 • edited Loading

Ocramius commented Jun 1, 2017

evgpisarchik commented Jun 1, 2017

Improve `Table#getColumns()` performance by reducing its runtime complexity #2724

Improve `Table#getColumns()` performance by reducing its runtime complexity #2724

evgpisarchik commented May 14, 2017 •

edited

Loading

verheyenkoen May 16, 2017 •

edited

Loading

Ocramius May 16, 2017 •

edited

Loading

evgpisarchik May 18, 2017 •

edited

Loading

evgpisarchik May 18, 2017 •

edited

Loading

evgpisarchik commented May 21, 2017 •

edited

Loading