[database] Improve schema defaults #9309
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
All tests are passing, pinging random people @ridz1208 @kongtiaowang if anyone wants to review it, if not I'll just add this PR to the next LORIS meeting. |
ridz1208
approved these changes
Jul 17, 2024
There was a problem hiding this comment.
Visual check passes. I would like IBIS @nicolasbrossard and CCNA @CamilleBeau to give the patch a test.
|
@ridz1208 I ran the patch on CCNA database and did not get any errors |
|
@ridz1208 Ran the patch on IBIS (uses V25) and did not get any errors or warnings. |
maximemulder
added a commit
to maximemulder/Loris
that referenced
this pull request
Sep 25, 2024
1. Drop the `DEFAULT 0` for foreign key fields 2. Change `DEFAULT '0'` (or `'1'`) to `DEFAULT 0` (or `1`) for other fields, which is more correct. Omitted the `mri_protocol.Scan_type` field as it is fixed in aces#9304 instead.
ZhichGaming
pushed a commit
to ZhichGaming/Loris
that referenced
this pull request
Nov 25, 2024
1. Drop the `DEFAULT 0` for foreign key fields 2. Change `DEFAULT '0'` (or `'1'`) to `DEFAULT 0` (or `1`) for other fields, which is more correct. Omitted the `mri_protocol.Scan_type` field as it is fixed in aces#9304 instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Brief summary of changes
While browsing the database schema, I noticed there were a lot of
default '0'everywhere, even for foreign keys ! This is an anti-pattern that is easy to fix and should not require any change to the codebase outside of the schema, so here is a PR.List of changes:
DEFAULT 0for foreign key fields (we do not have any table with a record at index0as far as I know/checked)DEFAULT '0'(or'1') toDEFAULT 0(or1) for other fields, which is more correct (the former can notably raise type errors with some database engines IIRC).I don't think the
ALTER column SET DEFAULT 0is actually useful in the SQL patch, but it cannot hurt.I omitted the
mri_protocol.Scan_typefield as it is fixed in #9304 instead.More broadly, it seems to me that there is an overuse of defaults in our database, but fixing everything would require a lot more work and analysis, and is therefore a job for another day.
Testing instructions (if applicable)