From: Peter Rabbitson Date: Wed, 5 Aug 2009 06:37:48 +0000 (+0000) Subject: Merge 'trunk' into 'mssql_storage_minor_refactor' X-Git-Tag: v0.08109~47^2~1 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=086901c30dcd1c87b678c44bfd1d42ace0b5e18f;hp=8ab3290b4c6bdb3c9c84172afca234169184ff06;p=dbsrgits%2FDBIx-Class.git Merge 'trunk' into 'mssql_storage_minor_refactor' r7208@Thesaurus (orig r7205): ribasushi | 2009-08-05 08:34:25 +0200 Bump dependencies: Test::More for the new no_plan/done_testing goodies File::Temp as per RT#48431 r7210@Thesaurus (orig r7207): ribasushi | 2009-08-05 08:36:32 +0200 r7156@Thesaurus (orig r7153): robkinyon | 2009-07-30 20:06:04 +0200 Create prefetch_redux branch r7164@Thesaurus (orig r7161): robkinyon | 2009-07-31 22:41:01 +0200 Added MooseX::Traits to Makefile.PL r7172@Thesaurus (orig r7169): robkinyon | 2009-08-03 05:49:59 +0200 Added two tests and marked one todo_skip r7187@Thesaurus (orig r7184): ribasushi | 2009-08-03 17:24:41 +0200 Use goto to preserve correct error-at-line reporting r7189@Thesaurus (orig r7186): ribasushi | 2009-08-04 12:34:58 +0200 Add an extra test specifically for distinct/prefetch Remove duplicate test in count/prefetch Switch to as_query instead of debug overloading r7190@Thesaurus (orig r7187): ribasushi | 2009-08-04 12:35:57 +0200 Fix how a distinct-induced group_by is calculated, taking in consideration the new prefetch mechanism r7197@Thesaurus (orig r7194): ribasushi | 2009-08-04 17:31:33 +0200 Traits not needed by anything currently in dbic r7198@Thesaurus (orig r7195): ribasushi | 2009-08-04 17:41:14 +0200 Move around tests a bit r7199@Thesaurus (orig r7196): mo | 2009-08-04 21:10:57 +0200 prefetch-grouped fails, again r7204@Thesaurus (orig r7201): ribasushi | 2009-08-04 22:50:51 +0200 Split the search_related prefetch tests into a standalone testfile r7205@Thesaurus (orig r7202): ribasushi | 2009-08-04 23:05:03 +0200 Move norbi's test to prefetch_redux - it's the same idea r7209@Thesaurus (orig r7206): ribasushi | 2009-08-05 08:35:48 +0200 Tadaaaa (even more prefetch insanity) --- diff --git a/Makefile.PL b/Makefile.PL index d21951b..79da35f 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -13,9 +13,11 @@ all_from 'lib/DBIx/Class.pm'; test_requires 'Test::Builder' => 0.33; test_requires 'Test::Deep' => 0; test_requires 'Test::Exception' => 0; -test_requires 'Test::More' => 0.82; +test_requires 'Test::More' => 0.92; test_requires 'Test::Warn' => 0.11; +test_requires 'File::Temp' => 0.22; + # Core requires 'List::Util' => 0; requires 'Scalar::Util' => 0; diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 9fba9fc..e24dafe 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -1264,7 +1264,7 @@ sub _count_subq_rs { my $sub_attrs = { %$attrs }; # extra selectors do not go in the subquery and there is no point of ordering it - delete $sub_attrs->{$_} for qw/collapse prefetch_select select as order_by/; + delete $sub_attrs->{$_} for qw/collapse select _prefetch_select as order_by/; # if we prefetch, we group_by primary keys only as this is what we would get out of the rs via ->next/->all # clobber old group_by regardless @@ -2875,6 +2875,12 @@ sub _resolved_attrs { $attrs->{group_by} = [ $attrs->{group_by} ]; } + # generate the distinct induced group_by early, as prefetch will be carried via a + # subquery (since a group_by is present) + if (delete $attrs->{distinct}) { + $attrs->{group_by} ||= [ grep { !ref($_) || (ref($_) ne 'HASH') } @{$attrs->{select}} ]; + } + $attrs->{collapse} ||= {}; if ( my $prefetch = delete $attrs->{prefetch} ) { $prefetch = $self->_merge_attr( {}, $prefetch ); @@ -2886,19 +2892,16 @@ sub _resolved_attrs { my @prefetch = $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $attrs->{collapse} ); - $attrs->{prefetch_select} = [ map { $_->[0] } @prefetch ]; - push @{ $attrs->{select} }, @{$attrs->{prefetch_select}}; + # we need to somehow mark which columns came from prefetch + $attrs->{_prefetch_select} = [ map { $_->[0] } @prefetch ]; + + push @{ $attrs->{select} }, @{$attrs->{_prefetch_select}}; push @{ $attrs->{as} }, (map { $_->[1] } @prefetch); push( @{$attrs->{order_by}}, @$prefetch_ordering ); $attrs->{_collapse_order_by} = \@$prefetch_ordering; } - - if (delete $attrs->{distinct}) { - $attrs->{group_by} ||= [ grep { !ref($_) || (ref($_) ne 'HASH') } @{$attrs->{select}} ]; - } - # if both page and offset are specified, produce a combined offset # even though it doesn't make much sense, this is what pre 081xx has # been doing diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 1dd8d8d..84969f6 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -1452,7 +1452,7 @@ sub _select_args { ( $attrs->{rows} && keys %{$attrs->{collapse}} ) || ( $attrs->{group_by} && @{$attrs->{group_by}} && - $attrs->{prefetch_select} && @{$attrs->{prefetch_select}} ) + $attrs->{_prefetch_select} && @{$attrs->{_prefetch_select}} ) ) { ($ident, $select, $where, $attrs) = $self->_adjust_select_args_for_complex_prefetch ($ident, $select, $where, $attrs); @@ -1497,14 +1497,15 @@ sub _adjust_select_args_for_complex_prefetch { # separate attributes my $sub_attrs = { %$attrs }; delete $attrs->{$_} for qw/where bind rows offset group_by having/; - delete $sub_attrs->{$_} for qw/for collapse prefetch_select _collapse_order_by select as/; + delete $sub_attrs->{$_} for qw/for collapse _prefetch_select _collapse_order_by select as/; - my $alias = $attrs->{alias}; + my $select_root_alias = $attrs->{alias}; my $sql_maker = $self->sql_maker; # create subquery select list - consider only stuff *not* brought in by the prefetch my $sub_select = []; - for my $i (0 .. @{$attrs->{select}} - @{$attrs->{prefetch_select}} - 1) { + my $sub_group_by; + for my $i (0 .. @{$attrs->{select}} - @{$attrs->{_prefetch_select}} - 1) { my $sel = $attrs->{select}[$i]; # alias any functions to the dbic-side 'as' label @@ -1526,29 +1527,15 @@ sub _adjust_select_args_for_complex_prefetch { ]; } - # mangle {from} + # mangle {from}, keep in mind that $from is "headless" from here on my $join_root = shift @$from; - my @outer_from = @$from; my %inner_joins; my %join_info = map { $_->[0]{-alias} => $_->[0] } (@$from); - # in complex search_related chains $alias may *not* be 'me' - # so always include it in the inner join, and also shift away - # from the outer stack, so that the two datasets actually do - # meet - if ($join_root->{-alias} ne $alias) { - $inner_joins{$alias} = 1; - - while (@outer_from && $outer_from[0][0]{-alias} ne $alias) { - shift @outer_from; - } - if (! @outer_from) { - $self->throw_exception ("Unable to find '$alias' in the {from} stack, something is wrong"); - } - - shift @outer_from; # the new subquery will represent this alias, so get rid of it - } + # in complex search_related chains $select_root_alias may *not* be + # 'me' so always include it in the inner join + $inner_joins{$select_root_alias} = 1 if ($join_root->{-alias} ne $select_root_alias); # decide which parts of the join will remain on the inside @@ -1617,13 +1604,15 @@ sub _adjust_select_args_for_complex_prefetch { # if a multi-type join was needed in the subquery ("multi" is indicated by # presence in {collapse}) - add a group_by to simulate the collapse in the subq - for my $alias (keys %inner_joins) { - - # the dot comes from some weirdness in collapse - # remove after the rewrite - if ($attrs->{collapse}{".$alias"}) { - $sub_attrs->{group_by} ||= $sub_select; - last; + unless ($sub_attrs->{group_by}) { + for my $alias (keys %inner_joins) { + + # the dot comes from some weirdness in collapse + # remove after the rewrite + if ($attrs->{collapse}{".$alias"}) { + $sub_attrs->{group_by} ||= $sub_select; + last; + } } } @@ -1634,14 +1623,42 @@ sub _adjust_select_args_for_complex_prefetch { $where, $sub_attrs ); - - # put it in the new {from} - unshift @outer_from, { - -alias => $alias, + my $subq_joinspec = { + -alias => $select_root_alias, -source_handle => $join_root->{-source_handle}, - $alias => $subq, + $select_root_alias => $subq, }; + # Generate a new from (really just replace the join slot with the subquery) + # Before we would start the outer chain from the subquery itself (i.e. + # SELECT ... FROM (SELECT ... ) alias JOIN ..., but this turned out to be + # a bad idea for search_related, as the root of the chain was effectively + # lost (i.e. $artist_rs->search_related ('cds'... ) would result in alias + # of 'cds', which would prevent from doing things like order_by artist.*) + # See t/prefetch/via_search_related.t for a better idea + my @outer_from; + if ($join_root->{-alias} eq $select_root_alias) { # just swap the root part and we're done + @outer_from = ( + $subq_joinspec, + @$from, + ) + } + else { # this is trickier + @outer_from = ($join_root); + + for my $j (@$from) { + if ($j->[0]{-alias} eq $select_root_alias) { + push @outer_from, [ + $subq_joinspec, + @{$j}[1 .. $#$j], + ]; + } + else { + push @outer_from, $j; + } + } + } + # This is totally horrific - the $where ends up in both the inner and outer query # Unfortunately not much can be done until SQLA2 introspection arrives, and even # then if where conditions apply to the *right* side of the prefetch, you may have diff --git a/t/count/prefetch.t b/t/count/prefetch.t index 2032a4b..8012e10 100644 --- a/t/count/prefetch.t +++ b/t/count/prefetch.t @@ -6,9 +6,6 @@ use lib qw(t/lib); use Test::More; use DBICTest; use DBIC::SqlMakerTest; -use DBIC::DebugObj; - -plan tests => 6; my $schema = DBICTest->init_schema(); @@ -20,20 +17,58 @@ my $schema = DBICTest->init_schema(); { prefetch => [qw/tracks artist/] }, ); is ($rs->all, 5, 'Correct number of objects'); + is ($rs->count, 5, 'Correct count'); + is_same_sql_bind ( + $rs->count_rs->as_query, + '( + SELECT COUNT( * ) + FROM ( + SELECT cds.cdid + FROM artist me + JOIN cd cds ON cds.artist = me.artistid + LEFT JOIN track tracks ON tracks.cd = cds.cdid + JOIN artist artist ON artist.artistid = cds.artist + WHERE tracks.position = ? OR tracks.position = ? + GROUP BY cds.cdid + ) count_subq + )', + [ map { [ 'tracks.position' => $_ ] } (1, 2) ], + ); +} - my ($sql, @bind); - $schema->storage->debugobj(DBIC::DebugObj->new(\$sql, \@bind)); - $schema->storage->debug(1); - - - is ($rs->count, 5, 'Correct count'); +# collapsing prefetch with distinct +{ + my $first_cd = $schema->resultset('Artist')->first->cds->first; + $first_cd->update ({ + genreid => $first_cd->create_related ( + genre => ({ name => 'vague genre' }) + )->id + }); + + my $rs = $schema->resultset("Artist")->search(undef, {distinct => 1}) + ->search_related('cds')->search_related('genre', + { 'genre.name' => { '!=', 'foo' } }, + { prefetch => q(cds) }, + ); + is ($rs->all, 1, 'Correct number of objects'); + is ($rs->count, 1, 'Correct count'); is_same_sql_bind ( - $sql, - \@bind, - 'SELECT COUNT( * ) FROM (SELECT cds.cdid FROM artist me JOIN cd cds ON cds.artist = me.artistid LEFT JOIN track tracks ON tracks.cd = cds.cdid JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid) count_subq', - [ qw/'1' '2'/ ], + $rs->count_rs->as_query, + '( + SELECT COUNT( * ) + FROM ( + SELECT genre.genreid + FROM artist me + JOIN cd cds ON cds.artist = me.artistid + JOIN genre genre ON genre.genreid = cds.genreid + LEFT JOIN cd cds_2 ON cds_2.genreid = genre.genreid + WHERE ( genre.name != ? ) + GROUP BY genre.genreid + ) count_subq + )', + [ [ 'genre.name' => 'foo' ] ], ); } @@ -47,17 +82,20 @@ my $schema = DBICTest->init_schema(); is ($rs->all, 10, 'Correct number of objects'); - my ($sql, @bind); - $schema->storage->debugobj(DBIC::DebugObj->new(\$sql, \@bind)); - $schema->storage->debug(1); - - is ($rs->count, 10, 'Correct count'); is_same_sql_bind ( - $sql, - \@bind, - 'SELECT COUNT( * ) FROM cd me JOIN track tracks ON tracks.cd = me.cdid JOIN cd disc ON disc.cdid = tracks.cd LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid WHERE ( ( position = ? OR position = ? ) )', - [ qw/'1' '2'/ ], + $rs->count_rs->as_query, + '( + SELECT COUNT( * ) + FROM cd me + JOIN track tracks ON tracks.cd = me.cdid + JOIN cd disc ON disc.cdid = tracks.cd + LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid + WHERE position = ? OR position = ? + )', + [ map { [ position => $_ ] } (1, 2) ], ); } + +done_testing; diff --git a/t/lib/DBIC/SqlMakerTest.pm b/t/lib/DBIC/SqlMakerTest.pm index 1098e25..44ccb4b 100644 --- a/t/lib/DBIC/SqlMakerTest.pm +++ b/t/lib/DBIC/SqlMakerTest.pm @@ -41,7 +41,8 @@ sub is_same_sql_bind { croak "Unexpected argument(s) supplied to is_same_sql_bind: " . join ('; ', @_) if @_; - SQL::Abstract::Test::is_same_sql_bind (@args); + @_ = @args; + goto &SQL::Abstract::Test::is_same_sql_bind; } *is_same_sql = \&SQL::Abstract::Test::is_same_sql; diff --git a/t/prefetch/grouped.t b/t/prefetch/grouped.t index 0a61f97..92f383c 100644 --- a/t/prefetch/grouped.t +++ b/t/prefetch/grouped.t @@ -1,14 +1,13 @@ use strict; use warnings; + use Test::More; +use Test::Exception; use lib qw(t/lib); use DBICTest; use DBIC::SqlMakerTest; -#plan tests => 6; -plan 'no_plan'; - my $schema = DBICTest->init_schema(); my $sdebug = $schema->storage->debug; @@ -205,3 +204,35 @@ for ($cd_rs->all) { $schema->storage->debugcb (undef); $schema->storage->debug ($sdebug); } + +# make sure that distinct still works +{ + my $rs = $schema->resultset("CD")->search({}, { + prefetch => 'tags', + order_by => 'cdid', + distinct => 1, + }); + + is_same_sql_bind ( + $rs->as_query, + '( + SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track, + tags.tagid, tags.cd, tags.tag + FROM ( + SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track + FROM cd me + GROUP BY me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track + ORDER BY cdid + ) me + LEFT JOIN tags tags ON tags.cd = me.cdid + ORDER BY cdid, tags.cd, tags.tag + )', + [], + 'Prefetch + distinct resulted in correct group_by', + ); + + is ($rs->all, 5, 'Correct number of CD objects'); + is ($rs->count, 5, 'Correct count of CDs'); +} + +done_testing; diff --git a/t/prefetch/via_search_related.t b/t/prefetch/via_search_related.t new file mode 100644 index 0000000..041c341 --- /dev/null +++ b/t/prefetch/via_search_related.t @@ -0,0 +1,93 @@ +use strict; +use warnings; + +use Test::More; +use Test::Exception; + +use lib qw(t/lib); +use DBICTest; + +my $schema = DBICTest->init_schema(); + +lives_ok ( sub { + my $no_prefetch = $schema->resultset('Track')->search_related(cd => + { + 'cd.year' => "2000", + }, + { + join => 'tags', + order_by => 'me.trackid', + rows => 1, + } + ); + + my $use_prefetch = $no_prefetch->search( + {}, + { + prefetch => 'tags', + } + ); + + is($use_prefetch->count, $no_prefetch->count, 'counts with and without prefetch match'); + is( + scalar ($use_prefetch->all), + scalar ($no_prefetch->all), + "Amount of returned rows is right" + ); + +}, 'search_related prefetch with order_by works'); + + +lives_ok (sub { + my $rs = $schema->resultset("Artwork")->search(undef, {distinct => 1}) + ->search_related('artwork_to_artist')->search_related('artist', + undef, + { prefetch => 'cds' }, + ); + is($rs->all, 0, 'prefetch without WHERE (objects)'); + is($rs->count, 0, 'prefetch without WHERE (count)'); + + $rs = $schema->resultset("Artwork")->search(undef, {distinct => 1}) + ->search_related('artwork_to_artist')->search_related('artist', + { 'cds.title' => 'foo' }, + { prefetch => 'cds' }, + ); + is($rs->all, 0, 'prefetch with WHERE (objects)'); + is($rs->count, 0, 'prefetch with WHERE (count)'); + + +# test where conditions at the root of the related chain + my $artist_rs = $schema->resultset("Artist")->search({artistid => 11}); + + + $rs = $artist_rs->search_related('cds')->search_related('genre', + { 'genre.name' => 'foo' }, + { prefetch => 'cds' }, + ); + is($rs->all, 0, 'prefetch without distinct (objects)'); + is($rs->count, 0, 'prefetch without distinct (count)'); + + + + $rs = $artist_rs->search(undef, {distinct => 1}) + ->search_related('cds')->search_related('genre', + { 'genre.name' => 'foo' }, + ); + is($rs->all, 0, 'distinct without prefetch (objects)'); + is($rs->count, 0, 'distinct without prefetch (count)'); + + + + $rs = $artist_rs->search({}, {distinct => 1}) + ->search_related('cds')->search_related('genre', + { 'genre.name' => 'foo' }, + { prefetch => 'cds' }, + ); + is($rs->all, 0, 'distinct with prefetch (objects)'); + is($rs->count, 0, 'distinct with prefetch (count)'); + + + +}, 'distinct generally works with prefetch on deep search_related chains'); + +done_testing;