From: Peter Rabbitson Date: Tue, 4 May 2010 08:00:11 +0000 (+0000) Subject: Refactor count handling, make count-resultset attribute lists inclusive rather than... X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=336feb8ef85d16d22ae0b131b0c7a85abc9e9435;p=dbsrgits%2FDBIx-Class-Historic.git Refactor count handling, make count-resultset attribute lists inclusive rather than exclusive (side effect - solves RT#56257 --- diff --git a/lib/DBIx/Class/Manual/Cookbook.pod b/lib/DBIx/Class/Manual/Cookbook.pod index 590e439..e31245f 100644 --- a/lib/DBIx/Class/Manual/Cookbook.pod +++ b/lib/DBIx/Class/Manual/Cookbook.pod @@ -292,7 +292,7 @@ See also L. my $count = $rs->count; # Equivalent SQL: - # SELECT COUNT( * ) FROM (SELECT me.name FROM artist me GROUP BY me.name) count_subq: + # SELECT COUNT( * ) FROM (SELECT me.name FROM artist me GROUP BY me.name) me: =head2 Grouping results diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index a360d3e..3805812 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -1235,10 +1235,11 @@ sub _count_rs { my $rsrc = $self->result_source; $attrs ||= $self->_resolved_attrs; - my $tmp_attrs = { %$attrs }; - - # take off any limits, record_filter is cdbi, and no point of ordering a count - delete $tmp_attrs->{$_} for (qw/select as rows offset order_by record_filter/); + # only take pieces we need for a simple count + my $tmp_attrs = { map + { $_ => $attrs->{$_} } + qw/ alias from where bind join / + }; # overwrite the selector (supplied by the storage) $tmp_attrs->{select} = $rsrc->storage->_count_select ($rsrc, $tmp_attrs); @@ -1256,12 +1257,12 @@ sub _count_subq_rs { my ($self, $attrs) = @_; my $rsrc = $self->result_source; - $attrs ||= $self->_resolved_attrs_copy; - - my $sub_attrs = { %$attrs }; + $attrs ||= $self->_resolved_attrs; - # extra selectors do not go in the subquery and there is no point of ordering it - delete $sub_attrs->{$_} for qw/collapse select _prefetch_select as order_by/; + my $sub_attrs = { map + { $_ => $attrs->{$_} } + qw/ alias from where bind join group_by having rows offset / + }; # if we multi-prefetch we group_by primary keys only as this is what we would # get out of the rs via ->next/->all. We *DO WANT* to clobber old group_by regardless @@ -1269,24 +1270,35 @@ sub _count_subq_rs { $sub_attrs->{group_by} = [ map { "$attrs->{alias}.$_" } ($rsrc->_pri_cols) ] } - $sub_attrs->{select} = $rsrc->storage->_subq_count_select ($rsrc, $attrs); + # Calculate subquery selector + if (my $g = $sub_attrs->{group_by}) { - # this is so that the query can be simplified e.g. - # * ordering can be thrown away in things like Top limit - $sub_attrs->{-for_count_only} = 1; + # necessary as the group_by may refer to aliased functions + my $sel_index; + for my $sel (@{$attrs->{select}}) { + $sel_index->{$sel->{-as}} = $sel + if (ref $sel eq 'HASH' and $sel->{-as}); + } - my $sub_rs = $rsrc->resultset_class->new ($rsrc, $sub_attrs); + for my $g_part (@$g) { + push @{$sub_attrs->{select}}, $sel_index->{$g_part} || $g_part; + } + } + else { + my @pcols = map { "$attrs->{alias}.$_" } ($rsrc->primary_columns); + $sub_attrs->{select} = @pcols ? \@pcols : [ 1 ]; + } - $attrs->{from} = [{ - -alias => 'count_subq', - -source_handle => $rsrc->handle, - count_subq => $sub_rs->as_query, - }]; - # the subquery replaces this - delete $attrs->{$_} for qw/where bind collapse group_by having having_bind rows offset/; + # this is so that the query can be simplified e.g. + # * ordering can be thrown away in things like Top limit + $sub_attrs->{-for_count_only} = 1; - return $self->_count_rs ($attrs); + return $rsrc->resultset_class + ->new ($rsrc, $sub_attrs) + ->as_subselect_rs + ->search ({}, { columns => { count => $rsrc->storage->_count_select ($rsrc, $attrs) } }) + -> get_column ('count'); } sub _bool { @@ -2668,15 +2680,18 @@ but because we isolated the group by into a subselect the above works. =cut sub as_subselect_rs { - my $self = shift; - - return $self->result_source->resultset->search( undef, { - alias => $self->current_source_alias, - from => [{ - $self->current_source_alias => $self->as_query, - -alias => $self->current_source_alias, - -source_handle => $self->result_source->handle, - }] + my $self = shift; + + my $attrs = $self->_resolved_attrs; + + return $self->result_source->resultset->search( undef, { + from => [{ + $attrs->{alias} => $self->as_query, + -alias => $attrs->{alias}, + -source_handle => $self->result_source->handle, + }], + map { $_ => $attrs->{$_} } qw/select as alias/ + }); } @@ -2702,7 +2717,7 @@ sub _chain_relationship { # ->_resolve_join as otherwise they get lost - captainL my $join = $self->_merge_attr( $attrs->{join}, $attrs->{prefetch} ); - delete @{$attrs}{qw/join prefetch collapse distinct select as columns +select +as +columns/}; + delete @{$attrs}{qw/join prefetch collapse group_by distinct select as columns +select +as +columns/}; my $seen = { %{ (delete $attrs->{seen_join}) || {} } }; @@ -2728,7 +2743,7 @@ sub _chain_relationship { -alias => $attrs->{alias}, $attrs->{alias} => $rs_copy->as_query, }]; - delete @{$attrs}{@force_subq_attrs, 'where'}; + delete @{$attrs}{@force_subq_attrs, qw/where bind/}; $seen->{-relation_chain_depth} = 0; } elsif ($attrs->{from}) { #shallow copy suffices diff --git a/lib/DBIx/Class/Storage/DBI.pm b/lib/DBIx/Class/Storage/DBI.pm index 1acd60e..4616b7b 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -2017,46 +2017,6 @@ sub _count_select { return { count => '*' }; } -# Returns a SELECT which will end up in the subselect -# There may or may not be a group_by, as the subquery -# might have been called to accomodate a limit -# -# Most databases would be happy with whatever ends up -# here, but some choke in various ways. -# -sub _subq_count_select { - my ($self, $source, $rs_attrs) = @_; - - if (my $groupby = $rs_attrs->{group_by}) { - - my $avail_columns = $self->_resolve_column_info ($rs_attrs->{from}); - - my $sel_index; - for my $sel (@{$rs_attrs->{select}}) { - if (ref $sel eq 'HASH' and $sel->{-as}) { - $sel_index->{$sel->{-as}} = $sel; - } - } - - my @selection; - for my $g_part (@$groupby) { - if (ref $g_part or $avail_columns->{$g_part}) { - push @selection, $g_part; - } - elsif ($sel_index->{$g_part}) { - push @selection, $sel_index->{$g_part}; - } - else { - $self->throw_exception ("group_by criteria '$g_part' not contained within current resultset source(s)"); - } - } - - return \@selection; - } - - my @pcols = map { join '.', $rs_attrs->{alias}, $_ } ($source->primary_columns); - return @pcols ? \@pcols : [ 1 ]; -} sub source_bind_attributes { my ($self, $source) = @_; diff --git a/lib/DBIx/Class/Storage/DBI/Replicated.pm b/lib/DBIx/Class/Storage/DBI/Replicated.pm index ef0cdaf..930a3be 100644 --- a/lib/DBIx/Class/Storage/DBI/Replicated.pm +++ b/lib/DBIx/Class/Storage/DBI/Replicated.pm @@ -308,7 +308,6 @@ has 'write_handler' => ( is_datatype_numeric _supports_insert_returning _count_select - _subq_count_select _subq_update_delete svp_rollback svp_begin diff --git a/t/90join_torture.t b/t/90join_torture.t index cd7d2ef..fb745dc 100644 --- a/t/90join_torture.t +++ b/t/90join_torture.t @@ -146,7 +146,7 @@ lives_ok ( sub { JOIN cd cd ON cd.cdid = me.cd_id JOIN artist artist_2 ON artist_2.artistid = cd.artist GROUP BY me.cd_id - ) count_subq + ) me )', [], ); diff --git a/t/count/count_rs.t b/t/count/count_rs.t index 0f2a1a0..f947a9b 100644 --- a/t/count/count_rs.t +++ b/t/count/count_rs.t @@ -54,7 +54,7 @@ my $schema = DBICTest->init_schema(); JOIN cd disc ON disc.cdid = tracks.cd WHERE ( ( position = ? OR position = ? ) ) LIMIT 3 OFFSET 8 - ) count_subq + ) tracks )', [ [ position => 1 ], [ position => 2 ] ], 'count_rs db-side limit applied', @@ -88,7 +88,7 @@ my $schema = DBICTest->init_schema(); JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid - ) count_subq + ) cds ', [ qw/'1' '2'/ ], 'count softlimit applied', @@ -109,7 +109,7 @@ my $schema = DBICTest->init_schema(); WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid LIMIT 3 OFFSET 4 - ) count_subq + ) cds )', [ [ 'tracks.position' => 1 ], [ 'tracks.position' => 2 ] ], 'count_rs db-side limit applied', diff --git a/t/count/prefetch.t b/t/count/prefetch.t index 7bc4708..f3818c1 100644 --- a/t/count/prefetch.t +++ b/t/count/prefetch.t @@ -31,7 +31,7 @@ my $schema = DBICTest->init_schema(); JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid - ) count_subq + ) cds )', [ map { [ 'tracks.position' => $_ ] } (1, 2) ], ); @@ -63,7 +63,7 @@ my $schema = DBICTest->init_schema(); WHERE ( genre.name = ? ) GROUP BY genre.genreid ) - count_subq + genre )', [ [ 'genre.name' => 'emo' ] ], ); diff --git a/t/prefetch/grouped.t b/t/prefetch/grouped.t index edb69b6..d2d1f17 100644 --- a/t/prefetch/grouped.t +++ b/t/prefetch/grouped.t @@ -76,7 +76,7 @@ for ($cd_rs->all) { WHERE ( me.cd IN ( ?, ?, ?, ?, ? ) ) GROUP BY me.cd ) - count_subq + me )', [ map { [ 'me.cd' => $_] } ($cd_rs->get_column ('cdid')->all) ], 'count() query generated expected SQL', @@ -151,7 +151,7 @@ for ($cd_rs->all) { WHERE ( me.cdid IS NOT NULL ) GROUP BY me.cdid LIMIT 2 - ) count_subq + ) me )', [], 'count() query generated expected SQL', @@ -262,7 +262,7 @@ for ($cd_rs->all) { WHERE ( me.cd IN ( ?, ?, ?, ?, ? ) ) GROUP BY SUBSTR(me.cd, 1, 1) ) - count_subq + me )', [ map { [ 'me.cd' => $_] } ($cd_rs->get_column ('cdid')->all) ], 'count() query generated expected SQL',