Merged fix-937234-pqa-wrong-res.001

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; }

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; }

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;