From: Peter Rabbitson Date: Wed, 5 Jan 2011 14:52:09 +0000 (+0100) Subject: Make sure unaliased selectors and prefetch coexist peacefully X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=36fd7f073078f8f36277b467910ab68676361edf;p=dbsrgits%2FDBIx-Class-Historic.git Make sure unaliased selectors and prefetch coexist peacefully --- diff --git a/Changes b/Changes index eaee66e..96478b4 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,7 @@ Revision history for DBIx::Class * Fixes + - Unaliased "dark" selectors no longer throw off prefetch - Fix proper composition of bind values across all possible SQL areas ( group_by => \[ ... ] now works properly ) diff --git a/lib/DBIx/Class/ResultSet.pm b/lib/DBIx/Class/ResultSet.pm index 733c82d..554c93d 100644 --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@ -1436,7 +1436,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, nor locking it - delete @{$sub_attrs}{qw/collapse columns as select _prefetch_select _trailing_select order_by for/}; + delete @{$sub_attrs}{qw/collapse columns as select _prefetch_selector_range _trailing_select order_by for/}; # 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 @@ -1650,7 +1650,7 @@ sub _rs_update_delete { my $attrs = $self->_resolved_attrs_copy; - delete $attrs->{$_} for qw/collapse _collapse_order_by select _prefetch_select as/; + delete $attrs->{$_} for qw/collapse _collapse_order_by select _prefetch_selector_range as/; $attrs->{columns} = [ map { "$attrs->{alias}.$_" } ($self->result_source->_pri_cols) ]; if ($needs_group_by_subq) { @@ -3243,9 +3243,6 @@ sub _resolved_attrs { push @sel, @{ ref $attrs->{select} eq 'ARRAY' ? $attrs->{select} : [ $attrs->{select} ] } if $attrs->{select}; - push @sel, @{$attrs->{_trailing_select}} - if $attrs->{_trailing_select}; - # assume all unqualified selectors to apply to the current alias (legacy stuff) for (@sel) { $_ = (ref $_ or $_ =~ /\./) ? $_ : "$alias.$_"; @@ -3331,8 +3328,13 @@ sub _resolved_attrs { carp ("Useless use of distinct on a grouped resultset ('distinct' is ignored when a 'group_by' is present)"); } else { + # distinct affects only the main selection part, not what prefetch may + # add below. However trailing is not yet a part of the selection as + # prefetch must insert before it $attrs->{group_by} = $source->storage->_group_over_selection ( - @{$attrs}{qw/from select order_by/} + $attrs->{from}, + [ @{$attrs->{select}||[]}, @{$attrs->{_trailing_select}||[]} ], + $attrs->{order_by}, ); } } @@ -3368,15 +3370,22 @@ sub _resolved_attrs { $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $attrs->{collapse} ); # we need to somehow mark which columns came from prefetch - $attrs->{_prefetch_select} = [ map { $_->[0] } @prefetch ]; + if (@prefetch) { + my $sel_end = $#{$attrs->{select}}; + $attrs->{_prefetch_selector_range} = [ $sel_end + 1, $sel_end + @prefetch ]; + } - push @{ $attrs->{select} }, @{$attrs->{_prefetch_select}}; + push @{ $attrs->{select} }, (map { $_->[0] } @prefetch); push @{ $attrs->{as} }, (map { $_->[1] } @prefetch); push( @{$attrs->{order_by}}, @$prefetch_ordering ); $attrs->{_collapse_order_by} = \@$prefetch_ordering; } + + push @sel, @{$attrs->{_trailing_select}} + if $attrs->{_trailing_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 d86a763..e67d36b 100644 --- a/lib/DBIx/Class/Storage/DBI.pm +++ b/lib/DBIx/Class/Storage/DBI.pm @@ -2087,9 +2087,7 @@ sub _select_args { && @{$attrs->{group_by}} && - $attrs->{_prefetch_select} - && - @{$attrs->{_prefetch_select}} + $attrs->{_prefetch_selector_range} ) ) { ($ident, $select, $where, $attrs) diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index 864cbf5..cbf4626 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -60,7 +60,7 @@ sub _adjust_select_args_for_complex_prefetch { my ($self, $from, $select, $where, $attrs) = @_; $self->throw_exception ('Nothing to prefetch... how did we get here?!') - if not @{$attrs->{_prefetch_select}}; + if not @{$attrs->{_prefetch_selector_range}}; $self->throw_exception ('Complex prefetches are not supported on resultsets with a custom from attribute') if (ref $from ne 'ARRAY' || ref $from->[0] ne 'HASH' || ref $from->[1] ne 'ARRAY'); @@ -71,7 +71,7 @@ sub _adjust_select_args_for_complex_prefetch { delete $outer_attrs->{$_} for qw/where bind rows offset group_by having/; my $inner_attrs = { %$attrs }; - delete $inner_attrs->{$_} for qw/for collapse _prefetch_select _collapse_order_by select as/; + delete $inner_attrs->{$_} for qw/for collapse _prefetch_selector_range _collapse_order_by select as/; # bring over all non-collapse-induced order_by into the inner query (if any) @@ -88,7 +88,9 @@ sub _adjust_select_args_for_complex_prefetch { # on the outside we substitute any function for its alias my $outer_select = [ @$select ]; my $inner_select = []; - for my $i (0 .. ( @$outer_select - @{$outer_attrs->{_prefetch_select}} - 1) ) { + + my ($p_start, $p_end) = @{$outer_attrs->{_prefetch_selector_range}}; + for my $i (0 .. $p_start - 1, $p_end + 1 .. $#$outer_select) { my $sel = $outer_select->[$i]; if (ref $sel eq 'HASH' ) { @@ -197,6 +199,7 @@ sub _adjust_select_args_for_complex_prefetch { # also throw in a group_by if restricting to guard against # cross-join explosions # + my $need_outer_group_by; while (my $j = shift @$from) { my $alias = $j->[0]{-alias}; @@ -205,13 +208,28 @@ sub _adjust_select_args_for_complex_prefetch { } elsif ($outer_aliastypes->{restricting}{$alias}) { push @outer_from, $j; - $outer_attrs->{group_by} ||= $outer_select unless $j->[0]{-is_single}; + $need_outer_group_by ||= ! $j->[0]{-is_single}; } } # demote the outer_from head $outer_from[0] = $outer_from[0][0]; + if ($need_outer_group_by and ! $outer_attrs->{group_by}) { + + my $unprocessed_order_chunks; + ($outer_attrs->{group_by}, $unprocessed_order_chunks) = $self->_group_over_selection ( + \@outer_from, $outer_select, $outer_attrs->{order_by} + ); + + $self->throw_exception ( + 'A required group_by clause could not be constructed automatically due to a complex ' + . 'order_by criteria. Either order_by columns only (no functions) or construct a suitable ' + . 'group_by by hand' + ) if $unprocessed_order_chunks; + + } + # 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 @@ -362,6 +380,9 @@ sub _group_over_selection { my (@group_by, %group_index); + # the logic is: if it is a { func => val } we assume an aggregate, + # otherwise if \'...' or \[...] we assume the user knows what is + # going on thus group over it for (@$select) { if (! ref($_) or ref ($_) ne 'HASH' ) { push @group_by, $_; diff --git a/t/prefetch/correlated.t b/t/prefetch/correlated.t index e4e747d..7e7690d 100644 --- a/t/prefetch/correlated.t +++ b/t/prefetch/correlated.t @@ -19,10 +19,6 @@ my $cd_data = { map { }, } ( $cdrs->all ) }; -my $queries = 0; -$schema->storage->debugcb(sub { $queries++; }); -$schema->storage->debug(1); - my $c_rs = $cdrs->search ({}, { prefetch => 'tracks', '+columns' => { sibling_count => $cdrs->search( @@ -53,13 +49,21 @@ is_same_sql_bind( ORDER BY tracks.cd )', [ + + # subselect [ 'siblings.cdid' => 'bogus condition' ], [ 'me.artist' => 2 ], + + # outher WHERE [ 'me.artist' => 2 ], ], 'Expected SQL on correlated realiased subquery' ); +my $queries = 0; +$schema->storage->debugcb(sub { $queries++; }); +$schema->storage->debug(1); + is_deeply ( { map { $_->cdid => { @@ -77,4 +81,65 @@ is ($queries, 1, 'Only 1 query fired to retrieve everything'); $schema->storage->debug($orig_debug); $schema->storage->debugcb(undef); +# try to unbalance the select + +# first add a lone non-as-ed select +# it should be reordered to appear at the end without throwing prefetch/bind off +$c_rs = $c_rs->search({}, { '+select' => \[ 'me.cdid + ?', [ __add => 1 ] ] }); + +# now add an unbalanced select/as pair +$c_rs = $c_rs->search ({}, { + '+select' => $cdrs->search( + { 'siblings.artist' => { -ident => 'me.artist' } }, + { alias => 'siblings', columns => [ + { first_year => { min => 'year' }}, + { last_year => { max => 'year' }}, + ]}, + )->as_query, + '+as' => [qw/active_from active_to/], +}); + + +is_same_sql_bind( + $c_rs->as_query, + '( + SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track, + (SELECT COUNT( * ) + FROM cd siblings + WHERE siblings.artist = me.artist + AND siblings.cdid != me.cdid + AND siblings.cdid != ? + AND me.artist != ? + ), + (SELECT MIN( year ), MAX( year ) + FROM cd siblings + WHERE siblings.artist = me.artist + AND me.artist != ? + ), + tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at, + me.cdid + ? + FROM cd me + LEFT JOIN track tracks + ON tracks.cd = me.cdid + WHERE me.artist != ? + ORDER BY tracks.cd + )', + [ + + # first subselect + [ 'siblings.cdid' => 'bogus condition' ], + [ 'me.artist' => 2 ], + + # second subselect + [ 'me.artist' => 2 ], + + # the addition + [ __add => 1 ], + + # outher WHERE + [ 'me.artist' => 2 ], + ], + 'Expected SQL on correlated realiased subquery' +); + done_testing; diff --git a/t/prefetch/with_limit.t b/t/prefetch/with_limit.t index 4acdcab..f6729b1 100644 --- a/t/prefetch/with_limit.t +++ b/t/prefetch/with_limit.t @@ -31,6 +31,62 @@ my $use_prefetch = $no_prefetch->search( } ); +# add a floating +select to make sure it does nto throw things off +# we also expect it to appear in both selectors, as we can not know +# for sure which part of the query it applies to (may be order_by, +# maybe something else) +# +# we use a reference to the same array in bind vals, because +# is_deeply picks up this difference too (not sure if bug or +# feature) +my $bind_one = [ __add => 1 ]; +$use_prefetch = $use_prefetch->search({}, { + '+select' => \[ 'me.artistid + ?', $bind_one ], +}); + +is_same_sql_bind ( + $use_prefetch->as_query, + '( + SELECT me.artistid, me.name, + cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track, + me.artistid + ? + FROM ( + SELECT me.artistid, me.name, + me.artistid + ? + FROM artist me + LEFT JOIN cd cds + ON cds.artist = me.artistid + LEFT JOIN cd_artwork artwork + ON artwork.cd_id = cds.cdid + LEFT JOIN track tracks + ON tracks.cd = cds.cdid + WHERE artwork.cd_id IS NULL + OR tracks.title != ? + GROUP BY me.artistid, me.name, me.artistid + ? + ORDER BY name DESC LIMIT 3 + ) me + LEFT JOIN cd cds + ON cds.artist = me.artistid + LEFT JOIN cd_artwork artwork + ON artwork.cd_id = cds.cdid + LEFT JOIN track tracks + ON tracks.cd = cds.cdid + WHERE artwork.cd_id IS NULL + OR tracks.title != ? + GROUP BY me.artistid, me.name, cds.cdid, cds.artist, cds.title, cds.year, cds.genreid, cds.single_track, me.artistid + ? + ORDER BY name DESC, cds.artist, cds.year ASC + )', + [ + $bind_one, # outer select + $bind_one, # inner select + [ 'tracks.title' => 'blah-blah-1234568' ], # inner where + $bind_one, # inner group_by + [ 'tracks.title' => 'blah-blah-1234568' ], # outer where + $bind_one, # outer group_by + ], + 'Expected SQL on complex limited prefetch' +); + is($no_prefetch->count, $use_prefetch->count, '$no_prefetch->count == $use_prefetch->count'); is( scalar ($no_prefetch->all), diff --git a/t/search/select_chains_unbalanced.t b/t/search/select_chains_unbalanced.t index d0facb8..d602605 100644 --- a/t/search/select_chains_unbalanced.t +++ b/t/search/select_chains_unbalanced.t @@ -34,33 +34,30 @@ my @chain = ( => [qw/cdid title foo bar/], { - '+select' => [ 'genreid', $multicol_rs->as_query ], - '+as' => [qw/genreid name rank/], + '+select' => \'unaliased randomness', } => 'SELECT me.cdid, me.title, DISTINCT(foo, bar), - me.genreid, - (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )) + unaliased randomness FROM cd me' - => [qw/cdid title foo bar genreid name rank/], - + => [qw/cdid title foo bar/], { - '+select' => { count => 'me.cdid', -as => 'cnt' }, # lack of 'as' infers from '-as' - '+columns' => { len => { length => 'me.title' } }, + '+select' => [ 'genreid', $multicol_rs->as_query ], + '+as' => [qw/genreid name rank/], } => 'SELECT me.cdid, me.title, - LENGTH( me.title ), - COUNT( me.cdid ) AS cnt, DISTINCT(foo, bar), me.genreid, - (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )) + (SELECT me.name, me.rank FROM artist me WHERE ( artistid 1 )), + unaliased randomness FROM cd me' - => [qw/cdid title len cnt foo bar genreid name rank/], + => [qw/cdid title foo bar genreid name rank/], { - '+select' => \'unaliased randomness', + '+select' => { count => 'me.cdid', -as => 'cnt' }, # lack of 'as' infers from '-as' + '+columns' => { len => { length => 'me.title' } }, } => 'SELECT me.cdid, me.title, @@ -73,6 +70,7 @@ my @chain = ( FROM cd me' => [qw/cdid title len cnt foo bar genreid name rank/], + ); my $rs = $schema->resultset('CD'); @@ -95,7 +93,7 @@ while (@chain) { is_deeply( $rs->_resolved_attrs->{as}, $as, - 'Correct dbic-side aliasing', + "Correct dbic-side aliasing for test $testno", ); $testno++;