-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Removing SQLAlchemy 1 #2690
Removing SQLAlchemy 1 #2690
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.
Interesting read. Didn't manage to get through everything tonight, will finish next week.
db/sql/prototype.sql
Outdated
-- Update table primary key sequence to latest ----------------------------------------------------- | ||
|
||
CREATE OR REPLACE FUNCTION | ||
__msar.update_pk_sequence_to_latest(table_name text, column_ text) RETURNS text AS $$/* |
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.
Is this compatible with multi-column PKs?
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.
No, this is a direct port of the current python function (which also doesn't handle multi-column primary keys).
My plan is to get the current logic ported over while making minimal changes. I'm hoping to use pretty much the same test suite.
However, reviewing all this is a great opportunity to write up bugs and issues about exactly this sort of thing.
I'm pretty sure (hoping) that this function is only called when we're already sure it's a single-column primary key, namely when we've set the default id
column as SERIAL PRIMARY KEY
after the splitting logic runs. If that's wrong, we definitely need a bug report issue for this.
db/sql/prototype.sql
Outdated
viewname, viewcols, __msar.get_table_name(table_id) | ||
); | ||
END; | ||
$$ LANGUAGE plpgsql RETURNS NULL ON NULL INPUT; |
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.
Do I understand this correctly? A Mathesar view is a reflection of a user table. A Mathesar view is algorithmically named according to the user table's oid. Its column values are the reflected user table's column values, and its columns' names are the corresponding reflected table columns' attnums. Would be great to put this in a docstring somewhere around here.
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.
You understand correctly. I'll figure out some reasonable way to document the purpose of these views, and how they work.
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.
Approving. Also asked for a non-critical improvement.
db/install.py
Outdated
@@ -30,6 +35,8 @@ def install_mathesar( | |||
if database_created: | |||
print(f"Installing Mathesar on PostgreSQL database {database_name} at host {hostname}...") | |||
install.install_mathesar_on_database(user_db_engine) | |||
with open(PROTOTYPE_FILE) as f, engine.begin() as conn: | |||
conn.execute(text(f.read())) |
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.
Could you export executing the prototype SQL into a function and replace this block and the indentical blocks on line 23 and on conftest.py:79
with that? Will make it easier to maintain.
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.
Naturally. This bit was a little too prototypey.
Looking good. Ping me if you think this needs a last review. |
@dmos62 Code-wise, this is ready. I'm just setting up some of the surrounding (meta)issues and project stuff so that I can note them in the PR description here for the convenience of future code archaeologists. |
Related to #2737
This removes SQLAlchemy imports from:
db.tables.operations.alter
anddb.tables.operations.drop
.Technical details
This also adds some scaffolding functions in
db.connection
that can be used to avoid wrangling around function signatures until later in the process. These help us run the new functions with old SQLAlchemy objects.It also has a couple of prototype database functions for
The SQL scripts setting these up need to be run manually if you want to use them, since they're not yet integrated into the overall project.
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin