Bug #108098 for DBIx-Class: Issue with alias names in +as with group by

Thu Oct 29 14:06:02 2015 tony.langdon [...] wcn.co.uk - Ticket created

CC:	Tony WCN <tony.langdon [...] wcn.co.uk>
Subject:	Issue with alias names in +as with group by - 0.082810
Date:	Thu, 29 Oct 2015 18:05:32 +0000
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Tony Langdon <tony.langdon [...] wcn.co.uk>

I have attached minimal code to reproduce the issue. I think part of the problem is in DBIx::Class::Storage::DBI in _select_args line 2481 $prefetch_needs_subquery = ! scalar grep { $_ ne $attrs->{alias} } keys %{ $grp_aliases->{grouping} || {} }; I think that the ! is wrong since the subquery would only be needed if there are grouping aliases that aren’t that of this table. That’s what I was trying to reproduce with the attached code (using Artist / CDs) but I also discovered that the aggregate column is not available in the result row when including the aggregate column. e.g. This is OK (with the_wall_cds being a custom relationship to cds) my $spec = { '+as' => [ 'name', 'number' ], '+select' => [ 'me.name', 'count(the_wall_cds.cdid)' ], 'columns' => [], 'join' => 'the_wall_cds', 'group_by' => ['me.name'], }; my $rs = $artist_rs->search_rs( {}, $spec ); However this is not: my $spec = { '+as' => [ 'name', 'the_wall_cds.number' ], '+select' => [ 'me.name', 'count(the_wall_cds.cdid)' ], 'columns' => [], 'join' => 'the_wall_cds', 'group_by' => ['me.name'], }; my $rs = $artist_rs->search_rs( {}, $spec ); Attached code should be self contained illustration of the issue. We’re trying to update our application to a more recent version of DBIx::Class (from 0.08196) and this is changed behaviour, Thanks.

I have attached minimal code to reproduce the issue.

I think part of the problem is in DBIx::Class::Storage::DBI in _select_args line 2481

$prefetch_needs_subquery = ! scalar grep { $_ ne $attrs->{alias} } keys %{ $grp_aliases->{grouping} || {} };

I think that the ! is wrong since the subquery would only be needed if there are grouping aliases that aren’t that of this table.

That’s what I was trying to reproduce with the attached code (using Artist / CDs) but I also discovered that the aggregate column is not available in the result row when including the aggregate column.

e.g.

This is OK (with the_wall_cds being a custom relationship to cds)

Show quoted text

my $spec = { '+as' => [ 'name', 'number' ],

'+select' => [ 'me.name', 'count(the_wall_cds.cdid)' ],

'columns' => [],

'join' => 'the_wall_cds',

'group_by' => ['me.name'], };

my $rs = $artist_rs->search_rs( {}, $spec );

However this is not:

Show quoted text

my $spec = { '+as' => [ 'name', 'the_wall_cds.number' ],

'+select' => [ 'me.name', 'count(the_wall_cds.cdid)' ],

'columns' => [],

'join' => 'the_wall_cds',

'group_by' => ['me.name'], };

my $rs = $artist_rs->search_rs( {}, $spec );

Attached code should be self contained illustration of the issue.

We’re trying to update our application to a more recent version of DBIx::Class (from 0.08196) and this is changed behaviour,

Thanks.

Download DBIxTest.tgz
application/octet-stream 1.7k

Message body not shown because it is not plain text.

Tue Nov 03 06:42:57 2015 ribasushi [...] leporine.io - Correspondence added

On Thu Oct 29 19:06:02 2015, tony.langdon@wcn.co.uk wrote: Show quoted text

> > That’s what I was trying to reproduce with the attached code (using > Artist / CDs) but I also discovered that the aggregate column is not > available in the result row when including the aggregate column. > > my $spec = { '+as' => [ 'name', 'the_wall_cds.number' ], > '+select' => [ 'me.name', 'count(the_wall_cds.cdid)' > ], > 'columns' => [], > 'join' => 'the_wall_cds', > 'group_by' => ['me.name'], }; > my $rs = $artist_rs->search_rs( {}, $spec ); >

You have several issues tangled here, I will first address the above one (as it is a clear bug in a subsystem I am currently working on). Once a devrel is available addressing the above, we will revisit your actual failure case. Sorry for the delay, last month was too conference-y.

Tue Nov 03 06:42:59 2015 The RT System itself - Status changed from 'new' to 'open'

Tue Feb 09 08:19:49 2016 ribasushi [...] leporine.io - Correspondence added

On Thu Oct 29 19:06:02 2015, tony.langdon@wcn.co.uk wrote: Show quoted text

> We’re trying to update our application to a more recent version of > DBIx::Class (from 0.08196) and this is changed behaviour,

Hi! This ticket got dropped on the floor, sorry about that :( Looking over it again it seems that what you described never worked on 0.08196 in the first place. Could you please check this self-contained example, and tell me how it differs from what you have in production? I am definitely missing something... Thanks! rabbit@Ahasver:~$ cpanm --look DBIx::Class@0.08196 --> Working on DBIx::Class Fetching http://www.cpan.org/authors/id/A/AR/ARODLAND/DBIx-Class-0.08196.tar.gz ... OK Entering /home/rabbit/.cpanm/work/1455023884.3729/DBIx-Class-0.08196 with /bin/bash rabbit@Ahasver:~/.cpanm/work/1455023884.3729/DBIx-Class-0.08196$ DBICTEST_NO_MAKEFILE_VERIFICATION=1 perl -I lib -I t/lib -MDBICTest -MDevel::Dwarn -e ' warn DBIx::Class->VERSION; Dwarn [ DBICTest->init_schema->resultset("Artist")->search({}, { join => "cds", group_by => "me.name", "+columns" => { "cds.count" => "count(cds.cdid)" } })->all ] ' 0.08196 at -e line 2. DBIx::Class::ResultSet::all(): Manual prefetch (via select/columns) not supported with accessor 'multi' at -e line 4

Thu Feb 11 10:04:54 2016 tony.langdon [...] wcn.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #108098] Issue with alias names in +as with group by - 0.082810
Date:	Thu, 11 Feb 2016 15:04:37 +0000
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Tony Langdon <tony.langdon [...] wcn.co.uk>

I agree that DBIx::Class doesn’t like the query with 0.08196 and reports a Manual prefetch error if we run the resultset as is. However in our code we don’t do that. 1. We use as_query to get the select statement and binds 2. We match the query against a regex m!^$SELECT (.+?) FROM (.+)$$! to get the column & table list 3. We wrap the query in an ‘insert into temptable’ which we run through DBI::do for paging / handling complicated many column selects On 0.08196 the SQL generated was valid (even though DBIx wouldn’t run it), with 0.082810 the subquery it is generating breaks our regex because of the subquery it adds. 0.08196 for my second query (with ‘.’ in the +as names) (SELECT me.name, count(the_wall_cds.cdid) FROM artist me LEFT JOIN cd the_wall_cds ON ( the_wall_cds.artistid = me.artistid AND the_wall_cds.title = ? ) GROUP BY me.name) 0.082810 (SELECT me.name, count(the_wall_cds.cdid) FROM (SELECT me.name, count(the_wall_cds.cdid), me.artistid FROM artist me LEFT JOIN cd the_wall_cds ON ( the_wall_cds.artistid = me.artistid AND the_wall_cds.title = ? ) GROUP BY me.name) me LEFT JOIN cd the_wall_cds ON ( the_wall_cds.artistid = me.artistid AND the_wall_cds.title = ? ) I know that this is unsupported behaviour, the query was invalid before and is still invalid, but it does seem to display an issue in DBIx::Class. The sub query in 0.082810 is unnecessary, the logic in DBIx::Class::Storage::DBI _select_args (~2479) seems to have the opposite logic to the comment. # no aliases other than our own in group_by # if there are - do not allow subquery even if limit is present # TL - Patch for subquery # $prefetch_needs_subquery = ! scalar grep { $_ ne $attrs->{alias} } keys %{ $grp_aliases->{grouping} || {} }; $prefetch_needs_subquery = scalar grep { $_ ne $attrs->{alias} } keys %{ $grp_aliases->{grouping} || {} }; Show quoted text

> On 9 Feb 2016, at 13:19, Peter Rabbitson via RT <bug-DBIx-Class@rt.cpan.org> wrote: > > <URL: https://rt.cpan.org/Ticket/Display.html?id=108098 > > > On Thu Oct 29 19:06:02 2015, tony.langdon@wcn.co.uk wrote:

>> We’re trying to update our application to a more recent version of >> DBIx::Class (from 0.08196) and this is changed behaviour,

> > > Hi! This ticket got dropped on the floor, sorry about that :( > > Looking over it again it seems that what you described never worked on 0.08196 in the first place. Could you please check this self-contained example, and tell me how it differs from what you have in production? > > I am definitely missing something... > > Thanks! > > > rabbit@Ahasver:~$ cpanm --look DBIx::Class@0.08196 > --> Working on DBIx::Class > Fetching http://www.cpan.org/authors/id/A/AR/ARODLAND/DBIx-Class-0.08196.tar.gz ... OK > Entering /home/rabbit/.cpanm/work/1455023884.3729/DBIx-Class-0.08196 with /bin/bash > > rabbit@Ahasver:~/.cpanm/work/1455023884.3729/DBIx-Class-0.08196$ DBICTEST_NO_MAKEFILE_VERIFICATION=1 perl -I lib -I t/lib -MDBICTest -MDevel::Dwarn -e ' > warn DBIx::Class->VERSION; > > Dwarn [ DBICTest->init_schema->resultset("Artist")->search({}, { > join => "cds", > group_by => "me.name", > "+columns" => { > "cds.count" => "count(cds.cdid)" > } > })->all ] > ' > 0.08196 at -e line 2. > DBIx::Class::ResultSet::all(): Manual prefetch (via select/columns) not supported with accessor 'multi' at -e line 4 >

Thu Feb 11 11:10:13 2016 ribasushi [...] leporine.io - Correspondence added

On Thu Feb 11 16:04:54 2016, tony.langdon@wcn.co.uk wrote: Show quoted text

> I agree that DBIx::Class doesn’t like the query with 0.08196 and > reports a Manual prefetch error if we run the resultset as is. > > However in our code we don’t do that. > 1. We use as_query to get the select statement and binds

Nothing wrong with that. Show quoted text

> 2. We match the query against a regex m!^$SELECT (.+?) FROM (.+)$$! > to get the column & table list > 3. We wrap the query in an ‘insert into temptable’ which we run > through DBI::do for paging / handling complicated many column selects

Cringe, but I can see why you are doing it this way ;) Show quoted text

> I know that this is unsupported behaviour

I wouldn't go that far. What you are failing to recognize is that you are asking DBIC to do something it could not do before 0.08250, precisely because it was producing the query that happened to work for you. In order to do what you want all you need is to replace "+columns" => { "cds.count" => "count(cds.cdid)" } with "+columns" => { "cds_count" => "count(cds.cdid)" } With the former invocation you are saying "I will retrieve some column and hang it off the *1:M* related slot". So DBIC gets into a mode where it needs to isolate the group_by, so that the "1:M" does not get collapsed. With the latter syntax you are saying "everything that comes back - just shove it into the main object and don't bother doing any related object resolution". Which ends up producing what you want. (SELECT me.artistid, me.name, me.rank, me.charfield, count(cds.cdid) FROM artist me LEFT JOIN cd cds ON cds.artist = me.artistid GROUP BY me.name) Show quoted text

> DBIx::Class::Storage::DBI _select_args (~2479) seems to have the > opposite logic to the comment.

It isn't opposite. If you are curious: simply edit it (by removing the !) and run the test suite (prove -lr) to see what will blow up. Let me know if this solves the problem for you.

Thu Feb 11 11:27:07 2016 tony.langdon [...] wcn.co.uk - Correspondence added

Subject:	Re: [rt.cpan.org #108098] Issue with alias names in +as with group by - 0.082810
Date:	Thu, 11 Feb 2016 16:26:44 +0000
To:	bug-DBIx-Class [...] rt.cpan.org
From:	Tony Langdon <tony.langdon [...] wcn.co.uk>

I’ll look at the column names suggestion as it’s better than our alternate patch which manipulates the internal structure to remove the +as with group_by: my $rs2 = $shema->resultset(‘SomeClass’)->search({…}) if (exists $rs2->{'attrs'}{'group_by'}) { delete $rs2->{'attrs'}{'+as'}; } $rs2->as_sql…. I’ve been running the tweaked DBIx::Class::Storage::DBI for a few months now and it seemed OK with our code. However if it breaks the DBIx::Class tests it must be breaking something that I haven’t noticed yet. Show quoted text

> On 11 Feb 2016, at 16:10, Peter Rabbitson via RT <bug-DBIx-Class@rt.cpan.org> wrote: > > <URL: https://rt.cpan.org/Ticket/Display.html?id=108098 > > > On Thu Feb 11 16:04:54 2016, tony.langdon@wcn.co.uk wrote:

>> I agree that DBIx::Class doesn’t like the query with 0.08196 and >> reports a Manual prefetch error if we run the resultset as is. >> >> However in our code we don’t do that. >> 1. We use as_query to get the select statement and binds

> > Nothing wrong with that. >

>> 2. We match the query against a regex m!^$SELECT (.+?) FROM (.+)$$! >> to get the column & table list >> 3. We wrap the query in an ‘insert into temptable’ which we run >> through DBI::do for paging / handling complicated many column selects

> > Cringe, but I can see why you are doing it this way ;) >

>> I know that this is unsupported behaviour

> > I wouldn't go that far. What you are failing to recognize is that you are asking DBIC to do something it could not do before 0.08250, precisely because it was producing the query that happened to work for you. > > In order to do what you want all you need is to replace > > "+columns" => { "cds.count" => "count(cds.cdid)" } > > with > > "+columns" => { "cds_count" => "count(cds.cdid)" } > > With the former invocation you are saying "I will retrieve some column and hang it off the *1:M* related slot". So DBIC gets into a mode where it needs to isolate the group_by, so that the "1:M" does not get collapsed. > > With the latter syntax you are saying "everything that comes back - just shove it into the main object and don't bother doing any related object resolution". Which ends up producing what you want. > > (SELECT me.artistid, me.name, me.rank, me.charfield, count(cds.cdid) FROM artist me LEFT JOIN cd cds ON cds.artist = me.artistid GROUP BY me.name) >

>> DBIx::Class::Storage::DBI _select_args (~2479) seems to have the >> opposite logic to the comment.

> > It isn't opposite. If you are curious: simply edit it (by removing the !) and run the test suite (prove -lr) to see what will blow up. > > Let me know if this solves the problem for you.

Thu Feb 11 13:33:14 2016 ribasushi [...] leporine.io - Correspondence added

On Thu Feb 11 17:27:07 2016, tony.langdon@wcn.co.uk wrote: Show quoted text

> > I’ve been running the tweaked DBIx::Class::Storage::DBI for a few > months now and it seemed OK with our code. However if it breaks the > DBIx::Class tests it must be breaking something that I haven’t noticed > yet.

To be honest it is kinda strange to me that you modified the library *without* executing its comprehensive test suite to see whether it will affect anything ;) In any case - please let me know when you had a chance to try, so I can scratch this off the todo. Cheers!

Wed Mar 30 14:33:12 2016 ribasushi [...] leporine.io - Correspondence added

No response - closing. You are very much encouraged to reopen this if the problem is not fully resolved. Cheers!

Wed Mar 30 14:33:20 2016 ribasushi [...] leporine.io - Status changed from 'open' to 'resolved'

Bug #108098 for DBIx-Class: Issue with alias names in +as with group by - 0.082810