Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[tools] Script to remove Zero dates for mysql 5.7 #2444

Merged
merged 10 commits into from
Dec 9, 2016
Merged

[tools] Script to remove Zero dates for mysql 5.7 #2444

merged 10 commits into from
Dec 9, 2016

Conversation

ridz1208
Copy link
Collaborator

No description provided.

@ridz1208 ridz1208 added Cleanup PR or issue introducing/requiring at least one clean-up operation Request Code Review labels Nov 30, 2016
@ridz1208 ridz1208 added this to the 17.0 milestone Nov 30, 2016
driusan
driusan requested changes Nov 30, 2016
View reviewed changes
tools/DB_date_zeros_removal.php
" WHERE CAST(".$field['COLUMN_NAME']." AS CHAR(20))='0000-00-00 00:00:00';\n";
}
}
$output .="SET FOREIGN_KEY_CHECKS=1; \n";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just do the update/alter in the other order? That way you wouldn't need to disable the foreign key checks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 AND SET FOREIGN_KEY_CHECKS=@OLD_FOREIGN_KEY_CHECKS at the end.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driusan I think it is because some field are not nullable but should be after the ALTER statement. But the only ones that should be nullable are those that have a default value of 0000-00-00 00:00:00 and not CURRENT_TIMESTAMP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another issue popped up where you can not alter if there are 0000-00-00 dates and you cannot update to NULL if column NOT NULL so catch22

change of logic needed

tools/DB_date_zeros_removal.php
{
if ($field['COLUMN_DEFAULT']=='0000-00-00') {
echo "The script will modify the date schema for TABLE: `".$field['TABLE_NAME']."` FIELD: `".$field['COLUMN_NAME']."` to default to NULL\n";
$output .= "ALTER TABLE `".$field['TABLE_NAME']."` MODIFY `".$field['COLUMN_NAME']."` DATE DEFAULT NULL;\n";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if the column name has a precision set? You can just add COLUMN_TYPE to your select statement and use that value instead of a hardcoded "DATE" to ensure you're setting it back to the exact same data type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same for datetime below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gluneau any arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driusan not sure what "precision" you are talking about. Those statements will only be generated for fields that currently try to default to "0000-00-00". The statements only change the default value of that field without affecting other characteristics of it.

xlecours
xlecours previously requested changes Nov 30, 2016
View reviewed changes
tools/DB_date_zeros_removal.php
*
* @category Main
* @package Loris
* @author Various <example@example.com>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@author Loris team info-loris.mni@mcgill.ca maybe?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't Rida the author? We should maybe standardize on something for the author line across all of standard LORIS, but right now I think everything just has the original author.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rida/greg collaboration

tools/DB_date_zeros_removal.php
$output .= "UPDATE ".$database['database'].".".$field['TABLE_NAME'].
" SET ".$field['COLUMN_NAME']."=NULL".
" WHERE CAST(".$field['COLUMN_NAME']." AS CHAR(20))='0000-00-00';\n";
} else if ($field['DATA_TYPE'] == 'datetime' || $field['DATA_TYPE'] == 'timestamp') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about timestamp column with CURRENT_TIMESTAMP as a default value?
Could you add a COLUMN_DEFAULT <> 'CURRENT_TIMESTAMP' in the select statement?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well this will only change if the date is 0000-00-00 whic is closest to null than current_timestamp no ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xlecours If column default is current timestamp, does it mean that it is impossible to have a 0000-00-00 value in that column ??

(my guess is no since even if the default is current timestamp, someone could have tried entering at somepoint an invalid date causing a 0000-00-00 date value to enter the DB)

tools/DB_date_zeros_removal.php
" WHERE CAST(".$field['COLUMN_NAME']." AS CHAR(20))='0000-00-00 00:00:00';\n";
}
}
$output .="SET FOREIGN_KEY_CHECKS=1; \n";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to use SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 AND SET FOREIGN_KEY_CHECKS=@OLD_FOREIGN_KEY_CHECKS at the end.

@ridz1208 ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Dec 1, 2016
@ridz1208
Copy link
Collaborator Author

ridz1208 commented Dec 4, 2016 •
edited
Loading

  • Only change schema to allow null when default is 0000-00-00 (without impacting other properties)
  • Restore foreign key to original value
  • Change order of operations (set alter tables first)
  • Handle Catch22 situation
  • Handle usecase: field TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP -> containing 0000-00-00 00:00:00

details:
ALTER TABLE videos MODIFY Date_uploaded TIMESTAMP DEFAULT NULL;
ERROR 1067 (42000): Invalid default value for 'Date_uploaded'

@ridz1208 ridz1208 removed the State: Needs work PR awaiting additional work by the author to proceed label Dec 9, 2016
@driusan driusan dismissed xlecours’s stale review December 9, 2016 16:31

We looked at this at the PR meeting

@christinerogers christinerogers added the Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release label Dec 9, 2016
@MounaSafiHarab MounaSafiHarab merged commit 5fce52d into aces:17.0-dev Dec 9, 2016
@christinerogers christinerogers added the Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects label Dec 9, 2016
MounaSafiHarab pushed a commit to MounaSafiHarab/Loris that referenced this pull request Jan 3, 2017
@ridz1208 @MounaSafiHarab
[tools] Script to remove Zero dates for mysql 5.7 (aces#2444)
4b67126 
* Added script for 00:00:00 dates

* bad path

* Changed author

* reset foreign key check to original value

* Reorganize output

* More details in query

* Replace type by original type

* fixed catch 22

* typo

* handle non-nullable fields
ridz1208 added a commit to ridz1208/Loris that referenced this pull request Jan 23, 2017
@ridz1208
[tools] Script to remove Zero dates for mysql 5.7 (aces#2444)
ebc2cd4 
* Added script for 00:00:00 dates

* bad path

* Changed author

* reset foreign key check to original value

* Reorganize output

* More details in query

* Replace type by original type

* fixed catch 22

* typo

* handle non-nullable fields
MounaSafiHarab pushed a commit to MounaSafiHarab/Loris that referenced this pull request Jan 26, 2017
@ridz1208 @MounaSafiHarab
[tools] Script to remove Zero dates for mysql 5.7 (aces#2444)
41c5a82 
* Added script for 00:00:00 dates

* bad path

* Changed author

* reset foreign key check to original value

* Reorganize output

* More details in query

* Replace type by original type

* fixed catch 22

* typo

* handle non-nullable fields
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xlecours xlecours xlecours left review comments

@driusan driusan driusan approved these changes

Assignees

@driusan driusan

@gluneau gluneau

@xlecours xlecours

Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects Release: Document at release PR whose changes need to be documented in the wiki (or other documentation) at release
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.
6 participants
@ridz1208 @driusan @xlecours @christinerogers @gluneau @MounaSafiHarab