Implement partial index support

[brandonramirez]

Partial/filtered indexes for Postgres, SQLite and Microsoft SQL Server.

Some of the SQL Server integration tests fail, even without this change. They appear to be existing issues.

I attempted to implement this feature for Oracle too, but getting connectivity to Oracle from my development environment is a nightmare. Even in the CI environment, Oracle client installation fails intermittently, so I decided to keep my hands out of Oracle.

MySQL and MariaDB do not support this feature, so I did not attempt to implement it there.

There are still outstanding test failures (I think all due to the above issues), so the Travis CI builds keep failing. I have verified that this feature does not introduce any new test failures.

Fixes #657

[elhigu]

I'll fix those oracle tests when I have some time. They just must be changed so that only node 4,6 and 8 are tested with oracle. Thanks for the PR though! I'll check this PR when I have some time in my hands.

[elhigu]

I got CI fixed up. Could you please rebase this?

[elhigu]

Code + tests looks good. I have nothing to complain :) Only thing missing is link to the documentation PR.

[brandonramirez]

Sorry, been on other things. Working on that now.

[brandonramirez]

I connected an upstream remote to my fork and merged changes from the upstream master into the feature branch. Now the CI tests pass. I think your fix was successful.

[brandonramirez]

Documentation PR: knex/documentation#82

[elhigu]

Btw. I just noticed that this feature seems to require passing builder instance to tell how partial index should be created. does the alternative (builder) => builder.whereRaw() syntax work too?

[brandonramirez]

I’m not sure what you mean exactly. It supports knex.raw if that is what you mean.

There are test cases which uses knex.raw to attach a raw SQL predicate to an index.

[elhigu]

I mean that usually knex APIs allow passing builder in following formats:

// query builder
table.index(['foo', 'bar'], 'baz', knex.whereNotNull('email'));

// builder callback arrow function style
table.index(['foo', 'bar'], 'baz', builder => builder.whereNotNull('email'));

// builder callback normal function + this style
table.index(['foo', 'bar'], 'baz', function () { this.whereNotNull('email') });

// raw query builder
table.index(['foo', 'bar'], 'baz', knex.raw('where ?? IS NOT NULL', ['email']));

[brandonramirez]

Ok I' not familiar with those approaches. I'll test them out.

[brandonramirez]

I understand what you're asking now. knex.whereIn, whereExists, etc. use this builder callback pattern. I'm tracing back to see how it works. The magic appears to be in the "compileCallback" method. I may need to refactor the approach to follow this standard. I'll keep working at it in my spare time.

Do you consider this a blocker for the PR? (I totally understand if you do - I am new to knex so am not a good judge of where things are).

I just had an addition to my family yesterday so I'm not sure how much time I will have to work on this. Just looking to gauge how important it is to enhance this now versus later.

[elhigu]

Congratulations! I think it is a blocker if the usual syntaxes are not supported, otherwise we start getting bug reports and confused people when they try to use the feature and those other parameters are not working. There was a time when I was accepting half-complete features, but I stopped it since those features were never completed afterwards (like missing implementations for other dialects etc.).

[kibertoad]

@elhigu Considering that this suppost MSSQL/Postgres/SQLite, isn't that plenty already? We have some features that not work in all DBs already anyway. Would it make sense to merge this one after syncing with master or there is something left to be done?

[elhigu]

If it is possible to implement for mysql / mssql too, it would be better. If it just is not possible, then throwing an error is fine. Also good integration tests would be good to have to verify that feature works consistently between dialects. For example with forUpdate / forShare and and unique index behaviour had to be tuned quite a lot at some point, where I added integration tests that those features acutally behaved the same way between dialects (with .unique constraint IIRC the behaviour with NULL values was the quite different in some DBs).

[elhigu]

Also being able to support those other normal ways to pass the predicates is must to have. Currently only passing query builder was supported #2401 (comment)

[brandonramirez]

I did implement support for MS SQL Server. And it has integration tests. However, MySQL support is impossible because MySQL does not support partial indexes.

[elhigu]

Resolving conflicts and supporting all the normal ways to pass partial predicate should be supported like mentioned in earlier comments.

[smithaitufe]

Hi folks, what is the state of this PR?

[kibertoad]

@smithaitufe There are merge conflicts all over the place. If someone resolves them and addresses the comments, this can be merged.

[dvalsagna89]

someone, please resolve those conflicts. I need this!

[kibertoad]

@dvalsagna89 Would you like to do that?

[dvalsagna89]

@kibertoad i'd do it

[dvalsagna89]

i solved the conflicts but i'm not able to push them. not sure how to do it. never collaborated before 😄

[kibertoad]

@dvalsagna89 I don't think you can push to someone else's fork. Create your own fork, push there and create a new PR.

[brandonramirez]

Is this something I can grant access to? I’d be happy to share my branch.

[kibertoad]

@brandonramirez You'd have to add @dvalsagna89 as a collaborator to your fork.

[dvalsagna89]

@brandonramirez may be better if you solve the conflicts and push. are not to many

[kibertoad]

@dvalsagna89 There are some outstanding comments beyond just conflicts, though.

[brandonramirez]

@dvalsagna89 I added you as a collaborator to my fork repo. I don't have time to resolve merge conflicts right now unfortunately.

[OlivierCavadenti]

i solved the conflicts but i'm not able to push them. not sure how to do it. never collaborated before 😄

@dvalsagna89 do you need help ? anyway if you want I can do it if @brandonramirez add me at collaborator too ^^.

[OlivierCavadenti]

Superseded by #4768