Repository navigation
fix(sqlalchemy-spanner): group multi-column unique constraints and add get_multi_unique_constraints - #18842
Open
rocksnow1942 wants to merge 4 commits into
Open
fix(sqlalchemy-spanner): group multi-column unique constraints and add get_multi_unique_constraints#18842rocksnow1942 wants to merge 4 commits into
rocksnow1942 wants to merge 4 commits into
Conversation
…d get_multi_unique_constraints get_unique_constraints() returned one entry per column of a multi-column unique constraint, in no particular order. Add get_multi_unique_constraints(), which reads the unique constraints of all tables in one query with their columns in KEY_COLUMN_USAGE order, and make get_unique_constraints() use it, like the other get_*() methods. Fixes googleapis#18836 Fixes googleapis#18837
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the get_multi_unique_constraints method and refactors get_unique_constraints to use it, alongside adding corresponding integration and unit tests. However, a critical issue was identified in the unit tests where the test_max_size_exported function definition was accidentally deleted, causing its body to be incorrectly merged into test_get_unique_constraints.
The previous commit dropped its def line, so its body ran as part of test_get_unique_constraints.
rocksnow1942
commented
Oct 24, 2026
rocksnow1942
commented
Oct 24, 2026
…unique constraints query Also write the INFORMATION_SCHEMA table and column names in upper case, as the replaced query did.
Group by schema, table and constraint name and collect the columns with ARRAY_AGG(... ORDER BY ORDINAL_POSITION), instead of ordering one row per column and grouping them in Python.
rocksnow1942
marked this pull request as ready for review
October 24, 2026 10:27
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SpannerDialect.get_unique_constraints()read the columns of unique constraints fromINFORMATION_SCHEMA.CONSTRAINT_COLUMN_USAGE, which has no column position, and returned one entry per column. A two-column constraint came back as two single-column constraints with the same name, in no particular order. On the emulator, which repeats the backing unique constraint of a foreign key once per referencing foreign key, one two-column constraint came back as eight entries.SpannerDialectalso didn't implementget_multi_unique_constraints(), so multi-table reflection (MetaData.reflect(),Inspector.get_multi_unique_constraints()) fell back to one query per table.This PR:
get_multi_unique_constraints(), which reads the unique constraints of all tables in one query with their columns fromKEY_COLUMN_USAGEinORDINAL_POSITIONorder, and skips the columns the emulator repeats;get_unique_constraints()call it, asget_columns(),get_pk_constraint(),get_foreign_keys()andget_indexes()do with theirget_multi_*()methods;test_get_multi_unique_constraintscompliance test with a Spanner override.A question for reviewers: Should we leave out the unique constraints?
Spanner DDL has no unique constraints, and unique indexes aren't listed in
TABLE_CONSTRAINTS, so the onlyUNIQUErows there are the indexes that Spanner creates to back foreign keys that refer to columns other than the primary key (IDX_<table>_<columns>_U_<hash>).get_multi_indexes()already leaves these Spanner-managed indexes out. Should the unique constraints leave them out too? Alembic's autogenerate emitsop.drop_constraint()for them, which Spanner rejects with... is not a constraint in .... This PR keeps returning them, as before.Tested with the unit and mockserver tests and the SQLAlchemy 1.4 and 2.x compliance suites against the emulator.
Fixes #18836
Fixes #18837