-
Notifications
You must be signed in to change notification settings - Fork 0
Replace raw SQL with ORM #3
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original thought was that other people (external researchers) might store the dq results in their own table with customized names. For now, it should work thought but the goal of Metis is to keep it generic and reusable in different settings.
sqlalchemy: great move, thanks. only one suggestion: could you rename the models.py to something more specific? Models in Metis could be anything (ML models, data models, etc.) - please make clear that these are the models required by sqlalchemy.
05de11d
to
3d27051
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates database operations from raw SQL to SQLAlchemy ORM, eliminating manual DDL statements and parameterized queries in favor of declarative models and ORM sessions. The refactor consolidates common database logic into a base DatabaseWriter
class while maintaining database-specific implementations for SQLite and PostgreSQL.
- Replaces raw SQL CREATE TABLE and INSERT statements with SQLAlchemy declarative models
- Introduces a new
DatabaseWriter
base class to centralize ORM operations - Implements dynamic table name configuration through a model registration pattern
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
metis/writer/sqlite_writer.py | Refactored to extend DatabaseWriter, implements SQLite-specific engine creation |
metis/writer/postgres_writer.py | Refactored to extend DatabaseWriter, implements PostgreSQL-specific engine creation |
metis/writer/database_writer.py | New base class implementing shared ORM logic for database writers |
metis/database_models.py | New file defining SQLAlchemy declarative models with dynamic table name support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3d27051
to
9687025
Compare
Thank you, good point. I've changed the file from |
9687025
to
ed0d84f
Compare
This replaces the table DDLs and insert statements with sqlalchemy.
The ability to set the table name through the config makes handling the models a bit cumbersome, raising the question whether we really need this?
(Does not include migrations using Alembic yet)