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

feat: added support for index include #14900

Merged
merged 15 commits into from Nov 17, 2022

Conversation

lohart13
Copy link
Contributor

@lohart13 lohart13 commented Aug 17, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does yarn test or yarn test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

This pull requests adds the ability to support indexes with included columns for MSSQL.
For information on how this works and the benefits, please see Microsoft documentation of this feature.

Fixes #15124

Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to do a more in-depth review later, but these are my initial comments

test/unit/sql/index.test.js Outdated Show resolved Hide resolved
test/integration/dialects/mssql/errors.test.js Outdated Show resolved Hide resolved
src/dialects/mssql/index.js Outdated Show resolved Hide resolved
@lohart13
Copy link
Contributor Author

@WikiRik looking at the test, do you want this to exist under the dialects > mssql folder?
It appears that db2 and postgres can support index with include (in specific versions) therefore not exactly mssql specific.

@WikiRik
Copy link
Member

WikiRik commented Oct 19, 2022

I guess you can include the tests after the following;

But I might prefer a separate test file test/integration/index.test.js since model.test.js already has quite some tests

@lohart13
Copy link
Contributor Author

But I might prefer a separate test file test/integration/index.test.js since model.test.js already has quite some tests

I've updated the test location now.
Let me know if you have any further questions

@lohart13 lohart13 requested a review from WikiRik October 19, 2022 23:06
@WikiRik
Copy link
Member

WikiRik commented Oct 27, 2022

Can you enable the support for db2 and postgres in this PR as well?

@lohart13
Copy link
Contributor Author

@WikiRik I've added support now, however there are some conditions for postgres and db2.

For postgres, the include option for indexes is only supported in version 11 and above therefore this would not be supported for all the versions of postgres that sequelize currently support.

For db2, the include option for indexes is only available for unique indexes.

I've added in some errors and tests to make sure that these limitations are enforced.

Let me know if you have any questions/suggestions

@lohart13 lohart13 changed the title feat(mssql): added support for index include feat: added support for index include Oct 28, 2022
@WikiRik
Copy link
Member

WikiRik commented Oct 28, 2022

Thanks! I think we're dropping support for postgres 10 in v7 as well since the final release of that will come out on November 10th so let's wait with merging this PR until we do so. That makes the implementation easier.

Pinging @ephys @sdepold as well to confirm dropping postgres 10 support as soon as postgres itself does it

@ephys
Copy link
Member

ephys commented Nov 5, 2022

We can drop postgres 10 in v7
image

@WikiRik
Copy link
Member

WikiRik commented Nov 16, 2022

I've created a PR to drop the postgres 10 support; it's #15289. Once that has been merged, this PR can be updated accordingly

@WikiRik WikiRik marked this pull request as draft November 16, 2022 16:56
lohart13 and others added 2 commits November 17, 2022 10:20
Index include is supported by the all sequelize postgres versions
@lohart13 lohart13 marked this pull request as ready for review November 16, 2022 21:49
@lohart13
Copy link
Contributor Author

I've removed the postgres db version checks now.
LMK if there are any further changes required

src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
test/integration/index.test.js Outdated Show resolved Hide resolved
test/unit/sql/index.test.js Outdated Show resolved Hide resolved
test/unit/sql/index.test.js Outdated Show resolved Hide resolved
@lohart13 lohart13 requested review from ephys and removed request for WikiRik November 17, 2022 11:01
ephys
ephys previously approved these changes Nov 17, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like really good work! Thank you :)

WikiRik
WikiRik previously approved these changes Nov 17, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are redundant since the error is clear. But it's a non-blocking issue.
Thanks for working on this!

test/unit/sql/index.test.js Outdated Show resolved Hide resolved
test/unit/sql/index.test.js Outdated Show resolved Hide resolved
test/unit/sql/index.test.js Outdated Show resolved Hide resolved
test/integration/index.test.ts Outdated Show resolved Hide resolved
lohart13 and others added 2 commits November 18, 2022 07:28
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@lohart13 lohart13 dismissed stale reviews from WikiRik and ephys via dab205b November 17, 2022 18:29
@WikiRik WikiRik merged commit dacc806 into sequelize:main Nov 17, 2022
@lohart13 lohart13 deleted the mssql-feat-index-include branch November 17, 2022 19:35
@krzysztofw-hippo
Copy link

Hi, any idea when this will be released? I don't see it in v6.26.0, will it be included in v6.26.1?
Many thanks

@WikiRik
Copy link
Member

WikiRik commented Dec 1, 2022

This will land in the next v7 alpha, there are no plans from the maintainers to backport this to v6 but we're always open to PRs from the community

@lohart13
Copy link
Contributor Author

Tagging PR to sequelize/website#501

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.

SQL Server - Create Index donot have support for "Include"
4 participants