509497 - Implement sql_group_concat for all databases

Status: RESOLVED FIXED

(Bugzilla :: Database, enhancement, P1)

Description

[Max Kanat-Alexander]

We need to have a GROUP_CONCAT equivalent for all DBs. I'll write the Pg one. Xiaoou, can you write one for Oracle? Perhaps using a similar method to what I'm going to do for Pg.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

You can test this by doing "TRUNCATE TABLE bugs_fulltext;" and then running checksetup.

Comment 2

[Frédéric Buclin]

Comment on attachment 393559 [details] [diff] [review]
v1

>Index: Bugzilla/Install/DB.pm

>+                 . $dbh->sql_group_concat('longdescs.thetext', '\'\n\'')
>+          . ', ' . $dbh->sql_group_concat('nopriv.thetext',    '\'\n\'') .

PostgreSQL fails. Should be $dbh->quote("\n") (tested on both MySQL and PostgreSQL). Otherwise looks good.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Comment 4

[Frédéric Buclin]

Comment on attachment 393656 [details] [diff] [review]
v2

r=LpSolit

Comment 5

[Max Kanat-Alexander]

Waiting for Xiaoou's patch to grant approval.

Comment 6

[Xiaoou]

Attachment: oracle-v1

This is an Oracle group_concat.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 393711 [details] [diff] [review]
oracle-v1

>Index: Bugzilla/DB/Oracle.pm
>+    if ( !@$t_string_delim ) {
>+        $self->do("CREATE OR REPLACE TYPE T_STRING_DELIM AS OBJECT "
>+              . "( p_STRING    VARCHAR2(32767), p_DELIMITER VARCHAR2(256));");

  What if the input is longer than 32767? For example, our MAX_COMMENT_LENGTH is 65535, by default, I believe, and we do use group_concat on comments at least once.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 393711 [details] [diff] [review]
oracle-v1

Also, isn't there some simpler way to implement this, maybe using the collect() function? I'm thinking something similar to what I did for PostgreSQL, where you make it into an array and then connect the elements of the array by the separator.

Comment 9

[Frédéric Buclin]

Comment on attachment 393711 [details] [diff] [review]
oracle-v1

I'm not an Oracle expert. :)

Comment 10

[Xiaoou]

(In reply to comment #7)

>   What if the input is longer than 32767? For example, our MAX_COMMENT_LENGTH
> is 65535, by default, I believe, and we do use group_concat on comments at
> least once.

The max string length allowed here is 32767 in Oracle :-(
Also, using the ODCIAggregate interface is the best generic way I think.

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> Also, using the ODCIAggregate interface is the best generic way I think.

  Why not create a TABLE custom type, collect() to it, and then join the collection with the separator?

Comment 12

[Frédéric Buclin]

xiaoou, look at http://www.oracle-base.com/articles/10g/StringAggregationTechniques.php#collect

It uses the new COLLECT() function from Oracle 10g, which is what we require for Bugzilla. It also looks faster than the older user-defined functions.

Comment 13

[Max Kanat-Alexander]

Or what about WM_CONCAT? What circumstances is it available under?

Comment 14

[Michael Thomas (Mockodin)]

Just to chime in, while not directly supported, mssql can be implemented in a udf for mssql as well. It's actually something I wrote and removed for the mssql port, I'll put it back in.

Comment 15

[Frédéric Buclin]

Comment on attachment 393656 [details] [diff] [review]
v2

>Index: Bugzilla/DB.pm

>+sub sql_group_concat {
>+    my ($self, $text, $separator) = @_;
>+    $separator ||= "','";
>+    return "array_to_string(array_accum($text), $separator)";
>+}

Thinking about this a bit more, shouldn't we move this definition of sql_group_concat() into Pg.pm and move the MySQL one here? It's more likely that GROUP_CONCAT() is being used by other DBs than this code above.

Comment 16

[Frédéric Buclin]

Comment on attachment 393656 [details] [diff] [review]
v2

>Index: Bugzilla/Install/DB.pm

>-        if (UNIVERSAL::can($dbh, 'sql_group_concat')) {

Also, if it's definitely not possible to pass over the 32767 characters limitation on Oracle, we should keep the old code here and only replace this line by: if (ref $dbh ne 'Bugzilla::DB::Oracle'). This check can go away later if a workaround is found.

Comment 17

[Xiaoou]

Attachment: oracle-v2

Actually, collect() doesn't support CLOB :-(

I still use the ODCIAggregate interface for this purpose and the oracle-v2 will not have the 32767 limit.

Comment 18

[Xiaoou]

Attachment: oracle-v3

oracle-v3 will be better.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 394792 [details] [diff] [review]
oracle-v3

Okay, I'm assuming that YOU TESTED THIS AND THAT IT WORKS. It looks okay to me.

Comment 20

[Max Kanat-Alexander]

(In reply to comment #15)
> Thinking about this a bit more, shouldn't we move this definition of
> sql_group_concat() into Pg.pm and move the MySQL one here? It's more likely
> that GROUP_CONCAT() is being used by other DBs than this code above.

  Oh! Actually, group_concat isn't ANSI, so it shouldn't have been in DB.pm at all. I'll move it to Pg.pm on checkin.

Comment 21

[Max Kanat-Alexander]

Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.127; previous revision: 1.126
done
Checking in Bugzilla/DB/Oracle.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Oracle.pm,v  <--  Oracle.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/DB/Pg.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Pg.pm,v  <--  Pg.pm
new revision: 1.32; previous revision: 1.31
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.68; previous revision: 1.67
done