Merge lp:~percona-toolkit-dev/percona-toolkit/fix-937234-pqa-wrong-res.001 into lp:percona-toolkit/2.1

[Brian Fraser]

[lp:~percona-toolkit-dev/percona-toolkit/fix-937234-pqa-wrong-res.001]

444. By Brian Fraser on 2012-11-16

Removed a useless part of the regex that looked for something function-like, and made the NULL check case-insensitive

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-query-advisor'
--- bin/pt-query-advisor	2012-11-13 15:22:01 +0000
+++ bin/pt-query-advisor	2012-11-16 15:07:22 +0000
@@ -3816,9 +3816,14 @@
                            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 @@
    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; }

=== modified file 'lib/QueryAdvisorRules.pm'
--- lib/QueryAdvisorRules.pm	2012-04-03 17:03:17 +0000
+++ lib/QueryAdvisorRules.pm	2012-11-16 15:07:22 +0000
@@ -341,11 +341,17 @@
                            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 @@
    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; }

=== modified file 't/lib/QueryAdvisorRules.t'
--- t/lib/QueryAdvisorRules.t	2012-03-06 13:56:08 +0000
+++ t/lib/QueryAdvisorRules.t	2012-11-16 15:07:22 +0000
@@ -9,7 +9,7 @@
 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 @@
       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 @@
    qr/Complete test coverage/,
    '_d() works'
 );
+
+done_testing;
 exit;