Support for Partial Indexes for PostgreSql and Sqlite

[PowerKiKi]

Support for Partial Indexes was available in Doctrine 1 following
http://www.doctrine-project.org/jira/browse/DC-82. This commit
reintroduce support for Doctrine 2. We use the same syntax with an
optionnal "where" attribute for Index and UniqueConstraint.

It is unit-tests covered and documented in manual.

[doctrinebot]

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-900

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

[guilhermeblanco]

Hi, I know that Oracle also supports partial indexes too.
Could you please evaluate each driver we support and update the implementation to match them?

[PowerKiKi]

Hey,

I already had a look at all platforms supporting this feature and only
found out about PostgreSQL and Sqlite, which confirms what is found on
http://en.wikipedia.org/wiki/Partial_index#Support.

Maybe you could point me to Oracle documentation mentioning those ? Also
since I don't have access to any Oracle installation I am not exactly sure
how to go about testing. But I'll gladly make the effort if you help me
with that.

On a side-note, SQL Server do support partial indexes, but I did not
implement them. This is because there already is what seems to be a
workaround that actual use partial indexes implicitly. It was introduced in
commit 6197492. And from what I understand
it implicitly force all indexes to be ignored if all columns are NULL (see
_appendUniqueConstraintDefinition()). At first glance, I would say it
would be better to leave that up to the end-user, but I may be mistaken.
Most importantly modifying this would probably be a huge compatibility
break.

To sum up, since I was not sure if this feature would be merged, I didn't
put much effort in other platforms, but if you can confirm that it is of
interest, we can complete it.

PS: I forced push a few modification in the last few days. What is the
preferred way of working, pushing new commits and squashing when everything
is ready or force pushing new version of the PR ?

[deeky666]

Sorry pushed the wrong button ^^

[PowerKiKi]

@PowerKiKi the commit you are referncing refers to a legacy class which does not exist anymore

The file does not exists anymore because it was renamed from MsSqlPlatform.php to SQLServerPlatform.php. However the method mentioned, _appendUniqueConstraintDefinition(), still exists in master today.

[Ocramius]

Needs a rebase...

[PowerKiKi]

Done ! Nothing major was changed. Thanks for your interest.

To come back on other platforms support, I think we should first focus on this PR and add other platforms later on if there is any interest. Not try to do everything at once.

[Ocramius]

Looks good to me, @deeky666?

[deeky666]

Please use {@inheritdoc} for this DocBlock here

[bakura10]

Any news regarding this feature? Would be nice to have :)

[PowerKiKi]

So I fixed the few things mentioned and rebased my work on master. I believe it is ready to be merged.

[PowerKiKi]

@Ocramius, @deeky666 could you have a look again before it needs to be rebased again ? I believe I fixed everything you asked for and it should be mergeable now.

[Ocramius]

I think this is good to go, but I'll have to defer pressing to @deeky666:

[deeky666]

No need for else here.

if ($this->supportsPartialIndexes() && $index->hasOption('where')) {
    return ' WHERE ' . $index->getOption('where');
}

return '';

[PowerKiKi]

Sure, I should have noticed those... fixed !

[PowerKiKi]

And don't forget to merge doctrine/orm#1027 as well, since it would not be very useful otherwise

[deeky666]

@PowerKiKi sure we have it in our minds ;)

[deeky666]

He have another BC break here. @PowerKiKi can you please also mention this change in the UPGRADE.md?

[stof]

Please add an array typehint on the argument here

[PowerKiKi]

Both remarks were fixed.

[deeky666]

@PowerKiKi I am missing the schema introspection part for partial indexes in SQLite. As far as I know you can get the information from the system table sqlite_master . Can you please evaluate? I think you can adopt the logic from SqliteSchemaManager::listTableForeignKeys(). Also a simple functional test might be useful to verify the schema introspection.

[deeky666]

Hmmm you are also skipping SQL generation tests for getUniqueConstraintDeclarationSQL() and getCreateIndexSQL() which actually is supported...

[PowerKiKi]

I actually get the following errors:

• getIndexDeclarationSQL()
  • Doctrine\DBAL\DBALException: Operation 'Doctrine\DBAL\Platforms\SQLAnywherePlatform::getIndexDeclarationSQL' is not supported by platform.``
• getIndexDeclarationSQL()
  • InvalidArgumentException: Can only create unique constraint declarations, no common index declarations with getUniqueConstraintDeclarationSQL().
• getCreateIndexSQL()
  • passed !

So it seems, there is only getCreateIndexSQL() which could be tested. And since this test was entirely written by me and getCreateIndexSQL() is already tested elsewhere, I don't think we "lose" coverage, nor that the effort would be worth it. What do you think ?

[PowerKiKi]

Index options are now covered by unit tests.

Regarding Sqlite, I already had look at it, and re-confirmed it. Unfortunately the state of Sqlite platform seems to be less well maintained. Not even speaking of partial indexes, but plain vanilla indexes already fail miserably the schema introspection. But it does not appear anywhere in unit tests AFAIK. When switching my current project to Sqlite all indexes are always deleted and re-created from scratch when doing things like orm:schema-tool:update --force.

I would actually consider dropping Sqlite support from this PR, and in another PR fix vanilla indexes first, and then re-consider the partial indexes. What do you think ? Also I am not sure how to unit tests against "real" database... what the best practice about it ?

[deeky666]

@PowerKiKi thanks for applying the changes and investigating concerning SQLite. I was not aware that SQLite fails the schema introspection on indexes so you might be right and we should defer that issue on another PR to move forward here. We need to reinvestigate support for other platforms anyway so yeah I'm fine with your suggestion to drop partial index support on SQLite for now.
Concerning SQL Anywhere, I can live with that too. So if you could remove SQLite support and maybe squash commits, I will merge if @Ocramius does not have any remarks left :)
Thanks alot!

[deeky666]

Btw concerning functional tests, have a look at schema manager functional tests. They cover real schema introspection test cases.

[Ocramius]

I don't feel like squashing commits is required here: we'd lose valuable history IMO

[PowerKiKi]

So I just pushed the de-activation of Sqlite.

As for squashing I don't have any preferences. What should we do ?

[deeky666]

@PowerKiKi no need to squash then. I'll merge as soon as Travis goes green :) Thank you.

[PowerKiKi]

You're very welcome. You were very thorough while reviewing this PR. It's nice to see that code quality is an important factor for Doctrine. And I am glad we were able to fix everything that needed fixing.

[Ocramius]

\o/

[deeky666]

@PowerKiKi we got a serious problem here in PostgreSQL >= 9.5. See #2565
The problem is revealed here. Seems they have changed the decompilation algorithm for partial index expressions.

PostgreSQL < 9.5

(((id IS NOT NULL) AND (name IS NULL)) AND (email IS NULL))

PostgreSQL >= 9.5

((id IS NOT NULL) AND (name IS NULL) AND (email IS NULL))

This makes the comparator freak out -.-
Question is what to do. pg_get_expr() takes a third argument pretty_print which when set results in:

id IS NOT NULL AND name IS NULL AND email IS NULL
id IS NOT NULL AND name IS NULL AND email IS NULL

But that option will convert the original expression, too. I do not know how to retrieve the original definition. @Ocramius should we simply adjust our test suite to return the same result on all platform versions and leave the expression as "fragile"?

[Ocramius]

This is DDL only, and for a quite complicated platform-specific version. Fragile can be acceptable given that the feature is not mainstream and generally user-agent-facing. On 18 Jan 2017 1:15 a.m., "Steve Müller" <notifications@github.com> wrote: @PowerKiKi <https://github.com/PowerKiKi> we got a serious problem here in PostgreSQL >= 9.5. See #2565 <#2565> The problem is revealed here <https://github.com/doctrine/dbal/blob/master/tests/Doctrine/Tests/DBAL/Functional/Schema/PostgreSqlSchemaManagerTest.php#L356>. Seems they have changed the decompilation algorithm for partial index expressions. *PostgreSQL < 9.5* (((id IS NOT NULL) AND (name IS NULL)) AND (email IS NULL)) *PostgreSQL >= 9.5* ((id IS NOT NULL) AND (name IS NULL) AND (email IS NULL)) This makes the comparator freak out -.- Question is what to do. pg_get_expr() takes a third argument pretty_print which when set results in: id IS NOT NULL AND name IS NULL AND email IS NULL id IS NOT NULL AND name IS NULL AND email IS NULL But that option will convert the original expression, too. I do not know how to retrieve the original definition. @Ocramius <https://github.com/Ocramius> should we simply adjust our test suite to return the same result on all platform versions and leave the expression as "fragile"? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#600 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakCMtPR1-ky2E0mxlD_KRnM58CNB_ks5rTVmpgaJpZM4B4uZe> .