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 3 commits
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
10 changes: 9 additions & 1 deletion lib/SQL/Translator/Parser/MySQL.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,19 +326,27 @@ create : CREATE PROCEDURE NAME not_delimiter "$delimiter"
PROCEDURE : /procedure/i
| /function/i

create : CREATE or_replace(?) create_view_option(s?) /view/i NAME /as/i view_select_statement "$delimiter"
create : CREATE or_replace(?) create_view_option(s?) /view/i NAME parens_field_list(?) /as/i view_select_statement "$delimiter"
Copy link
Member

Choose a reason for hiding this comment

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

An explicit field list overrides the names derived from the SELECT column list, so the field list needs to be saved here and stored in the fields attribute of the SQL::Translator::Schema::View object.

Copy link
Author

Choose a reason for hiding this comment

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

It looks like the fields attribute of the view is generated based on the column aliases, so I'm setting the column aliases rather than explicitly setting the fields attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I misread your statement, ignore the previous comment. It'd be easier to just store the fields in the %views hash and use that if present instead of the alias list in the ->add_view call in sub parse { … }.

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. So I'm continuing to set the aliases, and both the generated SQL and the fields attribute are returning the correct results. (At least, according to the tests I added.)

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.

@ilmari - Please review again when you have a chance.

{
@table_comments = ();
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 }
map { ref $_ eq 'ARRAY' ? @$_ : $_ }
$item{'CREATE'},
$item{'or_replace(?)'},
$options,
'view',
$view_name,
'as select',
join(', ',
Expand Down
59 changes: 59 additions & 0 deletions t/02mysql-parser.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,65 @@ BEGIN {
is( join(',', $t2c2->reference_fields), 'id', 'To field "id"' );
}

# Tests CREATE VIEW statements containing field lists and/or aliases
{
my $tr = SQL::Translator->new();
my $data = parse(
$tr,
q[
CREATE
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:
# comments like: /*!40101 SET SQL_MODE=@OLD_SQL_MODE */;
# char fields with character set and collate qualifiers
Expand Down