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

DM-41247: Add tool that checks for redundant datatype definitions #50

Merged
merged 19 commits into from Apr 12, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

This adds a Pydantic validator that will identity variant type overrides, e.g., mysql:datatype values, that appear to be redundant. To determine whether the datatypes are the same, they are each compiled to a SQL snippet. If the resultant SQL is identical, then the type override is considered to be superfluous and an error is raised.

The validation needs to be turned on using the -t/--check-redundant-datatypes flag when running the validate command.

Pydantic itself uses a global settings dictionary for data models, so
follow this style for validation options.
This was needed in SQLAlchemy v1 but should not be needed in v2.
If there are 'mysql:datatype' annotations present in the column data,
check that they are not redundant.

The datatypes are checked to see if they compile to the same SQL
string in the target dialect.
Copy link
Collaborator Author

@JeremyMcCormick JeremyMcCormick left a comment

Choose a reason for hiding this comment

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

Few changes

python/felis/datamodel.py Outdated Show resolved Hide resolved
tests/test_datatypes.py Outdated Show resolved Hide resolved
tests/test_datatypes.py Show resolved Hide resolved
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Functionally this is what was agreed. Not enough time to review the code in detail, but it generally looks consistent with the package and our style rules.

@JeremyMcCormick JeremyMcCormick merged commit a32f5f9 into main Apr 12, 2024
9 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-41247 branch April 12, 2024 19:54
timj added a commit that referenced this pull request Apr 12, 2024
This reverts commit a32f5f9, reversing
changes made to 0d31a49.

Some global state is being left uncleared which is causing test
failures with xdist that depend on how many processes are
being used.
timj added a commit that referenced this pull request Apr 12, 2024
DM-41247: Revert "Merge pull request #50 from lsst/tickets/DM-41247"
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.

None yet

2 participants