|
Description
Bernardo Gonzalez Kriegel
2020-03-16 21:33:05 UTC
Created attachment 100831 [details] [review] Bug 24883: Command line utility to load yaml files This patch adds a command line utility to load yaml files. To test: 1) Apply the patch 2) Try loading a file, e.g. sample notices a) mysql -e "delete from letter" b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml 3) Try loading a file with a syntax error, edit previous file adding ':' at the end of a field without quotes then load again Moving out of NSO queue, I am trying something. Created attachment 101009 [details] [review] Bug 24883: Command line utility to load yaml files This patch adds a command line utility to load yaml files. To test: 1) Apply the patch 2) Try loading a file, e.g. sample notices a) mysql -e "delete from letter" b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml 3) Try loading a file with a syntax error, edit previous file adding ':' at the end of a field without quotes then load again Created attachment 101010 [details] [review] Bug 24883: Centralize code to load YAML installer files to a sub Created attachment 101011 [details] [review] Bug 24883: Move to a flat array with all values to an array of arrayref For the following SQL query: INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina'); We move from: [ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ] to: [ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ] Which make more sense to split, build and construct the queries Created attachment 101012 [details] [review] Bug 24883: Add a new yml2sql sub to C4::Installer It will allow us to generate sql code from the yml files Created attachment 101013 [details] [review] Bug 24883: Move new sub from load_yaml.pl Use the sub from C4::Installer to avoid dup of code. Also add an option to convert the yml file to SQL queries Note: We are going to modify the script and so will do more stuffs. We may want to rename it, maybe installer_utilities.pl, misc/installer/yaml_utility.pl, any suggestions? Bernardo, what about those follow-ups? Do they make sense of I went to far? More work needed: - Use Pod2Usage - Write tests for the new subroutines But I'd like you to agree first before I continue. (In reply to Jonathan Druart from comment #8) > Bernardo, what about those follow-ups? Do they make sense of I went to far? > > More work needed: > - Use Pod2Usage > - Write tests for the new subroutines > > But I'd like you to agree first before I continue. Hi Jonathan, trying this and Bug 24904. I'm ok in general, I was also testing some changes of myself. One change I tried (not related with this bug) was disabling AutoCommit after the insertion loop and enabling it afterwards. With that I got almost the same time as with sql files. Have you explored that option? I tried to execute all the queries in a transaction (using txn_do), but no gain observed. (In reply to Jonathan Druart from comment #10) > I tried to execute all the queries in a transaction (using txn_do), but no > gain observed. With this diff in my original script diff --git a/misc/load_yaml.pl b/misc/load_yaml.pl index e670f56660..8eed5ab933 100755 --- a/misc/load_yaml.pl +++ b/misc/load_yaml.pl @@ -63,7 +63,7 @@ eval { if ($@){ die "Something went wrong loading file $file ($@)"; } - +$dbh->{'AutoCommit'} = 0; for my $table ( @{ $yaml->{'tables'} } ) { my $table_name = ( keys %$table )[0]; # table name my @rows = @{ $table->{$table_name}->{rows} }; # @@ -86,3 +86,4 @@ for my $table ( @{ $yaml->{'tables'} } ) { for my $statement ( @{ $yaml->{'sql_statements'} } ) { # extra SQL statements $dbh->do($statement); } +$dbh->{'AutoCommit'} = 1; I see changes, form ~9s to ~1.3 for marc21_framework_DEFAULT.yml for example. Of course I'm with you on putting the code in one place and other changes, just want to share my results. We got problems with AutoCommit=0 and transactions, and decided to use DBIC transactions only. I am surprised however do not have the same results between both. (In reply to Jonathan Druart from comment #12) > We got problems with AutoCommit=0 and transactions, and decided to use DBIC > transactions only. I am surprised however do not have the same results > between both. Understood. Works well, nice to load multiple file in one go, a few qa errors. Now, the conversion part. The problem here is that all values are printed as strings, producing INSERT INTO marc_tag_structure ( `authorised_value`,`frameworkcode`,`liblibrarian`,`libopac`,`mandatory`,`repeatable`,`tagfield` ) VALUES ('', '', 'LEADER', 'LEADER', '1', '0', '000'), instead of ('', '', 'LEADER', 'LEADER', 1, 0, '000'), A file like this will fail to load with strict mode. This could be fixed if we declare on each table which fields are numeric, and take that into account when doing the conversion. What do you think? I moved my patches to a separate bug report, see bug 24912. Created attachment 101292 [details] [review] Bug 24883: Centralize code to load YAML installer files to a sub Created attachment 101293 [details] [review] Bug 24883: Move to a flat array with all values to an array of arrayref For the following SQL query: INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina'); We move from: [ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ] to: [ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ] Which make more sense to split, build and construct the queries Created attachment 101294 [details] [review] Bug 24883: Move new sub from load_yaml.pl Use the sub from C4::Installer to avoid dup of code. Note: We are going to modify the script and so will do more stuffs. We may want to rename it, maybe installer_utilities.pl, misc/installer/yaml_utility.pl, any suggestions? Created attachment 101332 [details] [review] Bug 24883: Add POD entry and remove unused variable https://bugs.koha-community.org/show_bug.cgi?id=24833 Created attachment 101333 [details] [review] Bug 24883: Add POD entry and remove unused variable This is looking good so far.. and in testing seems to all work for me. It looks like you are perhaps still working on it together.. or is it ready for a signoff? (In reply to Martin Renvoize from comment #20) > This is looking good so far.. and in testing seems to all work for me. > > It looks like you are perhaps still working on it together.. or is it ready > for a signoff? I'm using it, works Ok :) My last patch was only to remove some complains from qa tools, don't know if Jonathan agrees with the pod text, but can be amended in the future. It's waiting a signoff Created attachment 101901 [details] [review] Bug 24883: Command line utility to load yaml files This patch adds a command line utility to load yaml files. To test: 1) Apply the patch 2) Try loading a file, e.g. sample notices a) mysql -e "delete from letter" b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml 3) Try loading a file with a syntax error, edit previous file adding ':' at the end of a field without quotes then load again Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 101902 [details] [review] Bug 24883: Centralize code to load YAML installer files to a sub Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 101903 [details] [review] Bug 24883: Move to a flat array with all values to an array of arrayref For the following SQL query: INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina'); We move from: [ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ] to: [ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ] Which make more sense to split, build and construct the queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 101904 [details] [review] Bug 24883: Move new sub from load_yaml.pl Use the sub from C4::Installer to avoid dup of code. Note: We are going to modify the script and so will do more stuffs. We may want to rename it, maybe installer_utilities.pl, misc/installer/yaml_utility.pl, any suggestions? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 101905 [details] [review] Bug 24883: Add POD entry and remove unused variable Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works well, passes QA scripts.. Signing off.. QAing Created attachment 101950 [details] [review] Bug 24883: Command line utility to load yaml files This patch adds a command line utility to load yaml files. To test: 1) Apply the patch 2) Try loading a file, e.g. sample notices a) mysql -e "delete from letter" b) misc/load_yaml.pl -f installer/data/mysql/en/mandatory/sample_notices.yml 3) Try loading a file with a syntax error, edit previous file adding ':' at the end of a field without quotes then load again Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 101951 [details] [review] Bug 24883: Centralize code to load YAML installer files to a sub Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 101952 [details] [review] Bug 24883: Move to a flat array with all values to an array of arrayref For the following SQL query: INSERT INTO cities(city_name, city_country) VALUES ('Madrid', 'Spain'), ('Buenos Aires', 'Argentina'); We move from: [ 'Madrid', 'Spain', 'Buenos Aires', 'Argentina' ] to: [ [ 'Madrid', 'Spain' ], [ 'Buenos Aires', 'Argentina' ] ] Which make more sense to split, build and construct the queries Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 101953 [details] [review] Bug 24883: Move new sub from load_yaml.pl Use the sub from C4::Installer to avoid dup of code. Note: We are going to modify the script and so will do more stuffs. We may want to rename it, maybe installer_utilities.pl, misc/installer/yaml_utility.pl, any suggestions? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 101954 [details] [review] Bug 24883: Add POD entry and remove unused variable Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Great! Looks good to me. No test for process_yml_table, but no blocker for me. We could add some more helpful messages in the future? E.g. If I am forgetting a : after tables, file is silently ignored. Could probably think of more examples. Nice work everyone! Pushed to master for 20.05 enhancement not backported to 19.11.x |