-
Notifications
You must be signed in to change notification settings - Fork 90
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.
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
MySQL Parser understands VIEWs with a field list. #87
base: master
Are you sure you want to change the base?
MySQL Parser understands VIEWs with a field list. #87
Conversation
Updates the MySQL parser to properly parse CREATE VIEW statements that contain a field list after the view name. This allows the parser to understand statements created by the MySQL producer. Prior to this change, this statement would be parsed properly: CREATE VIEW view_foo AS SELECT id, name FROM thing; But this one would not: CREATE VIEW view_foo (id, name) AS SELECT id, name FROM thing;
@@ -326,7 +326,7 @@ 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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 { … }
.
There was a problem hiding this comment.
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.
VIEW view_foo (id, name) AS | ||
SELECT id, name FROM thing; | ||
] | ||
) or die $tr->error; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
@ilmari Thanks for the review/feedback. I've updated the PR as you requested. Let me know if you seen any other errors or omissions. |
lib/SQL/Translator/Parser/MySQL.pm
Outdated
my @fields = ( $field_list->[0] ) ? @{ $field_list->[0] } : (); | ||
map { $select_sql->{'columns'}->[$_]->{'alias'} = $fields[$_] } | ||
( 0 .. $#fields ) | ||
if (@fields); |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Updates the MySQL parser to properly parse CREATE VIEW statements that
contain a field list after the view name. This allows the parser
to understand statements created by the MySQL producer.
Prior to this change, this statement would be parsed properly:
But this one would not:
(The latter statement is the same one added to the tests in this PR. It's taken directly from the mysql producer tests.)