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

Return only user defined indexes from IngresDialect::get_indexes #60

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

hab6
Copy link
Contributor

@hab6 hab6 commented Jul 18, 2024

Overview

A number of tests in area ComponentReflectionTest::get_multi_indexes of the SQLAlchemy Dialect Compliance Suite fail with an AssertionError caused by mismatched actual vs expected results.

Stack dumps from running with pytest reveal that the test suite expects only user defined indexes to be returned by the get_multi_index tests, whereas the Ingres dialect returns all indexes, including internal indexes defined automatically by the DBMS.

Here is a simple generic example, showing creation of a user index versus a system generated index:

=> CREATE TABLE staff (id INTEGER NOT NULL, ssn INTEGER NOT NULL, PRIMARY KEY (id), UNIQUE (id, ssn))
=> CREATE INDEX staffssn ON staff (ssn)

=> SELECT index_name, system_use FROM iiindexes WHERE base_name = 'staff'
    index_name                  system_use
    $staff_u000001820000        G
    staffssn                    U

Ingres documentation excerpt on iiindexes catalog :

Column Name Data Type Description
system_use char(1) S if the object is a system object
U if user object
G if generated by the system for the user
Blank if unknown
Used by utilities to determine which tables need reloading

Internal issue II-14927

Code change

The SQL statement used in IngresDialect::get_indexes is modified to also filter on the system_use column and retrieve only user defined indexes (ignoring system generated indexes).

Testing - SQLALchemy Dialect Compliance Suite

Command

pytest --db ingres_odbc -vv --maxfail=10000 .\test\dialect\test_suite.py --tb=no

(test.cfg contents: ingres_odbc=ingres:///sqatestdb)

Results - DBMS: Ingres 12.0

Tests Passed Failed Errors Skipped Total
Before Fix 532 110 33 859 1534
With Fix 548 94 33 859 1534

Test results with DBMS targets: Vector 6.3 and Data Platform warehouse 620.0.18 (GCS)

This code change has NO impact when the backend is Vector or Data Platform, since the 84 tests of the dialect compliance suite under area ComponentReflectionTest::get_multi_indexes all involve DDL that tries to create a table with a self-referential constraint, which is not allowed on X100 tables.

When attempting to run the get_multi_indexes tests against an X100 backend, each of the tests fail with this error:

sqlalchemy.exc.DBAPIError: (pyodbc.Error) ('50000', "[50000] [Actian][Actian ODBC Driver
[INGRES]CREATE/ALTER TABLE: You cannot create a self referencing
referential constraint on table 'users' in schema 'actian'
because it is an X100 table. (328949) (SQLExecDirectW)")
[SQL:
CREATE TABLE users (
    user_id INTEGER GENERATED BY DEFAULT AS IDENTITY NOT NULL,
    test1 CHAR(5) NOT NULL,
    test2 FLOAT NOT NULL,
    parent_user_id INTEGER,
    PRIMARY KEY (user_id),
    CONSTRAINT zz_test2_gt_zero CHECK (test2 > 0),
    CHECK (test2 <= 1000),
    CONSTRAINT user_id_fk FOREIGN KEY(parent_user_id) REFERENCES users (user_id)
)

Environment

Microsoft Windows [Version 10.0.19045.4529]

Python 3.10.7

mock              5.1.0
packaging         24.1
pip               24.1.2
platformdirs      4.2.2
pluggy            1.5.0
pyodbc            5.1.0
pyproject-api     1.7.1
pypyodbc          1.3.6
pytest            8.2.2
setuptools        63.2.0
SQLAlchemy        2.0.30.dev0
sqlalchemy-ingres 0.0.8.dev0
tox               4.16.0
typing_extensions 4.12.2
virtualenv        20.26.3

DBMS:  II 12.0.0

@hab6 hab6 marked this pull request as ready for review July 19, 2024 14:44
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.

Code looks good, commit message really should have had the PR text in the commit message for explanation.

I understand this fixes the test suite, I think we should offer an option to return system index information (off by default so test suite just runs). Either connection string option (preferred) or environment variable control..

Thoughts?

@@ -802,6 +802,7 @@ def get_indexes(self, connection, table_name, schema=None, **kw):
WHERE
i.index_name = c.index_name
AND i.index_owner = c.index_owner
AND i.system_use = 'U'
Copy link
Member

Choose a reason for hiding this comment

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

Code looks good, I think this should be conditional under user control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review and suggestion.

Added logic allowing inspection of system generated indexes by setting connection execution option inspect_indexes="ALL". The default is to return only user-defined indexes.

Example program to check additional code: inspectIndexes.py.txt

Output:

C:\test\> python inspectIndexes.py MYURL1
Inspect1: [{'name': 'address_idx', 'column_names': ['address_id'], 'unique': False}]
Inspect2: [{'name': 'address_idx', 'column_names': ['address_id'], 'unique': False}, {'name': '$house_u000001eb00000000', 'column_names': ['house_id', 'address_id'], 'unique': True}]
Inspect3: [{'name': 'address_idx', 'column_names': ['address_id'], 'unique': False}]

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.

Looks good! Thanks for the new feature :-)

@hab6 hab6 merged commit 87c2cbd into master Jul 22, 2024
@hab6 hab6 deleted the hab6-get-only-user-indexes branch July 22, 2024 13:20
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