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

Proposed by Brian Fraser on 2012-11-16
Status: Merged
Approved by: Daniel Nichter on 2012-12-05
Approved revision: 444
Merged at revision: 483
Proposed branch: lp:~percona-toolkit-dev/percona-toolkit/fix-937234-pqa-wrong-res.001
Merge into: lp:percona-toolkit/2.1
Diff against target: 124 lines (+57/-5)
3 files modified
bin/pt-query-advisor (+14/-2)
lib/QueryAdvisorRules.pm (+16/-2)
t/lib/QueryAdvisorRules.t (+27/-1)
To merge this branch: bzr merge lp:~percona-toolkit-dev/percona-toolkit/fix-937234-pqa-wrong-res.001
Reviewer Review Type Date Requested Status
Daniel Nichter 2012-11-16 Approve on 2012-12-05
Review via email: mp+134683@code.launchpad.net
To post a comment you must log in.
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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/pt-query-advisor'
2--- bin/pt-query-advisor 2012-11-13 15:22:01 +0000
3+++ bin/pt-query-advisor 2012-11-16 15:07:22 +0000
4@@ -3816,9 +3816,14 @@
5 grep { $_->{column} }
6 @$groupby;
7 return unless scalar %groupby_col;
8- my $cols = $event->{query_struct}->{columns};
9+ my $cols = [
10+ grep { _looks_like_column($_->{col}) }
11+ grep { ! exists $_->{func} }
12+ @{$event->{query_struct}->{columns}}
13+ ];
14 foreach my $col ( @$cols ) {
15- return 0 unless $groupby_col{ $col->{col} };
16+ return 0 unless $groupby_col{ $col->{col} }
17+ || ($col->{alias} && $groupby_col{ $col->{alias} });
18 }
19 return;
20 },
21@@ -4052,6 +4057,13 @@
22 return;
23 }
24
25+sub _looks_like_column {
26+ my $col = shift;
27+ return if $col eq '*' || uc($col) eq 'NULL';
28+ return if $col =~ /\A(?:\b[0-9]+\b|\@{1,2}.+)/;
29+ return $col;
30+}
31+
32 sub _d {
33 my ($package, undef, $line) = caller 0;
34 @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
35
36=== modified file 'lib/QueryAdvisorRules.pm'
37--- lib/QueryAdvisorRules.pm 2012-04-03 17:03:17 +0000
38+++ lib/QueryAdvisorRules.pm 2012-11-16 15:07:22 +0000
39@@ -341,11 +341,17 @@
40 grep { $_->{column} }
41 @$groupby;
42 return unless scalar %groupby_col;
43- my $cols = $event->{query_struct}->{columns};
44+ # Skip non-columns -- NULL, digits, functions, variables
45+ my $cols = [
46+ grep { _looks_like_column($_->{col}) }
47+ grep { ! exists $_->{func} }
48+ @{$event->{query_struct}->{columns}}
49+ ];
50 # All SELECT cols must be in GROUP BY cols clause.
51 # E.g. select a, b, c from tbl group by a; -- non-deterministic
52 foreach my $col ( @$cols ) {
53- return 0 unless $groupby_col{ $col->{col} };
54+ return 0 unless $groupby_col{ $col->{col} }
55+ || ($col->{alias} && $groupby_col{ $col->{alias} });
56 }
57 return;
58 },
59@@ -658,6 +664,14 @@
60 return;
61 }
62
63+sub _looks_like_column {
64+ my $col = shift;
65+ # NULL, numbers, variables and functions are definitely not columns
66+ return if $col eq '*' || uc($col) eq 'NULL';
67+ return if $col =~ /\A(?:\b[0-9]+\b|\@{1,2}.+)/;
68+ return $col;
69+}
70+
71 sub _d {
72 my ($package, undef, $line) = caller 0;
73 @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
74
75=== modified file 't/lib/QueryAdvisorRules.t'
76--- t/lib/QueryAdvisorRules.t 2012-03-06 13:56:08 +0000
77+++ t/lib/QueryAdvisorRules.t 2012-11-16 15:07:22 +0000
78@@ -9,7 +9,7 @@
79 use strict;
80 use warnings FATAL => 'all';
81 use English qw(-no_match_vars);
82-use Test::More tests => 87;
83+use Test::More;
84
85 use PerconaTest;
86 use PodParser;
87@@ -466,6 +466,30 @@
88 query => "select col1, col2 from tbl where i=1 order by col1, col2 desc",
89 advice => [qw(CLA.007)],
90 },
91+ {
92+ name => 'Bug 937234: wrong RES.001',
93+ query => q{select NULL, null, 1, COUNT(*), @@time_zone, foo as field2 from t1 group by field2},
94+ advice => [qw(CLA.001)],
95+ },
96+ {
97+ name => 'Bug 996069: wrong RES.001',
98+ query => q{SELECT cola, MAX(colb) FROM table WHERE cola = 123 GROUP BY cola},
99+ advice => [qw()],
100+ },
101+ {
102+ name => 'Bug 933465: false positive on RES.001',
103+ query => q{select name, population, count(*) from world.Country group by name, population},
104+ advice => [qw(CLA.001)],
105+ },
106+ {
107+ name => 'Bug 933465: false positive on RES.001',
108+ query => q{SELECT ID_organization,ToNalog,ID_nachtype, COUNT(*) as Cnt }
109+ . q{FROM buh_provodka_zp }
110+ . q{GROUP BY ID_organization,ToNalog,ID_nachtype }
111+ . q{HAVING Cnt>1 }
112+ . q{ORDER BY Cnt DESC},
113+ advice => [qw(CLA.001)],
114+ },
115 );
116
117 # Run the test cases.
118@@ -524,4 +548,6 @@
119 qr/Complete test coverage/,
120 '_d() works'
121 );
122+
123+done_testing;
124 exit;

Subscribers

People subscribed via source and target branches