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

MySQL Parser understands VIEWs with a field list. #87

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

MySQL Parser understands VIEWs with a field list. #87

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
707abe2
MySQL Parser understands VIEWs with a field list.
adherzog Apr 2, 2017
3d090a5
Include 'view' keyword in MySQL Parser generated SQL.
adherzog Apr 10, 2017
8bdf3bf
Improve MySQL view parsing of field list, w/tests.
adherzog Apr 10, 2017
9953926
Simplify setting of column aliases from field list.
adherzog Apr 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/SQL/Translator/Parser/MySQL.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,13 @@ create : CREATE or_replace(?) create_view_option(s?) /view/i NAME parens_field_l
my $view_name = $item{'NAME'};
my $select_sql = $item{'view_select_statement'};
my $options = $item{'create_view_option(s?)'};
my $field_list = $item{'parens_field_list(?)'};

# Map fields as aliases on the select columns
my @fields = ( $field_list->[0] ) ? @{ $field_list->[0] } : ();
map { $select_sql->{'columns'}->[$_]->{'alias'} = $fields[$_] }
( 0 .. $#fields )
if (@fields);
Copy link
Member

Choose a reason for hiding this comment

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

First of all, never use map in void context for side effects, use for instead.
Secondly, this all could be simplified to:

my @fields = @{$item{'parens_field_list(?)'} || []};
$select_sql->{columns}->[$_]->{alias} = $fields[$_]
    for 0..$#fields;

Copy link
Member

@ilmari ilmari Apr 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

More importantly, this is unnecessary if you instead store $item{'parens_field_list(?)'} in $views{$view_name}{fields} and use that in preference to the select aliases in the ->add_view call.

Copy link
Author

Choose a reason for hiding this comment

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

If we store the fields in the %views hash instead of setting the column aliases, then the generated SQL won't include the aliases.

I've used your simplified code (though I'm using $item{'parens_field_list(?)'}[0] instead of $item{'parens_field_list(?)'}, since it seems to be an arrayref within an arrayref) and left the rest the same.


my $sql = join(q{ },
grep { defined and length }
Expand Down
56 changes: 51 additions & 5 deletions t/02mysql-parser.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,17 +502,63 @@ BEGIN {
is( join(',', $t2c2->reference_fields), 'id', 'To field "id"' );
}

# Tests for CREATE VIEW statements that contain a column list
# after the view name
# Tests CREATE VIEW statements containing field lists and/or aliases
{
my $tr = SQL::Translator->new();
my $data = parse($tr,
my $tr = SQL::Translator->new();
my $data = parse(
$tr,
q[
CREATE
VIEW view_foo (id, name) AS
VIEW view_foo (a, b) AS
SELECT id, name FROM thing;

CREATE
VIEW view_bar AS
SELECT id AS c, name AS d FROM thing;

CREATE
VIEW view_baz (e, f) AS
SELECT id AS g, name AS h FROM thing;
]
) or die $tr->error;
Copy link
Member

Choose a reason for hiding this comment

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

Please also test that the view is in fact correctly parsed and that the column names after the view name take precedence over the ones in the SELECT list.

Copy link
Author

Choose a reason for hiding this comment

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

I've added better tests - it now checks VIEW statements that include 1) a field list, 2) aliases in the SELECT statement, and 3) both a field list and aliases. (When both are specified, I believe the field list should override the column aliases.)


my $schema = $tr->schema;
my @views = $schema->get_views;
is( scalar @views, 3, 'Right number of views (3)' );
my ( $view1, $view2, $view3 ) = @views;

is( $view1->name, 'view_foo', 'Found "view_foo" view' );
is( join( ',', $view1->fields ),
join( ',', qw[ a b ] ),
'View 1 has correct fields'
);
like(
$view1->sql,
qr/create view view_foo as select id as a, name as b\s+from\s+thing/i,
'View 1 has correct sql'
);

is( $view2->name, 'view_bar', 'Found "view_bar" view' );
is( join( ',', $view2->fields ),
join( ',', qw[ c d ] ),
'View 2 has correct fields'
);
like(
$view2->sql,
qr/create view view_bar as select id as c, name as d\s+from\s+thing/i,
'View 2 has correct sql'
);

is( $view3->name, 'view_baz', 'Found "view_baz" view' );
is( join( ',', $view3->fields ),
join( ',', qw[ e f ] ),
'View 3 has correct fields'
);
like(
$view3->sql,
qr/create view view_baz as select id as e, name as f\s+from\s+thing/i,
'View 3 has correct sql'
);
}

# cch Tests for:
Expand Down