Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix: include schema when creating indices #637

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

Merged
merged 2 commits into from
May 2, 2025

Conversation

waltaskew
Copy link
Contributor

@waltaskew waltaskew commented Apr 18, 2025

Fixes an issue where indices aren't able to be created for tables in schemas. At present, create_index emits SQL like

CREATE INDEX index_name ON schema_name.table_name (col)

which results in an error when creating the table:

Index index_name cannot index a table/index schema_name.table_name
which is in a different named schema.

because the name needs to be prefixed by schema.

drop_index does emit the correct SQL with the schema prefix.

DROP INDEX schema_name.index_name

This is due to an odd issue where the base class's visit_create_index method has in its signature
include_schema=False:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/sql/compiler.py#L6828C23-L6828C43

but visit_drop_index hard-codes the value to True:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/sql/compiler.py#L6870

The dialects handle this by hard-coding include_schema=True in visit_create_index, e.g.
sqlite:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/dialects/sqlite/base.py#L1740

The difference in defaults is odd in the base class, but it seems like include_schema=True is the appropriate setting for Spanner.

Part of #638

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner-sqlalchemy API. label Apr 18, 2025
@waltaskew waltaskew force-pushed the include-schema-on-create-index branch from 7b0af8e to 2936ea3 Compare April 18, 2025 00:17
Fixes an issue where indices aren't able to be created for tables in
schemas. At present, `vist_create_index` emits SQL like

```sql
CREATE INDEX index_name (col) ON schema_name.table_name
```
which results in an error when creating the table:
```
Index index_name cannot index a table/index schema_name.table_name
which is in a different named schema.
```
because the name needs to be prefixed by schema.

`vist_drop_index` does emit the correct SQL with the schema prefix.
```sql
DROP INDEX schema_name.index_name
```

This is due to an odd issue where the base class's
`visit_create_index` method has in its signature
`include_schema=False`:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/sql/compiler.py#L6828C23-L6828C43

but `visit_drop_index` hard-codes the value to `True`:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/sql/compiler.py#L6870

The dialects handle this by hard-coding `include_schema=True` in
`visit_create_index`, e.g.
sqlite:

https://github.com/sqlalchemy/sqlalchemy/blob/299284cec65076fd4c76bf1efaae60b60f4d4f7b/lib/sqlalchemy/dialects/sqlite/base.py#L1740

The difference in defaults is odd in the base class, but it seems like
include_schema=True is the appropriate setting for Spanner.
@waltaskew waltaskew force-pushed the include-schema-on-create-index branch from 2936ea3 to 2d1c08b Compare April 18, 2025 00:18
@olavloite olavloite force-pushed the include-schema-on-create-index branch from 2a0f34f to f65a0b7 Compare May 2, 2025 11:11
@olavloite olavloite changed the title Include Schema When Creating Indices fix: include schema when creating indices May 2, 2025
@olavloite olavloite added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 2, 2025
@olavloite olavloite merged commit 41905e2 into googleapis:main May 2, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner-sqlalchemy API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.