From: Norbert Buchmuller Date: Fri, 22 Oct 2010 12:21:28 +0000 (+0200) Subject: Fixed a prefetch bug (o2m->prefetch_o2m+order_by+rows) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=0a3441ee8e0e747cfa05eff02df0d918ed5d6acb;p=dbsrgits%2FDBIx-Class-Historic.git Fixed a prefetch bug (o2m->prefetch_o2m+order_by+rows) --- diff --git a/Changes b/Changes index 78a2a38..8bdefc4 100644 --- a/Changes +++ b/Changes @@ -45,6 +45,8 @@ Revision history for DBIx::Class -or condition - Remove rogue GROUP BY on non-multiplying prefetch-induced subqueries + - Fix incorrect order_by handling with prefetch on + $ordered_rs->search_related ('has_many_rel') resultsets - Oracle sequence detection now *really* works across schemas (fixed some ommissions from 0.08123) - dbicadmin now uses a /usr/bin/env shebang to work better with diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 6dcbbf9..47bd139 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -3121,34 +3121,9 @@ sub _resolved_attrs { carp ("Useless use of distinct on a grouped resultset ('distinct' is ignored when a 'group_by' is present)"); } else { - my $storage = $self->result_source->schema->storage; - my $rs_column_list = $storage->_resolve_column_info ($attrs->{from}); - - my $group_spec = $attrs->{group_by} = []; - my %group_index; - - for (@{$attrs->{select}}) { - if (! ref($_) or ref ($_) ne 'HASH' ) { - push @$group_spec, $_; - $group_index{$_}++; - if ($rs_column_list->{$_} and $_ !~ /\./ ) { - # add a fully qualified version as well - $group_index{"$rs_column_list->{$_}{-source_alias}.$_"}++; - } - } - } - # add any order_by parts that are not already present in the group_by - # we need to be careful not to add any named functions/aggregates - # i.e. select => [ ... { count => 'foo', -as 'foocount' } ... ] - for my $chunk ($storage->_extract_order_columns($attrs->{order_by})) { - - # only consider real columns (for functions the user got to do an explicit group_by) - my $colinfo = $rs_column_list->{$chunk} - or next; - - $chunk = "$colinfo->{-source_alias}.$chunk" if $chunk !~ /\./; - push @$group_spec, $chunk unless $group_index{$chunk}++; - } + $attrs->{group_by} = $source->storage->_group_over_selection ( + @{$attrs}{qw/from select order_by/} + ); } } diff --git a/lib/DBIx/Class/Storage/DBI/Replicated.pm b/lib/DBIx/Class/Storage/DBI/Replicated.pm index 7fff40e..967d0e0 100644 --- a/lib/DBIx/Class/Storage/DBI/Replicated.pm +++ b/lib/DBIx/Class/Storage/DBI/Replicated.pm @@ -381,6 +381,7 @@ my @unimplemented = qw( sql_limit_dialect _inner_join_to_node + _group_over_selection ); # the capability framework diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index 240fa3c..f5a03c9 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -117,8 +117,15 @@ sub _adjust_select_args_for_complex_prefetch { # if a multi-type join was needed in the subquery - add a group_by to simulate the # collapse in the subq - $inner_attrs->{group_by} ||= $inner_select - if first { ! $_->[0]{-is_single} } (@{$inner_from}[1 .. $#$inner_from]); + if ( + ! $inner_attrs->{group_by} + and + first { ! $_->[0]{-is_single} } (@{$inner_from}[1 .. $#$inner_from]) + ) { + $inner_attrs->{group_by} = $self->_group_over_selection ( + $inner_from, $inner_select, $inner_attrs->{order_by} + ); + } # we already optimized $inner_from above local $self->{_use_join_optimizer} = 0; @@ -327,6 +334,39 @@ sub _resolve_aliastypes_from_select_args { return $aliases_by_type; } +sub _group_over_selection { + my ($self, $from, $select, $order_by) = @_; + + my $rs_column_list = $self->_resolve_column_info ($from); + + my (@group_by, %group_index); + + for (@$select) { + if (! ref($_) or ref ($_) ne 'HASH' ) { + push @group_by, $_; + $group_index{$_}++; + if ($rs_column_list->{$_} and $_ !~ /\./ ) { + # add a fully qualified version as well + $group_index{"$rs_column_list->{$_}{-source_alias}.$_"}++; + } + } + } + + # add any order_by parts that are not already present in the group_by + # we need to be careful not to add any named functions/aggregates + # i.e. select => [ ... { count => 'foo', -as 'foocount' } ... ] + for my $chunk ($self->_extract_order_columns($order_by)) { + # only consider real columns (for functions the user got to do an explicit group_by) + my $colinfo = $rs_column_list->{$chunk} + or next; + + $chunk = "$colinfo->{-source_alias}.$chunk" if $chunk !~ /\./; + push @group_by, $chunk unless $group_index{$chunk}++; + } + + return \@group_by; +} + sub _resolve_ident_sources { my ($self, $ident) = @_; diff --git a/t/prefetch/o2m_o2m_order_by_with_limit.t b/t/prefetch/o2m_o2m_order_by_with_limit.t new file mode 100644 index 0000000..c593379 --- /dev/null +++ b/t/prefetch/o2m_o2m_order_by_with_limit.t @@ -0,0 +1,140 @@ +use strict; +use warnings; + +use Test::More; + +use lib qw(t/lib); +use DBIC::SqlMakerTest; +use DBICTest; + +my $schema = DBICTest->init_schema(); + +my $artist_rs = $schema->resultset('Artist'); +my $ar = $artist_rs->current_source_alias; + +my $filtered_cd_rs = $artist_rs->search_related('cds_unordered', + { "$ar.rank" => 13 }, + { + prefetch => [ 'tracks' ], + order_by => [ { -asc => "$ar.name" }, "$ar.artistid DESC" ], + offset => 3, + rows => 3, + }, +); + +is_same_sql_bind( + $filtered_cd_rs->as_query, + q{( + SELECT cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track, + tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at, tracks.small_dt + FROM artist me + JOIN ( + SELECT cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track + FROM artist me + JOIN cd cds_unordered + ON cds_unordered.artist = me.artistid + WHERE ( me.rank = ? ) + GROUP BY cds_unordered.cdid, cds_unordered.artist, cds_unordered.title, cds_unordered.year, cds_unordered.genreid, cds_unordered.single_track, me.name, me.artistid + ORDER BY me.name ASC, me.artistid DESC + LIMIT 3 + OFFSET 3 + ) cds_unordered + ON cds_unordered.artist = me.artistid + LEFT JOIN track tracks + ON tracks.cd = cds_unordered.cdid + WHERE ( me.rank = ? ) + ORDER BY me.name ASC, me.artistid DESC, tracks.cd + )}, + [ [ 'me.rank' => 13 ], [ 'me.rank' => 13 ] ], + 'correct SQL on limited prefetch over search_related ordered by root', +); + +# note: we only requested "get all cds of all artists with rank 13 then order +# by the artist name and give me the fourth, fifth and sixth", consequently the +# cds that belong to the same artist are unordered; fortunately we know that +# the first artist have 3 cds and the second and third artist both have only +# one, so the first 3 cds belong to the first artist and the fourth and fifth +# cds belong to the second and third artist, respectively, and there's no sixth +# row +is_deeply ( + [ $filtered_cd_rs->hri_dump ], + [ + { + 'artist' => '2', + 'cdid' => '4', + 'genreid' => undef, + 'single_track' => undef, + 'title' => 'Generic Manufactured Singles', + 'tracks' => [ + { + 'cd' => '4', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '1', + 'small_dt' => undef, + 'title' => 'Boring Name', + 'trackid' => '10' + }, + { + 'cd' => '4', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '2', + 'small_dt' => undef, + 'title' => 'Boring Song', + 'trackid' => '11' + }, + { + 'cd' => '4', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '3', + 'small_dt' => undef, + 'title' => 'No More Ideas', + 'trackid' => '12' + } + ], + 'year' => '2001' + }, + { + 'artist' => '3', + 'cdid' => '5', + 'genreid' => undef, + 'single_track' => undef, + 'title' => 'Come Be Depressed With Us', + 'tracks' => [ + { + 'cd' => '5', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '1', + 'small_dt' => undef, + 'title' => 'Sad', + 'trackid' => '13' + }, + { + 'cd' => '5', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '3', + 'small_dt' => undef, + 'title' => 'Suicidal', + 'trackid' => '15' + }, + { + 'cd' => '5', + 'last_updated_at' => undef, + 'last_updated_on' => undef, + 'position' => '2', + 'small_dt' => undef, + 'title' => 'Under The Weather', + 'trackid' => '14' + } + ], + 'year' => '1998' + } + ], + 'Correctly ordered result', +); + +done_testing;