The primary key columns don't have to be in the same order as the table columns #516

bartv2 · 2014-01-17T14:08:46Z

This happens when the columns are created in a different order then in the PK. This doesn't happen with Doctrine, as the order is taken from the PK.

…le columns

doctrinebot · 2014-01-17T14:08:51Z

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DBAL-793

We use Jira to track the state of pull requests and the versions they got
included in.

Ocramius · 2014-01-20T06:32:44Z

@bartv2 seems like Doctrine\Tests\DBAL\Functional\Schema\SqliteSchemaManagerTest::testListTableIndexes is failing on sqlite now.

Can you also add a test that ensures the order of the keys is kept?

…er as the table columns

bartv2 · 2014-01-26T14:26:55Z

Hmm, this change depends on sqlite version 3.7.16. See http://www.sqlite.org/changes.html#version_3_7_16
Enhance the PRAGMA table_info command so that the "pk" column is an increasing integer to show the order of columns in the primary key.
I'm not sure how to solve this now.

bartv2 · 2014-01-31T14:26:54Z

@beberlei @deeky666 I can add a version check to line 171, but is that acceptable?

Ocramius · 2014-01-31T18:32:11Z

@bartv2 if the check is platform-specific, then it probably needs a method in the platform /cc @deeky666

deeky666 · 2014-01-31T19:26:13Z

@bartv2 @Ocramius I would not add a version check in the schema manager, this seems an ugly hack. I think we had something like this in PostgreSQL and removed it again.
What I would try to do is just accept the fact that you cannot determine the order of PK columns before that version and rather use the order that is given instead.
So you would have to change the implementation not to set the ordinal PK column number as key in the index explicitely but rather just add columns to the index column stack and sort by key afterwards.

kimhemsoe · 2014-01-31T19:27:35Z

Agree dekyy

On 31/01/2014, at 20.26, "Steve Müller" notifications@github.com wrote:

@bartv2 https://github.com/bartv2 @Ocramius
https://github.com/OcramiusI would not add a version check in the
schema manager, this seems an ugly
hack. I think we had something like this in PostgreSQL and removed it again.
What I would try to do is just accept the fact that you cannot determine
the order of PK columns before that version and rather use the order that
is given instead.
So you would have to change the implementation not to set the ordinal PK
column number as key in the index explicitely but rather just add columns
to the index column stack and sort by key afterwards.

Reply to this email directly or view it on
GitHubhttps://github.com//pull/516#issuecomment-33833855
.

deeky666 · 2014-01-31T19:27:56Z

Sort by SQLite's PK index number of course, not array key.

…3.7.16

Take the column order into account and sort non PK columns last

bartv2 · 2014-02-07T10:36:22Z

How is this, all the tests pass

Ocramius · 2014-02-07T10:45:03Z

lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php

+            if ($a['pk'] == $b['pk']) {
+                return $a['cid'] - $b['cid'];
+            }
+            if ($a['pk'] == "0" && $b['pk'] != "0") {


Are this conditional and the one at line 176 required?

You're right they are not, it 0 columns location doesn't matter

beberlei · 2014-02-08T09:09:09Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

+    {
+        $version = \SQLite3::version();
+        if(version_compare($version['versionString'], '3.7.16', '<')) {
+            $this->markTestSkipped('This version of sqlite doesn\'t return the order of the Primary Key.');


What does that mean? We get a notice in the SqliteSchemaManager because some keys (pk or cid) do not exist? Or does the usort work as well.

@beberlei The test only works on SQLite versions > 3.7.16 because PK columns order information was added for the PRAGMA TABLE_INFO() statement there. Before that it is not possible to tell in which order the columns in a PK were defined when reverse engineering with PRAGMA TABLE_INFO().
Therefore this test will fail for earlier versions as it retrieves array('id', 'other_id') instead of array('other_id', 'id').

I'm not quite happy with this version constraint. I see the limitation for earlier SQLite versions but IMO we should at least document this limitation in the known vendors issues. I don't want to see any complaints about this issue from users using a SQLite version before this fix.

@deeky666 I am not sure @beberlei's comment has been addressed. He is referring to the changes in lib/Doctrine/DBAL/Schema/SqliteSchemaManager.php. If the pk column is not there on older Sqlite versions, the usort($indexArray, function($a, $b) { call will probably throw notices for undefined array index.

@bantu The pk column is there, it's just that it is always 1 on older versions of SQLite and there you cannot determine the order of columns in a composite PK.

Okay, thanks. So the column is there but contains incorrect values in some cases. Maybe this patch can be accepted now.

bartv2 · 2014-02-19T18:56:50Z

Anything stopping this from merging?

DeepDiver1975 · 2014-03-06T22:15:02Z

@beberlei @deeky666 @Ocramius Can this be merged?
In addition: how do your backporting strategies look like? Any chance to get this into 2.4.x?

Thanks a lot!

deeky666 · 2014-03-06T22:25:08Z

tests/Doctrine/Tests/DBAL/Functional/Schema/SqliteSchemaManagerTest.php

+    other_id INTEGER,
+    PRIMARY KEY(other_id, id)
+)
+EOS


@bartv2 Is this explicit raw SQL necessary? Can't we just $this->_sm->createTable($table) here?

No that function will call Schema\Table->getColumns() which returns a sorted list of columns and that is not what we want here.

deeky666 · 2014-03-06T22:27:06Z

Besides some CS issues and maybe mentioning this particular limitation in the known vendor issues, this PR is okay from my point of view.

bantu · 2014-03-10T15:34:03Z

On older SQLite versions the schema could be dumped which should also contain the correct order of primary key columns.

…qlite

bantu · 2014-03-24T18:36:21Z

Travis CI error seems to be unrelated. psql: could not connect to server: No such file or directory

@deeky666 @beberlei What needs to be done in order to get this merged?

deeky666 · 2014-03-25T07:10:27Z

Yeah travis seems to have problems sometimes bringing up the PostgreSQL service. Restarted the failing build and it should be okay now.
@beberlei This looks good to merge.

The primary key columns don't have to be in the same order as the table columns

bantu · 2014-03-26T11:09:26Z

@beberlei Can we get this backported to 2.3 please?

The primary key columns don't have to be in the same order as the tab…

b4dc7e0

…le columns

bartv2 mentioned this pull request Jan 17, 2014

Fix Sqlite PK detection owncloud-archive/3rdparty#68

Closed

Add unittest for primary key columns don't have to be in the same ord…

30b286c

…er as the table columns

bartv2 mentioned this pull request Jan 31, 2014

Command line tool to convert current database to others, except sqlite owncloud/core#6457

Merged

bartv2 added 3 commits February 4, 2014 21:06

Code the primary key ordering so it works with sqlite before version …

e1c36ac

…3.7.16

Skip test on sqlite versions where this can't work

cdd2d49

Make the column more predictable

18cbfdc

Take the column order into account and sort non PK columns last

Ocramius reviewed Feb 7, 2014
View reviewed changes

"0" columns location doesn't matter

4dd9392

beberlei reviewed Feb 8, 2014
View reviewed changes

bartv2 added a commit to owncloud-archive/3rdparty that referenced this pull request Feb 18, 2014

doctrine/dbal#516 PR

6240224

deeky666 reviewed Mar 6, 2014
View reviewed changes

Document known issue about reverse engineering primary key order in s…

454b8f4

…qlite

beberlei added a commit that referenced this pull request Mar 25, 2014

Merge pull request #516 from bartv2/sort-sqlite-pk-columns

7c68e80

The primary key columns don't have to be in the same order as the table columns

beberlei merged commit 7c68e80 into doctrine:master Mar 25, 2014

bartv2 deleted the sort-sqlite-pk-columns branch March 25, 2014 16:49

doctrinebot mentioned this pull request Dec 7, 2015

DBAL-793: [GH-516] The primary key columns don't have to be in the same order as the table columns #2024

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The primary key columns don't have to be in the same order as the table columns #516

The primary key columns don't have to be in the same order as the table columns #516

bartv2 commented Jan 17, 2014

doctrinebot commented Jan 17, 2014

Ocramius commented Jan 20, 2014

bartv2 commented Jan 26, 2014

bartv2 commented Jan 31, 2014

Ocramius commented Jan 31, 2014

deeky666 commented Jan 31, 2014

kimhemsoe commented Jan 31, 2014

deeky666 commented Jan 31, 2014

bartv2 commented Feb 7, 2014

Ocramius Feb 7, 2014

bartv2 Feb 7, 2014

beberlei Feb 8, 2014

deeky666 Feb 8, 2014

deeky666 Mar 6, 2014

bantu Mar 10, 2014

deeky666 Mar 10, 2014

bantu Mar 10, 2014

bartv2 commented Feb 19, 2014

DeepDiver1975 commented Mar 6, 2014

deeky666 Mar 6, 2014

bartv2 Mar 7, 2014

deeky666 commented Mar 6, 2014

bantu commented Mar 10, 2014

bantu commented Mar 24, 2014

deeky666 commented Mar 25, 2014

bantu commented Mar 26, 2014

The primary key columns don't have to be in the same order as the table columns #516

The primary key columns don't have to be in the same order as the table columns #516

Conversation

bartv2 commented Jan 17, 2014

doctrinebot commented Jan 17, 2014

Ocramius commented Jan 20, 2014

bartv2 commented Jan 26, 2014

bartv2 commented Jan 31, 2014

Ocramius commented Jan 31, 2014

deeky666 commented Jan 31, 2014

kimhemsoe commented Jan 31, 2014

deeky666 commented Jan 31, 2014

bartv2 commented Feb 7, 2014

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

bartv2 commented Feb 19, 2014

DeepDiver1975 commented Mar 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeky666 commented Mar 6, 2014

bantu commented Mar 10, 2014

bantu commented Mar 24, 2014

deeky666 commented Mar 25, 2014

bantu commented Mar 26, 2014