diff --git a/bin/pt-query-advisor b/bin/pt-query-advisor index 5c7b5730..ebae0152 100755 --- a/bin/pt-query-advisor +++ b/bin/pt-query-advisor @@ -3813,9 +3813,13 @@ sub get_rules { grep { $_->{column} } @$groupby; return unless scalar %groupby_col; - my $cols = $event->{query_struct}->{columns}; + my $cols = [ + grep { _looks_like_column($_->{col}) } + @{$event->{query_struct}->{columns}} + ]; foreach my $col ( @$cols ) { - return 0 unless $groupby_col{ $col->{col} }; + return 0 unless $groupby_col{ $col->{col} } + || ($col->{alias} && $groupby_col{ $col->{alias} }); } return; }, @@ -4049,6 +4053,13 @@ sub determine_table_for_column { return; } +sub _looks_like_column { + my $col = shift; + return if $col eq '*' || $col eq 'NULL'; + return if $col =~ /\A(?:\b[0-9]+\b|[^(]+\(|\@{1,2}.+)/; + return $col; +} + sub _d { my ($package, undef, $line) = caller 0; @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; } diff --git a/lib/QueryAdvisorRules.pm b/lib/QueryAdvisorRules.pm index 942ff550..5753a4d8 100644 --- a/lib/QueryAdvisorRules.pm +++ b/lib/QueryAdvisorRules.pm @@ -341,11 +341,16 @@ sub get_rules { grep { $_->{column} } @$groupby; return unless scalar %groupby_col; - my $cols = $event->{query_struct}->{columns}; + # Skip non-columns -- NULL, digits, functions, variables + my $cols = [ + grep { _looks_like_column($_->{col}) } + @{$event->{query_struct}->{columns}} + ]; # All SELECT cols must be in GROUP BY cols clause. # E.g. select a, b, c from tbl group by a; -- non-deterministic foreach my $col ( @$cols ) { - return 0 unless $groupby_col{ $col->{col} }; + return 0 unless $groupby_col{ $col->{col} } + || ($col->{alias} && $groupby_col{ $col->{alias} }); } return; }, @@ -658,6 +663,14 @@ sub determine_table_for_column { return; } +sub _looks_like_column { + my $col = shift; + # NULL, numbers, variables and functions are definitely not columns + return if $col eq '*' || $col eq 'NULL'; + return if $col =~ /\A(?:\b[0-9]+\b|[^(]+\(|\@{1,2}.+)/; + return $col; +} + sub _d { my ($package, undef, $line) = caller 0; @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; } diff --git a/t/lib/QueryAdvisorRules.t b/t/lib/QueryAdvisorRules.t index 4f1bc98b..2e49855d 100644 --- a/t/lib/QueryAdvisorRules.t +++ b/t/lib/QueryAdvisorRules.t @@ -9,7 +9,7 @@ BEGIN { use strict; use warnings FATAL => 'all'; use English qw(-no_match_vars); -use Test::More tests => 87; +use Test::More; use PerconaTest; use PodParser; @@ -466,6 +466,11 @@ my @cases = ( query => "select col1, col2 from tbl where i=1 order by col1, col2 desc", advice => [qw(CLA.007)], }, + { + name => 'Bug 937234: wrong RES.001', + query => q{select NULL, 1, COUNT(*), @@time_zone, foo as field2 from t1 group by field2}, + advice => [qw(CLA.001)], + }, ); # Run the test cases. @@ -524,4 +529,6 @@ like( qr/Complete test coverage/, '_d() works' ); + +done_testing; exit;