diff --git a/bin/pt-query-advisor b/bin/pt-query-advisor index ff1282e8..1033dd6b 100755 --- a/bin/pt-query-advisor +++ b/bin/pt-query-advisor @@ -3816,9 +3816,14 @@ sub get_rules { grep { $_->{column} } @$groupby; return unless scalar %groupby_col; - my $cols = $event->{query_struct}->{columns}; + my $cols = [ + grep { _looks_like_column($_->{col}) } + grep { ! exists $_->{func} } + @{$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; }, @@ -4052,6 +4057,13 @@ sub determine_table_for_column { return; } +sub _looks_like_column { + my $col = shift; + return if $col eq '*' || uc($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..3d4cddc1 100644 --- a/lib/QueryAdvisorRules.pm +++ b/lib/QueryAdvisorRules.pm @@ -341,11 +341,17 @@ 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}) } + grep { ! exists $_->{func} } + @{$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 +664,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 '*' || uc($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..3a8f6ca5 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,30 @@ 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, null, 1, COUNT(*), @@time_zone, foo as field2 from t1 group by field2}, + advice => [qw(CLA.001)], + }, + { + name => 'Bug 996069: wrong RES.001', + query => q{SELECT cola, MAX(colb) FROM table WHERE cola = 123 GROUP BY cola}, + advice => [qw()], + }, + { + name => 'Bug 933465: false positive on RES.001', + query => q{select name, population, count(*) from world.Country group by name, population}, + advice => [qw(CLA.001)], + }, + { + name => 'Bug 933465: false positive on RES.001', + query => q{SELECT ID_organization,ToNalog,ID_nachtype, COUNT(*) as Cnt } + . q{FROM buh_provodka_zp } + . q{GROUP BY ID_organization,ToNalog,ID_nachtype } + . q{HAVING Cnt>1 } + . q{ORDER BY Cnt DESC}, + advice => [qw(CLA.001)], + }, ); # Run the test cases. @@ -524,4 +548,6 @@ like( qr/Complete test coverage/, '_d() works' ); + +done_testing; exit;