MolSSI / QCFractal

A distributed compute and database platform for quantum chemistry.
https://molssi.github.io/QCFractal/
BSD 3-Clause "New" or "Revised" License
148 stars 48 forks source link

Result table + QCSpec migration + query projection optional join #607

Closed codtiger closed 3 years ago

codtiger commented 4 years ago

Description

Following this, results table and accordingly ResultORM has been changed to follow the normalization process of preserving the five fields of program, basis, driver, method, keywords at one place. Along this change, get_query_projection is also tweaked with two optional arguments supporting joining on a secondary table.

Changelog description

The database migration consisting of schema and data migration for pushing all the five tuples to a seperate table and keeping a qc_spec reference is done in a single alembic update file.(ending in result_to_qc_spec_normalization.py) . ResultORM class is changed accordingly, with a new column called qc_spec and the relationship accompanying it. With these changes, necessary changes was brought to add_results and update_results. Despite the two function changes being similar to their procedure function counterparts, get_results went down a different route which required a mandatory join on two tables results and qc_spec. This change is done with two optional keywords added to the get_query_projection which can be used with a new join table and secondary query conditions(on the join table).

Also, db_queries consisted of some hand-written queries on results which used the previously available five fields to construct ResultRecord objects. These two hand-written queries were also altered to retrieve the correct specification from qc_spec table.

Status

lgtm-com[bot] commented 4 years ago

This pull request introduces 2 alerts when merging ce0e82b284c8582121e716709bb757a94fca47cc into 296d05ce0c9a64774f769ce07f18ba270b9d73b1 - view on LGTM.com

new alerts:

codecov[bot] commented 4 years ago

Codecov Report

Merging #607 into master will increase coverage by 0.02%. The diff coverage is 92.74%.

lgtm-com[bot] commented 4 years ago

This pull request introduces 2 alerts when merging 17101ba8fb8c6cc9d98cdf04243d8697e23f772d into 296d05ce0c9a64774f769ce07f18ba270b9d73b1 - view on LGTM.com

new alerts:

lgtm-com[bot] commented 4 years ago

This pull request introduces 2 alerts when merging d3181e547bd2947ced06dbe1e49d632698d08b16 into aaac042a8c269031a57a3b87ae106b1b9111b68a - view on LGTM.com

new alerts:

bennybp commented 3 years ago

This is being a bit subsumed by a big refactor happening in next. I have a local copy of this branch so will be rebasing/cherry picking these changes into there at some point. So closing this PR.