Add support for SQL Server filtered indexes#5504
Add support for SQL Server filtered indexes#5504RamonWill wants to merge 6 commits intosqlalchemy:masterfrom RamonWill:issue4966
Conversation
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 01d1def of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change 01d1def: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
|
nice, let's try it out. |
| @@ -0,0 +1,7 @@ | |||
| .. change:: | |||
| :tags: mssql, usecase | |||
There was a problem hiding this comment.
Noted, changes now made. Thanks
| "name": row["name"], | ||
| "unique": row["is_unique"] == 1, | ||
| "column_names": [], | ||
| "dialect_options": {"mssql_where": row["filter_definition"]}, |
There was a problem hiding this comment.
shouldn't dialect_options be added only when needed like in postgres? for consistency I guess
There was a problem hiding this comment.
Thanks @CaselIT I have now amended the code to only include dialect_options when needed
|
hi @RamonWill you should be able to go to "reviewers" and "request a review" from "sqla-tester" - that kicks off a new build. |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of RamonWill to try to get revision 75c6caa of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset 75c6caa added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
|
My apologies the name "Add error message for lookup" was supposed to be on another issue i have just replace it. the most recent commit is the one that says: "edit changelog tag and amend to only include dialect_options when needed" |
|
ping sqla-tester as a reviewer and the bot will squash your changes into that gerrit. |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of RamonWill to try to get revision b3018ba of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset b3018ba added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
|
Ramon Williams (RamonWill) wrote: sorry for the noise caused by this. I believe i would have to add you manually as a reviewer View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
(13 comments)
OK here's a bunch of review. thanks! looks great
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
- /COMMIT_MSG (line 7): "Add support for SQL Server filtered indexes"
- /COMMIT_MSG (line 14): the "### Checklist" here can be removed
- /COMMIT_MSG (line 33): This seems to work though I'm used to seeing it like:
"Fixes: #4966"
- lib/sqlalchemy/dialects/postgresql/base.py (line 3448): and we can remove "prd" here....
- lib/sqlalchemy/dialects/postgresql/base.py (line 3469): ... and this conditional can be removed. unless something breaks with the sv_idx_name thing, which I'm not totally sure what that is for at the moment.
| @@ -3402,6 +3402,7 @@ def get_indexes(self, connection, table_name, schema, **kw): | |||
| ix.indisunique, ix.indexprs, ix.indpred, | |||
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
we are now looking at ix.indpred in the pg_get_expr() function below, so I think we can remove ix.indpred here....
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| eq_(set(list(t2.indexes)[0].columns), set([t2.c["x col"], t2.c.y])) | ||
|
|
||
| @testing.provide_metadata | ||
| def test_indexes_with_filtered(self): |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
there's a "connection" fixture you can use now:
def test_indexes_with_filtered(self, conneciton):
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| ) | ||
| Index("idx_x", t1.c.x, mssql_where=t1.c.x == "test") | ||
| Index("idx_y", t1.c.y, mssql_where=t1.c.y >= 5) | ||
| metadata.create_all() |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
metadata.create_all(connection)
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| Index("idx_x", t1.c.x, mssql_where=t1.c.x == "test") | ||
| Index("idx_y", t1.c.y, mssql_where=t1.c.y >= 5) | ||
| metadata.create_all() | ||
| with testing.db.connect() as conn: |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
use the "connection" here
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| ) | ||
|
|
||
| @testing.provide_metadata | ||
| def test_index_reflection_partial(self): |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
use connection fixture:
def test_index_reflection_partial(self, connection):
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| Column("x", Integer), | ||
| ) | ||
| metadata.create_all() | ||
| with testing.db.connect().execution_options(autocommit=True) as conn: |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
use connection, don't need autocommit=True as that's deprecated anyway
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
Done
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| ) | ||
| metadata.create_all() | ||
| with testing.db.connect().execution_options(autocommit=True) as conn: | ||
| conn.exec_driver_sql("create index idx1 on table1 ((id || name))") |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
do we need "idx1" in this test? we don't seem to be testing the expression based portion here and there are other tests for the "skipped unsupported reflection" warning below.
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
| with testing.expect_warnings( | ||
| "Skipped unsupported reflection of " | ||
| "expression-based index idx1", | ||
| "Predicate of partial index idx2 ignored during reflection", |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
the "predicate" warning can be removed, because that's what we're reflecting now
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
Ramon Williams (RamonWill) wrote:
(9 comments)
Hi Mike,
I've made changes locally I just have a question in regards to the conditional statement on Line 3469 in lib/sqlalchemy/dialects/postgresql/base.py.
Thank you for the review,
Ramon
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
- /COMMIT_MSG (line 7): Done
- /COMMIT_MSG (line 14): Done
- /COMMIT_MSG (line 33): Done
- lib/sqlalchemy/dialects/postgresql/base.py (line 3469): Hey Mike,
So I've made all the changes from this review locally and they work fine. However, the change here causes test_index_reflection (on line 882, test\dialect\postgresql\test_reflection.py) to fail, since the warning below wont be raised. Shall i amend that test to accomodate this change?
|
mike bayer (zzzeek) wrote: Workflow+1 i think you meant to workflow +1 this View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision b3018ba of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset b3018ba added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
sqla-tester
left a comment
sqla-tester
left a comment
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
(1 comment)
OK we can always pull whatever changes you've made into gerrit here, this is where we review them in any case.
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142
- lib/sqlalchemy/dialects/postgresql/base.py (line 3469): yes, if we are actually implementing Postgresql reflection of expression-based indexes, then we are no longer skipping them and the warning can be removed, as well as the assertions in the test that were checking for that warning.
|
Ramon Williams (RamonWill) wrote: Workflow+1 View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
|
mike bayer (zzzeek) wrote: Code-Review+2 Workflow+2 View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/2142 has been merged. Congratulations! :) |
4 participants
A user requested that the filter definition for Filtered Indexes (MSSQL) are reflected. This change will reflect that. The presence of these filtered definitions will mean that the index is filtered. I have also made this change for PostgreSQL (where they are called Partial Indexes).
Description
I have added a new column in the SELECT clause in Dialect.get_indexes() that will show the filter definition for the index if it exists. This will then be reflected under the dialect_options as postgresql_where or mssql_where depending on the dialect.
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!
Fixes: #4966