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

Load iidbcapabilities and use to build column definition involving unique constraints #58

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

hab6
Copy link
Contributor

@hab6 hab6 commented Jul 1, 2024

Overview

Method added for loading a dictionary of database values retrieved from the iidbcapabilities catalog upon initialization of the IngresDialect class. The dictionary can subsequently be used by IngresDialect class methods to query backend database settings.

Knowledge of the type of backend DBMS is then used in method get_column_specification to correctly handle whether a NOT NULL clause should be appended for columns in CREATE TABLE statements involving UNIQUE constraints when the backend is of type INGRES.

See documentation link for information about the difference between Ingres and X100 tables for columns that are unique constraints.

The values in dictionary iidbcapabilities can be queried directly using known keys, although possibly creating internal getter methods might be worth considering.

Justification for changes

Allowing the dialect to know the backend database configuration is important in some cases when processing SQL statements.

Examples:

  • SQL statements may have different syntax rules depending on if the backend is Ingres or X100.
  • Alphabetic case settings can impact processing of object names (e.g. tables, columns) in SQL statements.

SQLAlchemy standalone program used for testing changes

import sqlalchemy as sa

engine = sa.create_engine('ingres:///testdb', echo=True)
metadata_obj = sa.MetaData()

profile = sa.Table(
    'silly',
    metadata_obj,
    sa.Column('silly_id', sa.Integer, primary_key=True),
    sa.Column('address_id', sa.Integer),
#    sa.Column('address_id', sa.Integer, nullable=False),
    sa.Column('name', sa.String),
    sa.Column('contact', sa.Integer),
    sa.UniqueConstraint('address_id', 'silly_id', name='zz_dingalings_multiple', comment='di unique comment'),
)

metadata_obj.create_all(engine)

 

Testing with the SQLAlchemy Dialect Compliance Suite

Test execution command: pytest --db %dsn% --maxfail=1000 .\test\dialect\test_suite.py

Results

Using the SQLAlchemy Dialect Compliance Suite (lib/sqlalchemy/testing/suite)

Test Configuration Passed Failed Errors Skipped Total
Ingres 11.2 without changes 522 120 33 859 1534
Ingres 11.2 with changes 529 113 33 859 1534
Vector 6.3 with or without changes 323 68 785 (a) 358 1534

(a) Of the 785 errors that occur when running the suite against Vector, 771 involve invalid SQL operations with X100 tables. These issues will be dealt with in part via internal issue II-13753.

Environments

Windows 10 Client

Python 3.10.7
mock              5.1.0
packaging         24.1
pip               24.1.1
platformdirs      4.2.2
pluggy            1.5.0
psycopg2          2.9.9
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.7.dev6
tox               4.15.1
typing_extensions 4.12.2
virtualenv        20.26.3

Backends

Ingres on Windows

Microsoft Windows [Version 10.0.19045.4412
Ingres II 11.2.0 (a64.win/100) 15807

Vector on Ubuntu

Ubuntu 20.04.6 LTS
VW 6.3.0 (a64.lnx/146) 14608

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 as it stands will work for most use cases, but will break under some circumstances.

Can we get the new dictionary attached to the connection some how? For example, is IngresExecutionContext() something that's per connection that we could store those details in?

Anything in the other adapters we can review for ideas?

@@ -81,6 +81,8 @@
'TIMESTAMP WITH LOCAL TIME ZONE': types.TIMESTAMP,
'VARCHAR': types.VARCHAR}

iidbcapabilities = {}
Copy link
Member

Choose a reason for hiding this comment

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

This will make this essentially global to all connections. I.e.:

  1. application with single connection will be fine
  2. application with multiple connections to same database (or type) will be fine
  3. application with more than one connection to different types will end up with last connection meta data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created SQLAlchemy discussion 11574 seeking help with associating a dictionary of catalog options with the related connection.

Copy link
Contributor Author

@hab6 hab6 Jul 12, 2024

Choose a reason for hiding this comment

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

The latest commit fixes a scope/concurrency problem that was present in the initial commit so that the catalog settings are now loaded and distinct for each individual IngresDialect class instance.

I tested the dialect changes with a program that creates multiple simultaneous instances of the SQLAlchemy engine object with corresponding unique database connections to differing backends and there was no interference with the retrieved iidbcapabilities values across the multiple instances of the IngresDialect class.

Attached is simple SQLAlchemy ORM program createTableHouse.py.txt that was also used to test the updated fix. The test code defines column address_id without a NOT NULL constraint (sa.Column('address_id', sa.Integer)) or alternatively with a NOT NULL constraint (sa.Column('address_id', sa.Integer, nullable=False)) and includes the column as part of a UNIQUE constraint. If the backend DBMS_TYPE is INGRES, the Ingres dialect adds the required NOT NULL clause to the column definition. Otherwise, the column definition does not include the NOT NULL clause.

The differences can be seen in the following excerpts showing the generated SQL and column definitions when the program is run against Ingres versus Vector.

Ingres 11.2 database

CREATE TABLE house (
    house_id INTEGER GENERATED BY DEFAULT AS IDENTITY NOT NULL,
    address_id INTEGER NOT NULL, PRIMARY KEY (house_id),
  UNIQUE (house_id, address_id))
                                                                          Key
Column Name                      Type               Length Nulls Defaults Seq
house_id                         integer                 4   no  identity
address_id                       integer                 4   no      no

Vector 6.3 database

CREATE TABLE house (
    house_id INTEGER GENERATED BY DEFAULT AS IDENTITY NOT NULL,
    address_id INTEGER, PRIMARY KEY (house_id),
  UNIQUE (house_id, address_id))
                                                                          Key
Column Name                      Type               Length Nulls Defaults Seq Minmax
house_id                         integer                 4   no  identity       Y
address_id                       integer                 4  yes    null         Y

I re-ran the dialect compliance suite with and without the latest changes to the Ingres dialect and the results were the same as in the opening comment of this PR. With the latest changes against Ingres 11.2, there were 7 more tests that passed than failed as compared to a run using the original (pre-PR) Ingres dialect code as a baseline..

@clach04 clach04 merged commit e4d4ed7 into master Jul 15, 2024
@hab6 hab6 deleted the hab6-get-initial-iidbcap branch July 15, 2024 19:47
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