[database] Clean up mri_scan_type #9304

maximemulder · 2024-06-26T15:37:01Z

Summary of changes

Add missing foreign keys.
Rename Scan_type foreign key fields to MriScanTypeID.
Remove the default value of 0 for mri_protocol.MriScanTypeID (as this is a non-nullable foreign key, I don't think this should ever happen).
Adapt the existing queries and scripts in LORIS.

Testing instructions

Apply the patch
Reinstall Raisinbread
Test the associated LORIS feature

As we unfortunately have a very loose typing in LORIS, I may have missed a few bugs when adapting the scripts, but we're early on the next version development cycle so I guess it's the right time to do such changes.

Alternatives

The foreign key fields could just be named ScanTypeID (it's obvious it's an "MRI" scan type).
Should the files.AcquisitionTypeID also be renamed to files.MriScanTypeID ?
Should mri_scan_type.Scan_type be renamed to mri_scan_type.Name ?

Related issues

Resolves [imaging/database] Clean-up mri_protocol tables #9284
Complementary LORIS-MRI PR 1139

maximemulder · 2024-06-26T17:46:48Z

I would like to have some opinions on the alternatives before merging. Also I don't know who should review it so I'll just ping @ridz1208 and feel free to give your opinion or re-assign it.

maximemulder · 2024-07-04T15:19:30Z

After the MRI meeting today, we agreed on the following TODOs before reviewing/merging this PR :

Add a unique constraint on the scan type name as per [database] Scan types can have identical names #9306 .
Apply the alternatives 2 and 3.

maximemulder · 2024-07-16T17:55:32Z

Okay, I tested in LORIS as well as LORIS-MRI (imaging pipeline: dcm2mnc -> defacing -> mnc2bids) and everythings seem to wok, PR is ready for review/mege @cmadjar @nicolasbrossard. The PR is basically a SQL patch and renaming the fields in SQL queries (and some auto-formatting, feel free to ignore whitespace changes).

cmadjar

@maximemulder my initial review on the LORIS side for this.

One generic comment based on the spacing I see in your PR. In LORIS, we use the following convention: 1 tab = 4 spaces. I think you need to change your editor configuration to reflect that convention.

I also added some comments for @driusan and @nicolasbrossard to benefit from their wisdom on some of the changes :)

cmadjar · 2024-08-01T14:56:12Z

SQL/0000-00-00-schema.sql

@@ -675,27 +676,30 @@ CREATE TABLE `mri_protocol` (
 KEY `FK_mri_protocol_1` (`ScannerID`),
 CONSTRAINT `FK_mri_protocol_1` FOREIGN KEY (`ScannerID`) REFERENCES `mri_scanner` (`ID`),
 CONSTRAINT `FK_mri_protocol_2` FOREIGN KEY (`CenterID`) REFERENCES `psc` (`CenterID`),
+ CONSTRAINT `FK_mri_protocol_scan_type` FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),


seriously, we never had a foreign key for this? Interesting...

cmadjar · 2024-08-01T15:01:29Z

SQL/0000-00-00-schema.sql

@@ -968,7 +972,7 @@ CREATE TABLE `mri_violations_log` (
 `CandID` int(6) DEFAULT NULL,
 `Visit_label` varchar(255) DEFAULT NULL,
 `CheckID` int(11) DEFAULT NULL,
- `Scan_type` int(11) unsigned DEFAULT NULL,
+ `MriScanTypeID` int(11) unsigned DEFAULT NULL,


@nicolasbrossard @driusan I am curious what you think of the DEFAULT NULL for MriScanTypeID of table mri_violations_log. My understanding is that it can never be NULL based on the code since the violations log happen after scan identification. But before we remove the DEFAULT NULL from this table, I'd like your input. :)

Thank you!

cmadjar · 2024-08-01T15:02:27Z

SQL/0000-00-00-schema.sql

+ CONSTRAINT `FK_mri_violations_log_scan_type`
+ FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),


fascinating, another foreign key I was sure was already there. 🤣 Aaah, the things you take for granted...

cmadjar · 2024-08-01T15:05:21Z

SQL/0000-00-00-schema.sql

+ `MriProtocolGroupTargetID` INT(11) UNSIGNED NOT NULL AUTO_INCREMENT,
+ `MriProtocolGroupID` INT(4) UNSIGNED NOT NULL,
+ `ProjectID` INT(10) UNSIGNED DEFAULT NULL,
+ `CohortID` INT(10) UNSIGNED DEFAULT NULL,
+ `Visit_label` VARCHAR(255) DEFAULT NULL,
+ PRIMARY KEY (`MriProtocolGroupTargetID`),
+ CONSTRAINT `FK_mri_protocol_group_target_1` FOREIGN KEY (`MriProtocolGroupID`) REFERENCES `mri_protocol_group` (`MriProtocolGroupID`),
+ CONSTRAINT `FK_mri_protocol_group_target_2` FOREIGN KEY (`ProjectID`) REFERENCES `Project` (`ProjectID`),
+ CONSTRAINT `FK_mri_protocol_group_target_3` FOREIGN KEY (`CohortID`) REFERENCES `cohort` (`CohortID`),


That change should not show.

FYI - for LORIS code, one tab = 4 spaces so you might want to update your editor to use the same convention.

cmadjar · 2024-08-01T15:07:20Z

SQL/0000-00-00-schema.sql

+ CONSTRAINT `UK_mri_protocol_group_target`
+ UNIQUE (`ProjectID`, `CohortID`, `Visit_label`)


@nicolasbrossard is that new unique constraint assumption correct for that table? I don't remember if we said it would be something we would change.

@maximemulder the change with that new constraint is technically not related to the scan type renaming so I would tend to send this in a separate PR to keep things separate.

cmadjar · 2024-08-01T15:08:48Z

SQL/New_patches/2024-25-06-CleanMriScanType.sql

+ALTER TABLE `mri_protocol_group_target`
+ ADD CONSTRAINT `UK_mri_protocol_group_target`
+ UNIQUE (`ProjectID`, `CohortID`, `Visit_label`);


Same comment as in the default schema, just tagging this to not forget it at this place:

@nicolasbrossard is that new unique constraint assumption correct for that table? I don't remember if we said it would be something we would change.

@maximemulder the change with that new constraint is technically not related to the scan type renaming so I would tend to send this in a separate PR.

cmadjar · 2024-08-01T15:14:29Z

test/RBdata.sql

@@ -1486,7 +1486,7 @@ UNLOCK TABLES;
 SET SQL_MODE=@OLD_SQL_MODE;

 DELETE FROM `mri_scan_type`;
-INSERT INTO `mri_scan_type` (ID,Scan_type) VALUES
+INSERT INTO `mri_scan_type` (ID,MriScanTypeID) VALUES


Suggested change

INSERT INTO `mri_scan_type` (ID,MriScanTypeID) VALUES

INSERT INTO `mri_scan_type` (ID,Name) VALUES

I think the second field field should be Name, not MriScanTypeID

cmadjar · 2024-08-01T15:17:15Z

SQL/0000-00-00-schema.sql

@@ -530,8 +530,9 @@ SET SQL_MODE=@OLD_SQL_MODE;

 CREATE TABLE `mri_scan_type` (
 `ID` int(11) unsigned NOT NULL auto_increment,
- `Scan_type` text NOT NULL,
- PRIMARY KEY (`ID`)
+ `Name` VARCHAR(255) NOT NULL,


Suggested change

`Name` VARCHAR(255) NOT NULL,

`MriScanTypeName` VARCHAR(255) NOT NULL,

I would tend to name the fields in that table as MriScanTypeID and MriScanTypeName. The first one is so that we can use USING in the queries. The second one is because when looking at the code change, if you need to look for something in the code with the Name field, it is really not specific. However, if we use MriScanTypeName, then you could grep that easily in the code.

I personally do not like the TableField naming convention as I find it verbose and repetitive (I often use SELECT t.field FROM table AS t in my queries).

The USING and grep arguments do make sense. However, I feel like those are answers to self-inflicted problems that are due to :

How widespread queries are in our codebase (they should ideally be constrained to some database files).

How raw SQL is a lot less searchable than (good) ORMs.

Hhhhm, our current SQL guidelines do not say anything about prefixing fields (outside of the ID) with the table name, so I assume not doing it would be the standard convention. Anyway, I think this is an issue that is broader than this PR, and that it should be discussed at a LORIS meeting. I am fine with blocking until a decision is taken.

I do want to say that I understand both point of views though, so although I have a preference I am not entirely dismissing your version.

I hate to disagree with both of you (joking, I love it) but I only like the ID to have the full table name (because its used in linking and as a reference) I find the full table name redundant in other fields, if we are looking for a query we would look for the table itself in the join clause

I would tend to name the fields in that table as MriScanTypeID and MriScanTypeName. The first one is so that we can use USING in the queries. The second one is because when looking at the code change, if you need to look for something in the code with the Name field, it is really not specific. However, if we use MriScanTypeName, then you could grep that easily in the code.

After some reflexion I am ok with applying these suggestions. I'll do it if / when I have some time to work again on this PR.

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 #9304 instead.

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.

maximemulder mentioned this pull request Jun 26, 2024

Clean-up database scan type aces/Loris-MRI#1139

Open

maximemulder changed the title ~~[cleanup] Improve mri_scan_type database~~ [database] Clean-up mri_scan_type Jun 26, 2024

maximemulder marked this pull request as ready for review June 26, 2024 17:46

cmadjar requested review from cmadjar and nicolasbrossard July 4, 2024 14:35

kongtiaowang closed this Jul 15, 2024

kongtiaowang reopened this Jul 15, 2024

maximemulder assigned maximemulder and unassigned maximemulder Jul 16, 2024

maximemulder mentioned this pull request Jul 17, 2024

[database] Improve schema defaults #9309

Merged

maximemulder assigned cmadjar and nicolasbrossard Jul 25, 2024

cmadjar requested changes Aug 1, 2024

View reviewed changes

maximemulder force-pushed the improve-database-mri-scan-type branch 6 times, most recently from b0e1c2b to e1545af Compare September 23, 2024 16:14

maximemulder changed the title ~~[database] Clean-up mri_scan_type~~ [database] Clean up mri_scan_type Sep 23, 2024

maximemulder force-pushed the improve-database-mri-scan-type branch from e1545af to e5ffba7 Compare September 23, 2024 19:02

maximemulder marked this pull request as draft September 23, 2024 19:58

maximemulder assigned maximemulder and unassigned cmadjar and nicolasbrossard Sep 23, 2024

maximemulder mentioned this pull request Sep 23, 2024

[imaging_browser] Error 500 caused by PDO error #9349

Closed

maximemulder force-pushed the improve-database-mri-scan-type branch from 9dd02fa to e5ffba7 Compare September 23, 2024 20:20

maximemulder mentioned this pull request Sep 23, 2024

[mri_violations] Broken query results serialization #9350

Closed

maximemulder force-pushed the improve-database-mri-scan-type branch from e5ffba7 to 90c10ef Compare September 24, 2024 18:55

maximemulder force-pushed the improve-database-mri-scan-type branch from 90c10ef to 4dff326 Compare September 25, 2024 13:49

maximemuldermcgill and others added 2 commits September 26, 2024 12:30

add patch

ed9b675

address suggestions

52bea5f

maximemulder force-pushed the improve-database-mri-scan-type branch from 4dff326 to 52bea5f Compare September 26, 2024 16:30

maximemulder marked this pull request as ready for review September 26, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[database] Clean up mri_scan_type #9304

[database] Clean up mri_scan_type #9304

maximemulder commented Jun 26, 2024 •

edited

Loading

maximemulder commented Jun 26, 2024

maximemulder commented Jul 4, 2024

maximemulder commented Jul 16, 2024

cmadjar left a comment

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

cmadjar Aug 1, 2024

maximemulder Aug 1, 2024 •

edited

Loading

ridz1208 Aug 1, 2024

cmadjar Aug 1, 2024

maximemulder Aug 1, 2024 •

edited

Loading

ridz1208 Aug 1, 2024

maximemulder Aug 19, 2024 •

edited

Loading

		CONSTRAINT `FK_mri_violations_log_scan_type`
		FOREIGN KEY (`MriScanTypeID`) REFERENCES `mri_scan_type` (`ID`),

		CONSTRAINT `UK_mri_protocol_group_target`
		UNIQUE (`ProjectID`, `CohortID`, `Visit_label`)

	INSERT INTO `mri_scan_type` (ID,MriScanTypeID) VALUES
	INSERT INTO `mri_scan_type` (ID,Name) VALUES

	`Name` VARCHAR(255) NOT NULL,
	`MriScanTypeName` VARCHAR(255) NOT NULL,

[database] Clean up mri_scan_type #9304

Are you sure you want to change the base?

[database] Clean up mri_scan_type #9304

Conversation

maximemulder commented Jun 26, 2024 • edited Loading

Summary of changes

Testing instructions

Alternatives

Related issues

maximemulder commented Jun 26, 2024

maximemulder commented Jul 4, 2024

maximemulder commented Jul 16, 2024

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximemulder Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximemulder Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximemulder Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

maximemulder commented Jun 26, 2024 •

edited

Loading

maximemulder Aug 1, 2024 •

edited

Loading

maximemulder Aug 1, 2024 •

edited

Loading

maximemulder Aug 19, 2024 •

edited

Loading