add partial index predicate to SQLiteDialect.get_indexes() result by tgpfeiffer · Pull Request #8806 · sqlalchemy/sqlalchemy

tgpfeiffer · 2022-11-15T10:04:29Z

This fixes #8804 by populating the dialect_options's sqlite_where field inside SQLiteDialect.get_indexes() for an index where the index_list() pragma indicates that it is a partial index.

Description

For an index where the fifth column of the corresponding row in the index_list() return value is true (see https://www.sqlite.org/pragma.html#pragma_index_list), we get the exact SQL used to create that index from the sqlite_master table and look for the WHERE keyword, then use the string after that as the sqlite_where predicate.

Remark: I use a simple regular expression scanning for the string )\s+where\s+ (case-insensitive) to find the predicate. As written in a comment in the code, this can easily fail if the definition of the index contains a matching string such as

CREATE INDEX i ON t (col || ') where') WHERE col <> ''         -- case 1
CREATE INDEX i ON t (col)              WHERE col <> ') where'  -- case 2

but case 1 can be neglected because get_indexes() currently does not support expression-based indexes, and case 2 can be neglected because the regex finds the first match.

@CaselIT suggested in #8804 (comment) to attack expression-based indexes as well, but to be honest I am not sure what is a good way to parse the CREATE INDEX statement with reasonable effort in that case, so I didn't work on it. (Is there a parser somewhere in the codebase already?)

I have verified that in my local environment, the case I reported in sqlalchemy/alembic#1112 (comment) works well, so the partial index is kept across migrations and this PR also fixes sqlalchemy/alembic#1112.

Checklist

This pull request is:

A documentation / typographical error fix
- Good to go, no issue or tests are needed
A short code fix
- please include the issue number, and create an issue if none exists, which
  must include a complete example of the issue. one line code fixes without an
  issue and demonstration will not be accepted.
- Please include: Fixes: #<issue number> in the commit message
- please include tests. one line code fixes without tests will not be accepted.
A new feature implementation
- please include the issue number, and create an issue if none exists, which must
  include a complete example of how the feature would look.
- Please include: Fixes: #<issue number> in the commit message
- please include tests.

Have a nice day!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 0bd2750 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-11-15T13:39:29Z

New Gerrit review created for change 0bd2750: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

tgpfeiffer · 2022-11-16T09:38:36Z

I think I addressed your comments @CaselIT, could you take another look please? Thank you!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 56f16e3 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-11-16T21:06:18Z

Patchset 56f16e3 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester

Federico Caselli (CaselIT) wrote:

looks ok to me now. Thanks for updating the change

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-16T21:17:08Z

+                        "Failed to look up filter predicate of "
+                        "partial index %s" % row[1]
+                    )
+                    indexes.pop()


Federico Caselli (CaselIT) wrote:

this is a change compared to how the dialect works now (since it currently returns partial indexes without informations instead of ignoring them if it cannot parse them)

but since it's in an edge case that should never happen, at least when used with sqlite, I think it's fine

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

tgpfeiffer · 2022-11-18T01:38:21Z

Sorry I am not familiar with the review process, what is the next step to get this merged, looks like I need another approval?

On a related note, I would also like to see this change in the next 1.4.x bugfix release, can I do anything to help with the backporting? (I already noticed there will be a merge conflict, so it needs some manual work.)

CaselIT · 2022-11-18T07:01:38Z

Sorry I am not familiar with the review process, what is the next step to get this merged, looks like I need another approval?

Mostly wait, Mike will get to this when he has time

On a related note, I would also like to see this change in the next 1.4.x bugfix release, can I do anything to help with the backporting? (I already noticed there will be a merge conflict, so it needs some manual work.)

I placed it on the 2 milestone, but it seems contained enough to be backported to 1.4. let's wait for mike opinion on this

sqla-tester

mike bayer (zzzeek) wrote:

hi thanks for this! this is pretty good. I'm assuming we want this in 1.4 as 2.0 final is still some months away. Let's narrow the scope of the new logic, and there are also changes for the test suite; in the latter case, we can do that part if you dont know our test suite conventions.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-20T14:46:46Z

+                    indexes[-1]["dialect_options"]["sqlite_where"] = text(
+                        predicate
+                    )
+                except Exception:


mike bayer (zzzeek) wrote:

what Exception are we catching here? is this just if the partial_pred_re is None? Let's just assign it up front and do "if predicate_match is not None"

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-20T14:46:46Z

+                        "Failed to look up filter predicate of "
+                        "partial index %s" % row[1]
+                    )
+                    indexes.pop()


mike bayer (zzzeek) wrote:

this would be much safer to backport to 1.4 if we didnt actually do this indexes.pop() here, and return the same output we would have gotten previously. We can leave the new warning in place, which should be sufficient.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-20T14:46:47Z

 import datetime
 import json
 import os
+from unittest.mock import ANY


mike bayer (zzzeek) wrote:

for backporting this has to be "from testing.mock import ANY"

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-20T14:46:47Z

            ],
        )

+    def test_reflect_partial_indexes(self):


mike bayer (zzzeek) wrote:

use the connection fixture

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-20T14:46:47Z

+    def test_reflect_partial_indexes(self):
+        with testing.db.begin() as conn:
+            conn.exec_driver_sql(
+                "create table foo_with_partial_index (x integer, y integer)"


mike bayer (zzzeek) wrote:

do all of these individually using a @testing.combinations() block

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

tgpfeiffer · 2022-11-21T05:45:21Z

(Not sure why some reviewer comments show up in github and others in gerrit only... 🤔)

I addressed three of your comments (fix mock import, remove indexes.pop(), use an is None for the regex). As for the @testing.combinations() block, as @CaselIT wrote in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200/2/test/dialect/test_sqlite.py#2310 I wrote it that way to test that we got the list right for multiple indexes. About the connection fixture @CaselIT wrote "Done" in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200/comment/de64c240_86533cb7/ but I am not sure where it is done 😅

Please take another look and let me know if I should fix anything else.

zzzeek · 2022-11-21T16:39:47Z

(Not sure why some reviewer comments show up in github and others in gerrit only... thinking)

we have a mirror between the gerrit review and this PR. they should be bidrectional at the comments level.

I addressed three of your comments (fix mock import, remove indexes.pop(), use an is None for the regex). As for the @testing.combinations() block, as @CaselIT wrote in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200/2/test/dialect/test_sqlite.py#2310 I wrote it that way to test that we got the list right for multiple indexes. About the connection fixture @CaselIT wrote "Done" in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200/comment/de64c240_86533cb7/ but I am not sure where it is done sweat_smile

that would replace the engine.begin() and would look like this:

  def test_reflect_partial_indexes(self, connection):
        connection.exec_driver_sql(...)

the thing we are using there is pytest fixtures.

CaselIT · 2022-11-21T18:10:32Z

yes, it was a mistake on my part that done comment

sqla-tester

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 44cd132 of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-11-21T23:03:36Z

Patchset 44cd132 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

tgpfeiffer · 2022-11-21T23:52:11Z

Pushed another commit making use of the connection feature. I think I addressed all your comments now.

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 539dfcb of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-11-22T00:15:24Z

Patchset 539dfcb added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

tgpfeiffer · 2022-11-28T04:12:05Z

Sorry to bug you again, could you give this another round of review, please? Thank you!

sqla-tester

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 539dfcb of this pull request into gerrit so we can run tests and reviews and stuff

sqla-tester · 2022-11-28T12:52:38Z

Patchset 539dfcb added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200

sqla-tester · 2022-11-29T16:17:49Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4200 has been merged. Congratulations! :)

sqla-tester · 2022-11-29T16:19:03Z

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4251 has been merged. Congratulations! :)

Added support for reflection of expression-oriented WHERE criteria included in indexes on the SQLite dialect, in a manner similar to that of the PostgreSQL dialect. Pull request courtesy Tobias Pfeiffer. Fixes: #8804 Closes: #8806 Pull-request: #8806 Pull-request-sha: 539dfcb Change-Id: I0e34d47dbe2b9c1da6fce531363084843e5127a3 (cherry picked from commit ed39e84)

tgpfeiffer added 3 commits November 15, 2022 13:22

extract partial index flag from index_list() pragma call

af4a0a3

add predicate into dialect_options

6893ebe

use a text() for sqlite_where, not str

0bd2750

tgpfeiffer changed the title ~~extract partial index flag from index_list() pragma call~~ add partial index predicate to SQLDialect.get_indexes() result Nov 15, 2022

tgpfeiffer changed the title ~~add partial index predicate to SQLDialect.get_indexes() result~~ add partial index predicate to SQLiteDialect.get_indexes() result Nov 15, 2022

zzzeek requested a review from sqla-tester November 15, 2022 13:39

sqla-tester reviewed Nov 15, 2022

View reviewed changes

tgpfeiffer added 3 commits November 16, 2022 13:27

fix linter errors

6853912

extend test case

f262486

add exception handling

56f16e3

CaselIT requested a review from sqla-tester November 16, 2022 21:06

sqla-tester reviewed Nov 16, 2022

View reviewed changes

sqla-tester reviewed Nov 20, 2022

View reviewed changes

tgpfeiffer added 2 commits November 21, 2022 14:38

use 'is None' check for regex match

219af6e

use sqlalchemy.testing for mock

44cd132

CaselIT requested a review from sqla-tester November 21, 2022 23:03

sqla-tester reviewed Nov 21, 2022

View reviewed changes

use connection fixture

539dfcb

zzzeek requested a review from sqla-tester November 22, 2022 00:15

sqla-tester reviewed Nov 22, 2022

View reviewed changes

zzzeek requested a review from sqla-tester November 28, 2022 12:52

sqla-tester reviewed Nov 28, 2022

View reviewed changes

sqlalchemy-bot closed this in ed39e84 Nov 29, 2022

Uh oh!

Conversation

tgpfeiffer commented Nov 15, 2022

Description

Checklist

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Nov 15, 2022

Uh oh!

tgpfeiffer commented Nov 16, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Nov 16, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sqla-tester Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgpfeiffer commented Nov 18, 2022

Uh oh!

CaselIT commented Nov 18, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

sqla-tester Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

sqla-tester Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

sqla-tester Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

sqla-tester Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

tgpfeiffer commented Nov 21, 2022

Uh oh!

zzzeek commented Nov 21, 2022

Uh oh!

CaselIT commented Nov 21, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Nov 21, 2022

Uh oh!

tgpfeiffer commented Nov 21, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Nov 22, 2022

Uh oh!

tgpfeiffer commented Nov 28, 2022

Uh oh!

sqla-tester left a comment

Choose a reason for hiding this comment

Uh oh!

sqla-tester commented Nov 28, 2022

Uh oh!

sqla-tester commented Nov 29, 2022

Uh oh!

sqla-tester commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels