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

Added method get_unique_constraints II-14372 #52

Merged
merged 1 commit into from
May 31, 2024

Conversation

hab6
Copy link
Contributor

@hab6 hab6 commented May 16, 2024

Description

Added reflection method get_unique_constraints to retrieve names of existing unique constraints.

Error fixed when running the SQLAlchemy dialect compliance suite

File "C:\.....\sqlalchemy\lib\sqlalchemy\engine\interfaces.py", line 1613,
 in get_unique_constraints
raise NotImplementedError()
NotImplementedError

Testing - How to reproduce (before fix)

  1. Configure SQLAlchemy dialect compliance suite testing according to the instructions in README.testsuite.md
  2. Run unique_constraints tests:
    pytest --db ingres_odbc --maxfail=100 test_suite_reflection.py -k "test_get_multi_unique_constraints"

Impact of fix

Results Passed Failed
Before Fix 0 42
After Fix 26 16

The remaining 16 failed tests are caused by different issues and will be handled separately.

Environment

Microsoft Windows [Version 10.0.19045.4291]
II 11.2.0 (a64.win/100) 15807
Python 3.10.7
mock              5.1.0
packaging         24.0
pip               24.0
pluggy            1.5.0
pyodbc            5.1.0
pyproject-api     1.6.1
pypyodbc          1.3.6
pytest            8.2.0
setuptools        63.2.0
SQLAlchemy        2.0.29.dev0
sqlalchemy-ingres 0.0.7.dev4ingres
tox               4.15.0
typing_extensions 4.11.0
virtualenv        20.26.2

Internal ticket II-14372

@hab6 hab6 requested review from clach04, mianculovici and vsreeniv May 16, 2024 22:00
@hab6 hab6 changed the title New method get_unique_constraints II-14372 Added method get_unique_constraints II-14372 May 16, 2024
Copy link
Member

@clach04 clach04 left a comment

Choose a reason for hiding this comment

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

Nothing but good things to save about this. I suspect we need at least a few additional tests added to (our) test suite.

It's been a while since I got my hands dirty with Ingres specific schemas. I think this change will work fine for ANY DDL issued via SA (orm). I suspect it will miss a few cases:

  1. modify table to BTREE/ISAM unique - I do NOT think this shows up in that meta data table
  2. create index unique - same as above

Constraints are created under the covers as case number 2 above BUT show up in additional syscats.

Recommend manual test for now, if I'm wrong we're done. If I'm correct then open new issue and get this merged with a link.

@hab6
Copy link
Contributor Author

hab6 commented May 31, 2024

Opened new issues prompted by comments

#53 Create test suite for the SQLAlchemy Ingres connector
#54 Add support for unique constraints when using BTREE & ISAM
#55 Review Ingres dialect support for unique indexes

@hab6 hab6 merged commit 39ae514 into master May 31, 2024
@hab6 hab6 deleted the hab6-get-unique-constraints branch May 31, 2024 16:58
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.

2 participants