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

Properly reflect column attributes: nulllable, autoincrement, comment #61

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

hab6
Copy link
Contributor

@hab6 hab6 commented Jul 25, 2024

Overview

Changes are made to class method IngresDialect::get_columns to provide reflection of column attributes autoincrement and comment and to correctly handle columns that are defined as unique constraints.

The column attributes fix was prompted by failing tests from ComponentReflectionTest::test_get_multi_columns in the SQLAlchemy dialect compliance suite in which the actual reflected results were missing the attributes autoincrement and comment which are part of the expected results.

Notes on Unique Constraints and Null Values

Per Ingres documentation, Ingres tables do not allow columns specified as unique to contains nulls whereas X100 tables do allow columns specified as unique to contain null values.

PostgreSQL doc indicates that column members of unique constraints can be null.

The SQLAlchemy get_multi_columns tests expect that null values are allowed for any column that is part of a unique constraint, and thus with an Ingres database, a number of these tests continue to fail with the code fix of this PR, even though the fix allows the inspection results returned by the dialect to be correct for Actian databases (Ingres, Vector, Data Platform).

Many of the related dialect compliance suite tests fail when the database is VectorWise due to an unrelated issue, because the suite tests try to create a table with a self-referencing referential constraint, which is not allowed with X100 tables.

Testing - Dialect Compliance Suite

Ingres 12.0.0

Results Passed Failed Errors Skipped Total
Before Fixes 548 94 33 859 1534
After Fixes 548 94 33 859 1534

Vector 6.3

Results Passed Failed Errors Skipped Total
Before Fixes 323 68 785 358 1534
After Fixes 324 67 785 358 1534

It is worth mentioning that in my testing, the dialect compliance suite tests sometimes give differing results for multiple runs even if nothing is changed in between test runs. For example, if the test suite is run 10 times in a row without changing anything in between, the pass/fail rates for half of the runs might be different than the results for the other half of the runs. I have not spent much time trying to understand the reason for this behavior, although suspect it could be a timing issue with the creation/deletion of objects.

Testing - Simple Python program that reflects columns attributes

inspect_columns.py.txt

Given the following table definition in the attached program, results are show below.

sqlalchemy.Table(
    'house',
    metadata_obj,
    sqlalchemy.Column('house_id', sqlalchemy.Integer, primary_key=True),
    sqlalchemy.Column('address_id', sqlalchemy.Integer),
    sqlalchemy.Column('lot_id', sqlalchemy.Integer, comment='County record'),
    sqlalchemy.Column('data', sqlalchemy.String(10), unique=True),
    sqlalchemy.UniqueConstraint('house_id', 'address_id'), )

With the fixes of this PR, the attributes/values for nullable, autoincrement, and comment in the following inspection results are now correct.

Results with database: Ingres 12.0.0 GA

Inspection: [
{'name': 'house_id', 'nullable': False, 'default': 'next value for mhabiger"."$iiidentity_sequence_0000297"', 'type': Integer(), 'autoincrement': True, 'comment': None},
{'name': 'address_id', 'nullable': False, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': None},
{'name': 'lot_id', 'nullable': True, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': 'County record'},
{'name': 'data', 'nullable': False, 'default': None, 'type': VARCHAR(length=10), 'autoincrement': False, 'comment': None}]

Results with database: VectorWise 6.3 build 14614

Inspection: [
{'name': 'house_id', 'nullable': False, 'default': 'next value for "actian"."$iiidentity_sequence_0000364"', 'type': Integer(), 'autoincrement': True, 'comment': None},
{'name': 'address_id', 'nullable': True, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': None},
{'name': 'lot_id', 'nullable': True, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': 'County record'},
{'name': 'data', 'nullable': True, 'default': None, 'type': VARCHAR(length=10), 'autoincrement': False, 'comment': None}]

Results with database: Actian Data Platform 620.0.18 GCS

Inspection: [
{'name': 'house_id', 'nullable': False, 'default': 'next value for "vmxuser"."$iiidentity_sequence_0000384"', 'type': Integer(), 'autoincrement': True, 'comment': None},
{'name': 'address_id', 'nullable': True, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': None},
{'name': 'lot_id', 'nullable': True, 'default': None, 'type': Integer(), 'autoincrement': False, 'comment': 'County record'},
{'name': 'data', 'nullable': True, 'default': None, 'type': VARCHAR(length=10), 'autoincrement': False, 'comment': None}]

Client 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
psycopg2          2.9.9
pyodbc            5.1.0
pyproject-api     1.7.1
pypyodbc          1.3.6
pytest            8.3.1
setuptools        63.2.0
SQLAlchemy        2.0.30.dev0
sqlalchemy-ingres 0.0.8.dev1
tomli             2.0.1
tox               4.16.0
typing_extensions 4.12.2
virtualenv        20.26.3

Internal ticket II-14573

@hab6 hab6 changed the title Reflect column autoincrement and comment Reflect column attributes: autoincrement and comment Jul 26, 2024
@hab6 hab6 marked this pull request as ready for review July 26, 2024 17:42
@hab6 hab6 changed the title Reflect column attributes: autoincrement and comment Properly reflect column attributes: nulllable, autoincrement, comment Jul 26, 2024
@@ -505,7 +534,10 @@ def get_columns(self, connection, table_name, schema=None, **kw):
coldata['type'] = ischema_names[coltype](precision, scale)
else:
coldata['type'] = ischema_names[coltype]

coldata['autoincrement'] = (row[6] == 'Y' or row[7] == 'Y')
Copy link
Member

Choose a reason for hiding this comment

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

Not required, a comment here on what 6/7 represent could be helpful here.

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. Added comment for what row[6] and row[7] represent.

@hab6 hab6 merged commit 4bacec1 into master Jul 29, 2024
@hab6 hab6 deleted the hab6-autoincrement-and-comment branch July 29, 2024 16:34
@hab6 hab6 mentioned this pull request Jul 29, 2024
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