174295 - ANSI SQL requires all columns in SELECT to be in GROUP BY, unless they are in "aggregate" functions

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P3)

Description

[David Lawrence [:dkl]]

SQL standard requires that any column referenced in the HAVING
clause be in the GROUP BY clause or be the argument of some aggregate
function.  This is done to make sure it is a scalar expression. 
This touches on many cgi's throught out Bugzilla. Patch coming up.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to make GROUP BY more standard compliant (v1)

Comment 2

[Bradley Baetz (:bbaetz)]

Comment on attachment 102763 [details] [diff] [review]
Patch to make GROUP BY more standard compliant (v1)

The vast majority of these sql statements don't have HAVING clauses

This is real ugly - can we do better, w/o subselects?

Comment 3

[David Lawrence [:dkl]]

Having a HAVING clause is not required for this to be in effect. With PostgreSQL
and Oracle, if you have once or more columns in the SELECT that are part of an
aggregate function, then all other columns must be listed in a GROUP BY clause.
I am sure this could be nicer with subselects once Mysql supports them in the
next release. Otherwise this is a good interim solution that works with multiple
db's.

Comment 4

[Dave Miller [:justdave]]

Comment on attachment 102763 [details] [diff] [review]
Patch to make GROUP BY more standard compliant (v1)

This turns out to be necessary for Sybase as well. :)

This all looks really good except for Search.pm.  In the two months since this
patch was posted, there's been too many changes to Search.pm, and that one hunk
of the patch is completely bitrotted, and I can't figure out how to unrot it. 
I did do a quick hack on that one file that got it working on Sybase, but I
don't think it's production quality so I'd rather have you try your magic on it
again. :-)

Comment 5

[Dave Miller [:justdave]]

Just for reference, here's what I ended up doing:

Index: Bugzilla/Search.pm
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v
retrieving revision 1.37
diff -u -r1.37 Search.pm
--- Bugzilla/Search.pm	22 Nov 2002 02:38:18 -0000	1.37
+++ Bugzilla/Search.pm	10 Dec 2002 10:30:05 -0000
@@ -949,7 +949,9 @@
         }
     }
 
-    $query .= ") GROUP BY bugs.bug_id";
+    my $group_by = join(', ',@fields);
+    $group_by =~ s/\w+\s+AS\s+//gsi;
+    $query .= ") GROUP BY $group_by";
 
     if (@having) {
         $query .= " HAVING " . join(" AND ", @having);

Comment 6

[Dave Miller [:justdave]]

*** Bug 247998 has been marked as a duplicate of this bug. ***

Comment 7

[Dave Miller [:justdave]]

Attachment: Group by conversion patch

dkl's group-by patch unmodified from bug 247998

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 151416 [details] [diff] [review]
Group by conversion patch

>Index: Bugzilla/Search.pm

+    if ($fieldsref) {
>+        foreach my $field (@$fieldsref) {
>+            next if ($field =~ /(SUM|COUNT)/i);
>+            push(@groupby, $field);
>+        }
>+    }

I don't think this is a good way to exclude fields...  we have columns with
"count" as a substring of the column name, for example.  Perhaps check for the
open parenthesis after it as well?

	next if ($field =~ /(SUM|COUNT)\s*\(/i);

Comment 9

[David Lawrence [:dkl]]

Attachment: Group by conversion patch (v2)

Changed code in Search.pm that looked for aggregate functions to match
justdave's suggestion.

my @groupby;
if ($fieldsref) {
    foreach my $field (@$fieldsref) {
	next if ($field =~
/(trim|sum|count|length|lower|upper|to_char|date_format)\s*\(/i);
	push(@groupby, $field);
    }
}

Also added other possibilities such as upper, lower, to_char, etc.

Comment 10

[Joel Peshkin]

Comment on attachment 151455 [details] [diff] [review]
Group by conversion patch (v2)

This looks OK, but 2 questions before I r= this....

1) Has this been tested on mysql and/or oracle with timetracking enabled and
displayed in buglist?

2) Is there any performance hit on mysql due to the extra columns in the
group_by clause?

Comment 11

[David Lawrence [:dkl]]

I have a test database setup with 5-6 test bugs running on Mysql setup. I have
enabled timetracking and added some time values to several of the bugs. When
adding the appropriate time columns to the buglist.cgi list, it displays the
values properly with this patch attached. As for performance, someone else will
have to try it out with Mysql and I will test it with out PgSQL database here to
see if any slowdowns are noticed.

Comment 12

[Dave Miller [:justdave]]

(In reply to comment #9)
> 	next if ($field =~
> /(trim|sum|count|length|lower|upper|to_char|date_format)\s*\(/i);

> Also added other possibilities such as upper, lower, to_char, etc.

hmmm....  how safe would it be to do something along the lines of
    next if ($field =~ /^\s*\w+\s*\(/);  ?

Is it safe to assume that any one single word with an open paren after it is
going to be a function?

Comment 13

[Joel Peshkin]

Comment on attachment 151455 [details] [diff] [review]
Group by conversion patch (v2)

r=joel if you try this on postgres with timetracking enabled and buglisting
with %complete as a field and if you change join(',', @groupby) to join(', ',
@groupby)  [so the debug=1 output can fit on the screen]

Comment 14

[David Lawrence [:dkl]]

Attachment: Group by conversion patch (v3)

New patch with joels suggestion for proper separation in the join() part. Also
changed to check for values in @groupby before adding to query string just in
case we only select bugs.bug_id and no columns. As for testing, can someone
apply the patch that has a database with quite a bit of test bugs in it and
make sure that searches work properly with all of the timetracking columns
enabled? I have done so myself but I only have a few reports in my test MySQL
database.

Comment 15

[Joel Peshkin]

I'm not sure if you want to make this change, but....

You can simplify things by putting bug_id in @groupby before you start adding
anything else.  Then, you just have to unconditionally append the joined list.

Comment 16

[David Lawrence [:dkl]]

Attachment: Group by converstion patch (v4)

That does seem cleaner and like everyone says about Perl in general, there is
always more than one way to do it ;) Changed patch to remove groupby condition
and explicitly added bugs.bug_id to the groupby list.

Comment 17

[Joel Peshkin]

Comment on attachment 151536 [details] [diff] [review]
Group by converstion patch (v4)

r=joel
Tested on mysql including timetracking columns

Comment 18

[Joel Peshkin]

Comment on attachment 151536 [details] [diff] [review]
Group by converstion patch (v4)

whoops! not so fast....

Unknown column '42217354' in 'group statement' when we run a graphical report. 
Looks like we need to kill numeric fields from the @groups list as well.

Comment 19

[David Lawrence [:dkl]]

Attachment: Group by converstion patch (v5)

Skip numeric column names when forming @groupby. Fixes the problem with charts
for me in my testing.

Comment 20

[Joel Peshkin]

Comment on attachment 151722 [details] [diff] [review]
Group by converstion patch (v5)

r=joel

Same assumption...  you've tested in on PG with timetracking fields in the
buglist.

Comment 21

[David Lawrence [:dkl]]

Comment on attachment 151722 [details] [diff] [review]
Group by converstion patch (v5)

Seeking second review

Comment 22

[Jouni Heikniemi]

Comment on attachment 151722 [details] [diff] [review]
Group by converstion patch (v5)

>Index: Bugzilla/FlagType.pm
>===================================================================

>-    $query .= " GROUP BY flagtypes.id " if ($include_count || $having ne "");
>+    $query .= " GROUP BY flagtypes.id, " . join(",", @base_columns) if ($include_count || $having ne "");

This produces

"GROUP BY flagtypes.id, 1,flagtypes.id,flagtypes.name, ..." 

The 1 didn't give an error for me, but a similar one did for Joel (comment 18).
Also, you have flagtypes.id twice (which isn't a real error though).

Nit: The line exceeds 80 chars.

>Index: Bugzilla/Search.pm
>+            next if ($field =~ /(trim|sum|count|length|lower|upper|to_char|date_format)\s*\(/i || $field =~ /^\d+$/);

Nit: Another line too long.

Tested on MySQL 4.0.18.

Comment 23

[Tomas Kopal]

I had problems with Search.pm with this patch, as it was not correctly removing
"relevance" aggregate and Postgres was complaining. I had to move the loop to
the end (just before query construction) and modified it to use $fields, instead
of $fieldsref.

I ended up with this (using older version, with just SUM and COUNT):

Index: Search.pm
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v
retrieving revision 1.60
diff -u -r1.60 Search.pm
--- Search.pm   17 Jul 2004 01:33:51 -0000      1.60
+++ Search.pm   18 Jul 2004 06:17:29 -0000
@@ -1166,7 +1166,16 @@
         }
     }

-    $query .= ") GROUP BY bugs.bug_id";
+    # SQL standard requires that any column referenced in the HAVING
+    # clause be in the GROUP BY clause or be the argument of some aggregate
+    # function.  This is done to make sure it is a scalar expression.
+    my @groupby;
+    foreach my $field (@fields) {
+        next if ($field =~ /(SUM|COUNT)/i);
+        push(@groupby, $field );
+    }
+
+    $query .= ") GROUP BY " . join(',', @groupby);

     if (@having) {
         $query .= " HAVING " . join(" AND ", @having);

Comment 24

[Dave Miller [:justdave]]

bug 264249 also makes this conversion for Bugzilla/Series.pm

Comment 25

[David Lawrence [:dkl]]

Attachment: Group by converstion patch (v6)

Changes for editclassifications.cgi, Series.pm and Chart.pm added in. Also
moved @groupby down as per Tom's suggestion. Shortening the regex is still
needed in Search.pm if someone can suggest the best way to do that without just
using the multiline regex switch (m).

Comment 26

[Max Kanat-Alexander]

Comment on attachment 162020 [details] [diff] [review]
Group by converstion patch (v6)

>Index: Bugzilla/FlagType.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v
>retrieving revision 1.9
>diff -u -r1.9 FlagType.pm
>--- Bugzilla/FlagType.pm	4 Aug 2004 16:17:10 -0000	1.9
>+++ Bugzilla/FlagType.pm	13 Oct 2004 22:49:28 -0000
>@@ -135,7 +135,7 @@
>     my $where_clause = "WHERE " . join(" AND ", @criteria);
>     
>     my $query = "$select_clause $from_clause $where_clause";
>-    $query .= " GROUP BY flagtypes.id " if ($include_count || $having ne "");
>+    $query .= " GROUP BY flagtypes.id, " . join(",", @base_columns) if ($include_count || $having ne "");

  *nit* The precedence of the if and the "." are a little unclear, there. You
might want to use parens to make sure that it's clear what we're doing there.

>-    $query .= ") GROUP BY bugs.bug_id";
>+    # SQL standard requires that any column referenced in the HAVING
>+    # clause be in the GROUP BY clause or be the argument of some aggregate
>+    # function.  This is done to make sure it is a scalar expression.
>+    my @groupby;
>+    foreach my $field (@fields) {
>+        next if ($field =~ /(trim|sum|count|length|lower|upper|to_char|date_format)\s*\(/i || $field =~ /^\d+$/);

  *nit* Can't we just search for parens? Is there any valid real field that
will actually have a paren?

  All the rest of this looks "obviously correct" to me. If you assert that it
works without bugs, I'm willing to give it a review+.

Comment 27

[Max Kanat-Alexander]

Comment on attachment 162020 [details] [diff] [review]
Group by converstion patch (v6)

Tomas, could you also give this patch a review?

Comment 28

[Tomas Kopal]

(In reply to comment #27)
> (From update of attachment 162020 [details] [diff] [review] [edit])
> Tomas, could you also give this patch a review?
> 

Sorry, I don't have the rights to modify the attachment to set the flag, so here
just goes my comments:

I am still not completely sure this is the best way to tackle this problem. The
main reason for my concern is this comment
(https://bugzilla.mozilla.org/show_bug.cgi?id=173130#c3):

"Also, ANSI SQL says if we use GROUP BY, that we have to include every column
mentioned in the SELECT part in the GROUP BY clause.  Sybase won't let you put
more than 31 columns in a GROUP BY, and also has temporary table size
limitations with GROUP BY that are pretty darn restrictive.  So the "GROUP BY
bugs.bug_id" has been replaced with "DISTINCT bugs.bug_id" at the beginning of
the SELECT part."

DISTINCT will not help for Postgres, we need all the columns in GROUP BY. But if
it breaks Sybase, it may break others. Though this patch is a good short term
solution for MySQL and Postgres, I would like to be as generic as possible.

So I was thinking about alternatives, and so far the best solution appears to be
a new function in DBcompat layer (once we have it in :-) ), taking two
parameters, first string (or array) with columns which are mandatory, present
for all DBs, and second with optional columns, present for Postgres and similar.
The function would return the first param for MySQL and Sybase, and both
concatenated for Postgres.

Note this probably affects only Search.pm monster, all the other are quite
simple and can go in as they are (although Bug.pm with 29 columns is close).

Also with Search.pm, I sort of don't like the way we are first putting all
neccessary columns to a list of fields, and then analyzing this list and trying
to separate columns which needs to go to the GROUP BY statement. Are we sure we
are not missing any column? I am not :-).

It looks we are just complicating the thing more than it needs to be. Wouldn't
it be possible e.g. to create a new list, where we would build the list of
group_by columns in the same as we are currently doing with e.g. order_by?

Functionality wise, I am running my code with this patch for a while now and it
seems to work fine. The only thing I have added is one more change in
Bugzilla/User.pm, function get_userlist. My GROUP BY "have userid, login_name,
realname" instead of just "userid", otherwise when you turn "usemenuforusers"
option on, Postgres will fail on bug creation.

If the Bugzilla/User.pm gets fixed, I am giving the patch r+ except for the
Search.pm part, which gets r-. I would even recommend taking the Search.pm
changes to a new bug, so we can check in the rest here...

Comment 29

[Tomas Kopal]

Also see discussion at bug 173130. There are some ideas how to cope with the
GROUP BY problem in Search.pm.

Comment 30

[Max Kanat-Alexander]

Comment on attachment 162020 [details] [diff] [review]
Group by converstion patch (v6)

OK, that's an r- from Tomas, although it's an r+ on certain parts.

Comment 31

[Tomas Kopal]

Well, we should probably give this another shot, this needs to get fixed. Ideas,
anyone?

Comment 32

[Tomas Kopal]

Ok, I finally managed to run some tests on MySQL:
I created two tables, bugs and flags, both with 300.000 rows. I run two queries
similar to what are generating in search module, left-joining these two tables,
and selecting all columns with a GROUP BY and HAVING statements. First query was
GROUPing only by bugs.bug_id (thats what the code is doing at the moment), the
second by all columns of bugs table (thats what Pg needs). I run both queries
six times, discarding first run.
The first query took around 84 seconds, the second around 378 seconds. That
corresponds what MySQL documentation is suggesting - less columns is less
sorting and thus better performance.
So, although specifying all columns is ANSI standard, we should not penalize
MySQL performance for that.
My suggestion is: Lets create a new method in the DB compatibility layer,
sql_group_by(), taking two parameters. First parameter will be a list of all
columns we need to group by (that's what we are using now), the second will be
list of all the remaining columns.
DB base class will join these lists together and do GROUP BY containing all
columns, as ANSI SQL requires. MySQL specialization will override this method
and will return just the first list, discarding the second, so we'll end up with
the same statement as we have now. This also addresses the problem with Sybase
not supporting more than certain number of columns in GROUP BY statement.

Also, for Search.pm, I would suggest to construct the two lists during query
construction, and not as a post-procesing, by parsing the resulting query. It
should be more obvious what we are doing. Although I do not insist on this :-).

Comments?

DKL: Will you have some time to look at this in a very near future? We don't
have much time before freeze, but I don't want to "steal" this from you without
even asking :-).

Comment 33

[Tomas Kopal]

(In reply to comment #32)
> Also, for Search.pm, I would suggest to construct the two lists during query
> construction, and not as a post-procesing, by parsing the resulting query. It
> should be more obvious what we are doing. Although I do not insist on this :-).

I just looked at the implementation and I realized that the columns are coming
in as a parameter, they are not constructed in Search.pm. So the current loop
parsing the columns is probably inevitable. So ignore my suggestion above :-).
We want to keep that loop, we just want to remove bugs.bug_id from the list also
(as it will be explicit first parameter to sql_group_by method and we don't want
duplicates).

Comment 34

[Tomas Kopal]

We really need to get this in, so I am taking it.

I have implemented a new DB method sql_group_by, as I described it before, and
modified the code to use it everywhere.
Everything works ok, except for Search.pm. Current solution with a loop parsing
all fields coming to the query seems to be the only viable at the moment, but it
currently does not work correctly, as we can't simply ignore all fields with
functions.
- First, the exception for GROUP BY holds only for aggregate functions, such as
SUM or COUNT. Functions as LOWER does not qualify, we still need to group by the
column used as a parameter for the function.
- Moreover, some of the fields contain expresstion with aggregate functions and
normal columns. In this case, we can leave out the columns used in the aggregate
functions, but we still need to inglude the other columns.

These problems seems to affect time tracking (expression
"(100*((SUM(ldtime.work_time)*COUNT(DISTINCT
ldtime.bug_when)/COUNT(bugs.bug_id))/((SUM(ldtime.work_time)*COUNT(DISTINCT
ldtime.bug_when)/COUNT(bugs.bug_id))+bugs.remaining_time))) AS
percentage_complete" where bugs.remaining_time is not used in aggregate
function), and full text search (expression "(SUM($term1)/COUNT($term1) +
$term2) AS relevance" where $term2 is not used in aggregate function, and it
corresponds to full text search in bugs.short_desc colum).

Timetracking seems to add the bugs.remaining_time column to fields, when used
(buglist.cgi:
# remaining and actual_time are required for precentage_complete calculation:
if (lsearch(\@displaycolumns, "percentage_complete") >= 0) {
    push (@selectcolumns, "remaining_time");
    push (@selectcolumns, "actual_time");
}
), so the only problem remains with full text search, which is, fortunatelly
simple to fix. I will post the patch later today.

It's a solution which is prone to problems in the future, such as if we add more
expressions like the timetracking one for display in the buglist page. However,
I can't see any better solution without redesigning the whole search class (it
probably should be done, as passing DB columns as parameters is not very object
oriented approach, better would be to set some flags what we want to search for
and Search class would create the query based on the flags). But that's
definitelly outside of scope of this bug.

Comment 35

[Tomas Kopal]

Attachment: V7

OK, here is my version. Tested on MySQL, very limited testing on Postgres.

Comment 36

[Max Kanat-Alexander]

Comment on attachment 176708 [details] [diff] [review]
V7

OK, this *looks* good to me, but I need Joel to comment on the Search.pm stuff.
:-)

Comment 37

[Joel Peshkin]

+        next if ($field =~ /(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i ||

Is this missing an anchor??

Comment 38

[Joel Peshkin]

Comment on attachment 176708 [details] [diff] [review]
V7

Close, but please check the anchor situation and try this in pg with the
realname column included.  I suspect you will need to break the CASE WHEN into
both mentioned fields.

Comment 39

[Tomas Kopal]

(In reply to comment #38)
> (From update of attachment 176708 [details] [diff] [review] [edit])
> Close, but please check the anchor situation and try this in pg with the
> realname column included.  I suspect you will need to break the CASE WHEN into
> both mentioned fields.
> 

I do not understand what you mean by anchor. If you are questioning the regexp,
I don't see anything wrong with it...

With respect to the CASE WHEN, you are right, good catch, thanks. I will fix that.

Comment 40

[Joel Peshkin]

The anchor question is .... should this be....
/(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i
or
/^(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i 

I am being a bit paranoid here.  Changing this is optional but suggested IMHO.

Comment 41

[Tomas Kopal]

Attachment: V7.1

OK, new version, fixed the CASE WHEN problem, no other changes.

(In reply to comment #40)
> The anchor question is .... should this be....
> /(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i
> or
> /^(AVG|SUM|COUNT|MAX|MIN|VARIANCE)\s*\(/i 
> 
> I am being a bit paranoid here.  Changing this is optional but suggested
IMHO.
> 
> 

Ahhh, this anchor :-). I think it's better as it is, we want to get rid of
anything using any of the aggregate functions, as e.g.
"(100*((SUM(ldtime.work_time)*COUNT(DISTINCT
ldtime.bug_when)/COUNT(bugs.bug_id))/((SUM(ldtime.work_time)*COUNT(DISTINCT
ldtime.bug_when)/COUNT(bugs.bug_id))+bugs.remaining_time))) AS
percentage_complete".
Second option would be to look for all these full expressions as we are
currently using them, that has the advantage that we can throw code error if we
hit expression we do not recognize (if someone edits the code and does not
update the group by clause). But listing all of them here would be quite messy
IMHO.

Comment 42

[Joel Peshkin]

Comment on attachment 176871 [details] [diff] [review]
V7.1

r=joel
I can't think of a better way to do it at the moment, but I am a bit worried
that the CASE WHEN handling is rather specific to the current situation with
realname.
If someone adds another such column, we may have to revisit this, probably by
adding something to the column definition table.

Comment 43

[Max Kanat-Alexander]

Yes, a better solution would be to somehow get a recursive function and pass the
contents of the CASE WHEN into it.

Or, even better, require all CASE WHEN statements to have an alias, and just use
the alias in the GROUP BY statement (if all our DBs allow that).

However, this is good enough for now.

Comment 44

[Tomas Kopal]

(In reply to comment #42)
> (From update of attachment 176871 [details] [diff] [review] [edit])
> r=joel
> I can't think of a better way to do it at the moment, but I am a bit worried
> that the CASE WHEN handling is rather specific to the current situation with
> realname.
> If someone adds another such column, we may have to revisit this, probably by
> adding something to the column definition table.
> 

Yeah, that's what I was describing in comment #34. It would be great to do it in
a generic way, but it can't be done without redesigning Search.pm interface
(either by adding a column, or by switching from direct SQL to something more
abstract, which is IMHO even cleaner solution). But that's really a different bug.

Comment 45

[Tomas Kopal]

(In reply to comment #43)
> Yes, a better solution would be to somehow get a recursive function and pass the
> contents of the CASE WHEN into it.
> 
> Or, even better, require all CASE WHEN statements to have an alias, and just use
> the alias in the GROUP BY statement (if all our DBs allow that).
> 
> However, this is good enough for now.

Hmmm, that brings an idea... If we require all expressions to have an alias (and
use the AS keyword), we can separate the alias name by very simple regex. That
would solve all problems, and we could even drop the regexp for aggregate
functions. And it would be more generic...
So the only question is - is it supported by all DBs? I just tested that on
Pg8.0 and it works. I will test it on 7.4 tonight. Can someone test it on 7.3?
Does someone know what will Oracle or Sybase do with aliases as parameters to
the GROUP BY clause?

Comment 46

[Max Kanat-Alexander]

Hey Ed, do you know the answer to Tomas's question in comment 45?

Tomas: Re Pg 7.3, I know that aliases work in the ORDER BY, so I'd assume that
they work in the GROUP BY...

Comment 47

[Ed Sabol]

(In reply to comment #45)
> Does someone know what will Oracle or Sybase do with aliases as parameters to
> the GROUP BY clause?

Don't know about Oracle, but, as coincidence would have it, I was just writing
some code for work the other day to do something very similar to what you are
describing in Sybase and I came to the same conclusion. It's a lot cleaner if
you require all expressions to be aliased. Anyway, for Sybase, if you alias an
expression in the SELECT clause, then you must use the alias in the GROUP BY
clause. I hope that answers your question, at least as far as Sybase goes.

Comment 48

[Tomas Kopal]

OK, I am convinced now, I will change the Search.pm part to use aliases. It
seems to be the best way to go.

Comment 49

[Tomas Kopal]

Attachment: V7.2

(In reply to comment #48)
> OK, I am convinced now, I will change the Search.pm part to use aliases. It
> seems to be the best way to go.

Hmmm, I implemented that using aliases, it was nice, simple, clean. But
postgres said: "ERROR:	aggregates not allowed in GROUP BY clause" and that's
the end of the story.
So here is a partial version, still skipping aggregate functions, but the CASE
WHEN case is handled by aliases, as Max suggested. It's still a bit better than
the previous version though.

Comment 50

[Joel Peshkin]

Comment on attachment 176996 [details] [diff] [review]
V7.2

I'd be a little happier if 
if ($field =~ /.*AS\s+(\w+)$/i) {

took advantage of the fact that we know AS is upper case and has whitespace
before it as well.  Why not /\s+AS\s+(\w+)$/ without the case insensitivity?

You can change that or not 
r=joel

Comment 51

[Tomas Kopal]

Attachment: V7.3

(In reply to comment #50)
> (From update of attachment 176996 [details] [diff] [review] [edit])
> I'd be a little happier if 
> if ($field =~ /.*AS\s+(\w+)$/i) {
> 
> took advantage of the fact that we know AS is upper case and has whitespace
> before it as well.  Why not /\s+AS\s+(\w+)$/ without the case insensitivity?
> 
> You can change that or not 
> r=joel
> 

New version. I have added the white-space before the AS in the regex as you
suggest. I don't want to make it case insensitive, as there are still places
where SQL is lowercase (it's probably not in the AS which are coming to
Search.pm, but I want to play safe - upper case is not required by SQL and if
someone uses it in some new code, it would produce strange looking errors).
I have also fixed full text search - it uses aggregate functions as well, so we
can't use alias, we need explicit column name.
No other changes done.

Comment 52

[Joel Peshkin]

Comment on attachment 177052 [details] [diff] [review]
V7.3

r=joel
Careful on checkin - there were a few glitches in appliying the patch.

Comment 53

[Max Kanat-Alexander]

Yeah, the DB::Mysql part of the patch was corrupted, but I fixed it on checkin.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.288; previous revision: 1.287
done
Checking in describekeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/describekeywords.cgi,v  <--  describekeywords.cgi
new revision: 1.14; previous revision: 1.13
done
Checking in editclassifications.cgi;
/cvsroot/mozilla/webtools/bugzilla/editclassifications.cgi,v  <-- 
editclassifications.cgi
new revision: 1.9; previous revision: 1.8
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.51; previous revision: 1.50
done
Checking in editkeywords.cgi;
/cvsroot/mozilla/webtools/bugzilla/editkeywords.cgi,v  <--  editkeywords.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in editmilestones.cgi;
/cvsroot/mozilla/webtools/bugzilla/editmilestones.cgi,v  <--  editmilestones.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.75; previous revision: 1.74
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
new revision: 1.20; previous revision: 1.19
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.90; previous revision: 1.89
done
Checking in summarize_time.cgi;
/cvsroot/mozilla/webtools/bugzilla/summarize_time.cgi,v  <--  summarize_time.cgi
new revision: 1.3; previous revision: 1.2
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.69; previous revision: 1.68
done
Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.11; previous revision: 1.10
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.91; previous revision: 1.90
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.7; previous revision: 1.6
done

Comment 54

[Tomas Kopal]

(In reply to comment #53)
> Yeah, the DB::Mysql part of the patch was corrupted, but I fixed it on checkin.

Max, for some reason, Bugzilla/User.pm has not been checked in. Can you, please,
re-apply the patch to this file and check it in?

Comment 55

[Max Kanat-Alexander]

OK. Turns out the User.pm part of the patch was corrupted, which is why diff
thought the User.pm part was corrupted (it's immediately afterward). It was just
missing one line of context on the end. I fixed it and checked it in.

Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.49; previous revision: 1.48
done