From 4b5c8969f0feb486ce336c4332fd6ef8a6067033 Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Thu, 1 Nov 2012 05:33:35 -0300 Subject: [PATCH 1/4] Fix for 937234: pt-query-advisor issues wrong RES.001 --- bin/pt-query-advisor | 15 +++++++++++++-- lib/QueryAdvisorRules.pm | 17 +++++++++++++++-- t/lib/QueryAdvisorRules.t | 9 ++++++++- 3 files changed, 36 insertions(+), 5 deletions(-) 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; From 1aa154d6c66eadd2c0438c3c95aba8ea68810839 Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Fri, 2 Nov 2012 01:18:10 -0300 Subject: [PATCH 2/4] Fix for 996069: Incorrect RES.001 when using aggregate functions --- bin/pt-query-advisor | 1 + lib/QueryAdvisorRules.pm | 1 + t/lib/QueryAdvisorRules.t | 5 +++++ 3 files changed, 7 insertions(+) diff --git a/bin/pt-query-advisor b/bin/pt-query-advisor index ebae0152..85bc76d8 100755 --- a/bin/pt-query-advisor +++ b/bin/pt-query-advisor @@ -3815,6 +3815,7 @@ sub get_rules { return unless scalar %groupby_col; my $cols = [ grep { _looks_like_column($_->{col}) } + grep { ! exists $_->{func} } @{$event->{query_struct}->{columns}} ]; foreach my $col ( @$cols ) { diff --git a/lib/QueryAdvisorRules.pm b/lib/QueryAdvisorRules.pm index 5753a4d8..119eb1d0 100644 --- a/lib/QueryAdvisorRules.pm +++ b/lib/QueryAdvisorRules.pm @@ -344,6 +344,7 @@ sub get_rules { # 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. diff --git a/t/lib/QueryAdvisorRules.t b/t/lib/QueryAdvisorRules.t index 2e49855d..0601ce74 100644 --- a/t/lib/QueryAdvisorRules.t +++ b/t/lib/QueryAdvisorRules.t @@ -471,6 +471,11 @@ my @cases = ( query => q{select 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()], + }, ); # Run the test cases. From c711c57859887f0855cc0f6c89fcc8cfb52c59bb Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Wed, 7 Nov 2012 09:51:35 -0300 Subject: [PATCH 3/4] Test for 933465: pt-query-advisor false positive on RES.001 --- t/lib/QueryAdvisorRules.t | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/t/lib/QueryAdvisorRules.t b/t/lib/QueryAdvisorRules.t index 0601ce74..2733d390 100644 --- a/t/lib/QueryAdvisorRules.t +++ b/t/lib/QueryAdvisorRules.t @@ -476,6 +476,20 @@ my @cases = ( 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. From 270a4710fb13af01940ec688347893dff2ffa2ce Mon Sep 17 00:00:00 2001 From: Brian Fraser Date: Fri, 16 Nov 2012 12:04:14 -0300 Subject: [PATCH 4/4] Removed a useless part of the regex that looked for something function-like, and made the NULL check case-insensitive --- bin/pt-query-advisor | 4 ++-- lib/QueryAdvisorRules.pm | 4 ++-- t/lib/QueryAdvisorRules.t | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bin/pt-query-advisor b/bin/pt-query-advisor index 85bc76d8..5f26b3fc 100755 --- a/bin/pt-query-advisor +++ b/bin/pt-query-advisor @@ -4056,8 +4056,8 @@ sub determine_table_for_column { 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 if $col eq '*' || uc($col) eq 'NULL'; + return if $col =~ /\A(?:\b[0-9]+\b|\@{1,2}.+)/; return $col; } diff --git a/lib/QueryAdvisorRules.pm b/lib/QueryAdvisorRules.pm index 119eb1d0..3d4cddc1 100644 --- a/lib/QueryAdvisorRules.pm +++ b/lib/QueryAdvisorRules.pm @@ -667,8 +667,8 @@ sub determine_table_for_column { 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 if $col eq '*' || uc($col) eq 'NULL'; + return if $col =~ /\A(?:\b[0-9]+\b|\@{1,2}.+)/; return $col; } diff --git a/t/lib/QueryAdvisorRules.t b/t/lib/QueryAdvisorRules.t index 2733d390..3a8f6ca5 100644 --- a/t/lib/QueryAdvisorRules.t +++ b/t/lib/QueryAdvisorRules.t @@ -468,7 +468,7 @@ my @cases = ( }, { name => 'Bug 937234: wrong RES.001', - query => q{select NULL, 1, COUNT(*), @@time_zone, foo as field2 from t1 group by field2}, + query => q{select NULL, null, 1, COUNT(*), @@time_zone, foo as field2 from t1 group by field2}, advice => [qw(CLA.001)], }, {