Skip Menu |

This queue is for tickets about the SQL-Translator CPAN distribution.

Report information
The Basics
Id: 82818
Status: open
Priority: 0/
Queue: SQL-Translator

People
Owner: Nobody in particular
Requestors: wftk [...] vivtek.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Parser matches INSERT, but processors don't handle it
This behavior dates back probably to 2005 or 2006. The parser began matching INSERT but INSERT support wasn't added to the processors, and that breaks the tests for Class::DBI::DATA::Schema, which expects anything not handled by SQLFairy to return a blank string (instead of wrapping that blank string in begin/commit and returning that). It would be nice if it were at least possible to disable this with a flag, but if it's been six years, maybe INSERT should just not appear in the grammar at all?
On Sat Jan 19 06:40:46 2013, Michael wrote: Show quoted text
> This behavior dates back probably to 2005 or 2006. The parser began > matching INSERT but INSERT support wasn't added to the processors, and > that breaks the tests for Class::DBI::DATA::Schema, which expects
anything Show quoted text
> not handled by SQLFairy to return a blank string (instead of wrapping
that Show quoted text
> blank string in begin/commit and returning that). > > It would be nice if it were at least possible to disable this with a
flag, Show quoted text
> but if it's been six years, maybe INSERT should just not appear in the > grammar at all?
The mandate of SQL::Translator has always been DDL only. Insert appears in the grammars to correctly throw away any INSERT statements that could be found in a RDBMS schema dump. Without a mention in the grammar the entire loading process would choke. By mentioning and ignoring it the DDL is correctly loaded from the dump. So all in all this behavior is intended. Can you elaborate how it breaks the test in question?
On Wed Jan 23 05:55:44 2013, RIBASUSHI wrote: Show quoted text
> On Sat Jan 19 06:40:46 2013, Michael wrote:
> > This behavior dates back probably to 2005 or 2006. The parser began > > matching INSERT but INSERT support wasn't added to the processors,
and Show quoted text
> > that breaks the tests for Class::DBI::DATA::Schema, which expects
> anything
> > not handled by SQLFairy to return a blank string (instead of
wrapping Show quoted text
> that
> > blank string in begin/commit and returning that). > > > > It would be nice if it were at least possible to disable this with a
> flag,
> > but if it's been six years, maybe INSERT should just not appear in
the Show quoted text
> > grammar at all?
> > The mandate of SQL::Translator has always been DDL only. Insert
appears Show quoted text
> in the grammars to correctly throw away any INSERT statements that
could Show quoted text
> be found in a RDBMS schema dump. Without a mention in the grammar the > entire loading process would choke. By mentioning and ignoring it the > DDL is correctly loaded from the dump. > > So all in all this behavior is intended. Can you elaborate how it
breaks Show quoted text
> the test in question?
Ah, I see - that makes a lot of sense, sure. The only problem on my end is that Class::DBI::DATA::Schema was effectively using it to process the entire dump. Basically, the strategy there is to pass each SQL command in the dump through Translator, and if something's returned, to use that - and otherwise to use the original command. Really elegant, actually. The problem is that, as I say, about 2005 or 2006 it stopped working, because when translating an INSERT command on its own from MySQL to SQLite, the result was no longer blank. It occurred to me just now, reading your response, that Translator may well have parsed this before and returned a blank response - and that the change may have been that the SQLite processor started wrapping the TRANSACTION/COMMIT statement pair around the response even when it was blank. This wouldn't normally happen, of course. The answer is probably to modify Class::DBI::DATA::Schema to use Translator as intended - that is, for it to recognize that INSERT statements should not be passed through translation in the first place. It would be nice to make INSERT interception a configurable option, but you've got to draw the line somewhere, and your explanation makes perfect sense (although the behavior certainly didn't make sense when I initially saw it). I guess the other alternative would be to modify the SQLite processor so that only issues a transaction/commit pair if there is something to commit.
On Wed Jan 23 08:18:03 2013, Michael wrote: Show quoted text
> the SQLite processor started wrapping the TRANSACTION/COMMIT statement > pair around the response even when it was blank. > ... > I guess the other alternative would be to modify the SQLite processor so > that only issues a transaction/commit pair if there is something to > commit.
I much prefer this last option. Care to take a stab at a patch?
On Wed Jan 23 08:32:45 2013, RIBASUSHI wrote: Show quoted text
> I much prefer this last option. Care to take a stab at a patch?
In principle, yes (actually quite interesting) - in reality, subject to my getting some time in the next week or so. I've made a to-do point. No, seriously, don't laugh, at least this way I won't forget *forever*.
On Wed Jan 23 08:32:45 2013, RIBASUSHI wrote: Show quoted text
> I much prefer this last option. Care to take a stab at a patch?
Done! The first code I've ever written on an airplane.
Subject: patch.txt
*** SQL-Translator-0.11016/lib/SQL/Translator/Producer/SQLite.pm 2012-09-23 11:17:34.000000000 -0400 --- SQL-Translator-0.11016-mod/lib/SQL/Translator/Producer/SQLite.pm 2013-01-27 11:27:04.682708900 -0400 *************** *** 70,81 **** my @create = (); ! push @create, "BEGIN TRANSACTION" unless $no_txn; for my $table ( $schema->get_tables ) { push @create, create_table($table, { no_comments => $no_comments, sqlite_version => $sqlite_version, add_drop_table => $add_drop_table,}); } for my $view ( $schema->get_views ) { --- 70,82 ---- my @create = (); ! my $stmt_input = 0; for my $table ( $schema->get_tables ) { push @create, create_table($table, { no_comments => $no_comments, sqlite_version => $sqlite_version, add_drop_table => $add_drop_table,}); + $stmt_input = 1; } for my $view ( $schema->get_views ) { *************** *** 83,88 **** --- 84,90 ---- add_drop_view => $add_drop_table, no_comments => $no_comments, }); + $stmt_input = 1; } for my $trigger ( $schema->get_triggers ) { *************** *** 90,98 **** add_drop_trigger => $add_drop_table, no_comments => $no_comments, }); } ! push @create, "COMMIT" unless $no_txn; if (wantarray) { return ($head||(), @create); --- 92,104 ---- add_drop_trigger => $add_drop_table, no_comments => $no_comments, }); + $stmt_input = 1; } ! if ($stmt_input and not $no_txn) { ! unshift @create, "BEGIN TRANSACTION"; ! push @create, "COMMIT"; ! } if (wantarray) { return ($head||(), @create); *************** *** 100,106 **** return join ('', $head||(), join(";\n\n", @create ), ! ";\n", ); } } --- 106,112 ---- return join ('', $head||(), join(";\n\n", @create ), ! $stmt_input ? ";\n" : '', ); } }
On Mon Jan 28 08:17:45 2013, Michael wrote: Show quoted text
> On Wed Jan 23 08:32:45 2013, RIBASUSHI wrote: >
> > I much prefer this last option. Care to take a stab at a patch?
> > Done! The first code I've ever written on an airplane.
Oh, sorry - I should have noted that this passes all tests for SQL::Translator. (And also makes Class::DBI::DATA::Schema pass *its* tests - except for one it wasn't getting to because of this, and that's not your problem, ha.)
By the way, is there any current plan or schedule for a version update that might include this patch? I've got another module that depends on this change and I'd like to be able to mark it in the dependencies with a version number.