Skip to content
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

Support for Partial Indexes for PostgreSql and Sqlite #1027

Merged
merged 4 commits into from
Nov 5, 2014

Conversation

PowerKiKi
Copy link
Contributor

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. This Pull Request depends on doctrine/dbal#600. So they should both be merged or rejected together.

Thanks for your time !

@doctrinebot
Copy link

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/DDC-3117

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

@Ocramius
Copy link
Member

@PowerKiKi since this seems to be a platform-specific feature, what about introducing it in an "options" key of the index mappings (like tables do)?

@PowerKiKi
Copy link
Contributor Author

While this pull-request is clearly about PostgreSql, partial indexes are also possible on Sqlite and Mssql. Does that still falls in "platform-specific" definition ?

But most importantly my choice was to be consistent with what existed in Doctrine 1.

However, since I don't have a good overview of the Doctrine way of doing things, I am open to discussion... Wouldn't introducing an array of options for only a single possible option be slightly cumbersome to use ?

@Ocramius
Copy link
Member

@PowerKiKi yes, but it would still be an indicator of something "custom" going on.

We can explicitly support only what is being supported by all (supported) SQL RDBMSs out there, that's why I suggested making it a stashed-away "option"

@PowerKiKi
Copy link
Contributor Author

Fair enough. Since it may impact all platform-specific things for indexes
in the future, maybe it's an "important" decision. Would there be someone
else who should give his opinion on that matter ? Or is the two of us
enough ?

@Ocramius
Copy link
Member

@deeky666 maybe (if he has time)

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

On 14 July 2014 14:58, PowerKiKi [email protected] wrote:

Fair enough. Since it may impact all platform-specific things for indexes
in the future, maybe it's an "important" decision. Would there be someone
else who should give his opinion on that matter ? Or is the two of us
enough ?
On Jul 14, 2014 7:08 PM, "Marco Pivetta" [email protected]
wrote:

@PowerKiKi https://github.com/PowerKiKi yes, but it would still be an
indicator of something "custom" going on.

We can explicitly support only what is being supported by all
(supported)
SQL RDBMSs out there, that's why I suggested making it a stashed-away
"option"


Reply to this email directly or view it on GitHub
#1027 (comment).


Reply to this email directly or view it on GitHub
#1027 (comment).

@deeky666
Copy link
Member

@Ocramius @PowerKiKi sorry for the delay on this. I am tempting to agree with @Ocramius because of the given reasons. Also introducing more and more parameters for each platform specific feature is really bloating the API. I would also stick to using an options array here as being done for other specific features, too. The only "problem" I see here is that there already is a flags array for indexes which might be a bit confusing if we have both flags and options for indexes. But it might still be better than introducing a where parameter here. Don't know...

@PowerKiKi
Copy link
Contributor Author

Could we have where inside flags ? and later on, when we can break
compatibility rename flags into options ?

Btw, I don't see any mention of flags in the manual. Shouldn't they be
mentioned somewhere ?

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.
This coherent with what is done for Table. All platform specific things
are grouped into an options array. Eventually flags should be migrated
into options as well.
@PowerKiKi
Copy link
Contributor Author

@deeky666, @Ocramius, I moved where into a new options array, as you both suggested to be more coherent with what is done for Tables.

One future thing to do would be to move flags into options (or even merge them if that makes sense). But that's definitely out of this PR scope and clearly a BC.

Anyway I believe this PR is ready to be merged (as well as its dependency: doctrine/dbal#600)

@PowerKiKi
Copy link
Contributor Author

@Ocramius, @deeky666, a while back we merged doctrine/dbal#600, but this PR here seems to have been forgotten and without it the first PR is useless.

Would you mind having a look at it one more time ?

PS: on a related note, I also opened a new PR, doctrine/dbal#716, to be able to use partial indexes with "complex" conditions

@Ocramius Ocramius self-assigned this Nov 5, 2014
@Ocramius
Copy link
Member

Ocramius commented Nov 5, 2014

@PowerKiKi thanks for poking again: merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants