From: Peter Rabbitson Date: Fri, 22 Apr 2011 12:14:02 +0000 (+0200) Subject: Fix stupid optimizer bug with has_many joined over might_have X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=a4812caaab21149be7c3c65ad6c0388046430130;p=dbsrgits%2FDBIx-Class-Historic.git Fix stupid optimizer bug with has_many joined over might_have --- diff --git a/Changes b/Changes index c5220ec..4b1567a 100644 --- a/Changes +++ b/Changes @@ -60,6 +60,8 @@ Revision history for DBIx::Class sub-selects in the selectors list (correlated subqueries) - Fix inconsistency between $rs->next with and without HRI when all the "root" columns are in fact injected from the right rs side + - Fix the join optimizer to correctly preserve the non-multi path to + a multi relationship ( x -> might_have y -> has_many z ) * Misc - Rewire all warnings to a new Carp-like implementation internal diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index 10dd178..583045c 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -39,7 +39,14 @@ sub _prune_unused_joins { my @newfrom = $from->[0]; # FROM head is always present - my %need_joins = (map { %{$_||{}} } (values %$aliastypes) ); + my %need_joins; + for (values %$aliastypes) { + # add all requested aliases + $need_joins{$_} = 1 for keys %$_; + + # add all their parents (as per joinpath which is an AoH { table => alias }) + $need_joins{$_} = 1 for map { values %$_ } map { @$_ } values %$_; + } for my $j (@{$from}[1..$#$from]) { push @newfrom, $j if ( (! $j->[0]{-alias}) # legacy crap @@ -119,8 +126,8 @@ sub _adjust_select_args_for_complex_prefetch { my $inner_aliastypes = $self->_resolve_aliastypes_from_select_args( $inner_from, $inner_select, $where, $inner_attrs ); - # if a multi-type non-selecting (only restricting) join was needed in the subquery - # add a group_by to simulate the collapse in the subq + # we need to simulate collapse in the subq if a multiplying join is pulled + # by being a non-selecting restrictor if ( ! $inner_attrs->{group_by} and @@ -193,20 +200,26 @@ sub _adjust_select_args_for_complex_prefetch { my $outer_aliastypes = $self->_resolve_aliastypes_from_select_args( $from, $outer_select, $where, $outer_attrs ); + # unroll parents + my ($outer_select_chain, $outer_restrict_chain) = map { +{ + map { $_ => 1 } map { values %$_} map { @$_ } values %{ $outer_aliastypes->{$_} || {} } + } } qw/selecting restricting/; + # see what's left - throw away if not selecting/restricting - # also throw in a group_by if restricting to guard against - # cross-join explosions - # + # also throw in a group_by if a non-selecting multiplier, + # to guard against cross-join explosions my $need_outer_group_by; while (my $j = shift @$from) { my $alias = $j->[0]{-alias}; - if ($outer_aliastypes->{selecting}{$alias}) { - push @outer_from, $j; + if ( + $outer_select_chain->{$alias} + ) { + push @outer_from, $j } - elsif ($outer_aliastypes->{restricting}{$alias}) { + elsif ($outer_restrict_chain->{$alias}) { push @outer_from, $j; - $need_outer_group_by ||= ! $j->[0]{-is_single}; + $need_outer_group_by ||= $outer_aliastypes->{multiplying}{$alias} ? 1 : 0; } } @@ -267,8 +280,13 @@ sub _resolve_aliastypes_from_select_args { or next; $alias_list->{$al} = $j; - $aliases_by_type->{multiplying}{$al} = 1 - if ref($_) eq 'ARRAY' and ! $j->{-is_single}; # not array == {from} head == can't be multiplying + $aliases_by_type->{multiplying}{$al} ||= $j->{-join_path}||[] if ( + # not array == {from} head == can't be multiplying + ( ref($_) eq 'ARRAY' and ! $j->{-is_single} ) + or + # a parent of ours is already a multiplier + ( grep { $aliases_by_type->{multiplying}{$_} } @{ $j->{-join_path}||[] } ) + ); } # get a column to source/alias map (including unqualified ones) @@ -329,7 +347,8 @@ sub _resolve_aliastypes_from_select_args { for my $type (keys %$to_scan) { for my $piece (@{$to_scan->{$type}}) { - $aliases_by_type->{$type}{$alias} = 1 if ($piece =~ $al_re); + $aliases_by_type->{$type}{$alias} ||= $alias_list->{$alias}{-join_path}||[] + if ($piece =~ $al_re); } } } @@ -343,7 +362,10 @@ sub _resolve_aliastypes_from_select_args { for my $type (keys %$to_scan) { for my $piece (@{$to_scan->{$type}}) { - $aliases_by_type->{$type}{$colinfo->{$col}{-source_alias}} = 1 if ($piece =~ $col_re); + if ($piece =~ $col_re) { + my $alias = $colinfo->{$col}{-source_alias}; + $aliases_by_type->{$type}{$alias} ||= $alias_list->{$alias}{-join_path}||[]; + } } } } @@ -351,22 +373,13 @@ sub _resolve_aliastypes_from_select_args { # Add any non-left joins to the restriction list (such joins are indeed restrictions) for my $j (values %$alias_list) { my $alias = $j->{-alias} or next; - $aliases_by_type->{restricting}{$alias} = 1 if ( + $aliases_by_type->{restricting}{$alias} ||= $j->{-join_path}||[] if ( (not $j->{-join_type}) or ($j->{-join_type} !~ /^left (?: \s+ outer)? $/xi) ); } - # mark all restricting/selecting join parents as such - # (e.g. join => { cds => 'tracks' } - tracks will need to bring cds too ) - for my $type (qw/restricting selecting/) { - for my $alias (keys %{$aliases_by_type->{$type}||{}}) { - $aliases_by_type->{$type}{$_} = 1 - for (map { values %$_ } @{ $alias_list->{$alias}{-join_path} || [] }); - } - } - return $aliases_by_type; } diff --git a/t/90join_torture.t b/t/90join_torture.t index fb745dc..17d5116 100644 --- a/t/90join_torture.t +++ b/t/90join_torture.t @@ -154,4 +154,28 @@ lives_ok ( sub { ok (defined $rs->count); }); +# make sure multiplying endpoints do not lose heir join-path +lives_ok (sub { + my $rs = $schema->resultset('CD')->search ( + { }, + { join => { artwork => 'images' } }, + )->get_column('cdid'); + + is_same_sql_bind ( + $rs->as_query, + '( + SELECT me.cdid + FROM cd me + LEFT JOIN cd_artwork artwork + ON artwork.cd_id = me.cdid + LEFT JOIN images images + ON images.artwork_id = artwork.cd_id + )', + [], + ); + + # execution + $rs->next; +}); + done_testing;