Stop adding GROUP BY's to subqueries that do not contain 1:M joins

siracusa++ for spotting the incorrect SQL in the first place,
and pointing out that the competition sucks :)

diff --git a/Changes b/Changes
index 6e593da..fda6216 100644 (file)
--- a/Changes
+++ b/Changes
@@ -30,6 +30,8 @@ Revision history for DBIx::Class
           due to badly-written handlers (the mechanism was never meant
           to be able to suppress exceptions)
         - Fixed rels ending with me breaking subqueried limit realiasing
+        - Remove rogue GROUP BY on non-multiplying prefetch-induced
+          subqueries
         - 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/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm
index d4a9df8..7576eb9 100644 (file)
--- a/lib/DBIx/Class/Storage/DBIHacks.pm
+++ b/lib/DBIx/Class/Storage/DBIHacks.pm
@@ -37,7 +37,6 @@ sub _prune_unused_joins {
   # {multiplying} joins can go
   delete $aliastypes->{multiplying} if $attrs->{group_by};
 
-
   my @newfrom = $from->[0]; # FROM head is always present
 
   my %need_joins = (map { %{$_||{}} } (values %$aliastypes) );
@@ -103,7 +102,11 @@ sub _adjust_select_args_for_complex_prefetch {
 
   # construct the inner $from for the subquery
   # we need to prune first, because this will determine if we need a group_by below
-  my $inner_from = $self->_prune_unused_joins ($from, $inner_select, $where, $inner_attrs);
+  # the fake group_by is so that the pruner throws away all non-selecting, non-restricting
+  # multijoins (since we def. do not care about those inside the subquery)
+  my $inner_from = $self->_prune_unused_joins ($from, $inner_select, $where, {
+    group_by => ['dummy'], %$inner_attrs,
+  });
 
   # if a multi-type join was needed in the subquery - add a group_by to simulate the
   # collapse in the subq
@@ -134,14 +137,13 @@ sub _adjust_select_args_for_complex_prefetch {
   # - it is part of the restrictions, in which case we need to collapse the outer
   #   result by tackling yet another group_by to the outside of the query
 
-  # normalize a copy of $from, so it will be easier to work with further
-  # down (i.e. promote the initial hashref to an AoH)
   $from = [ @$from ];
-  $from->[0] = [ $from->[0] ];
 
   # so first generate the outer_from, up to the substitution point
   my @outer_from;
   while (my $j = shift @$from) {
+    $j = [ $j ] unless ref $j eq 'ARRAY'; # promote the head-from to an AoH
+
     if ($j->[0]{-alias} eq $attrs->{alias}) { # time to swap
       push @outer_from, [
         $subq_joinspec,
@@ -218,7 +220,7 @@ sub _resolve_aliastypes_from_select_args {
 
     $alias_list->{$al} = $j;
     $aliases_by_type->{multiplying}{$al} = 1
-      unless $j->{-is_single};
+      if ref($_) eq 'ARRAY' and ! $j->{-is_single}; # not array == {from} head == can't be multiplying
   }
 
   # get a column to source/alias map (including unqualified ones)

diff --git a/t/prefetch/with_limit.t b/t/prefetch/with_limit.t
index b8c13a3..9c1f067 100644 (file)
--- a/t/prefetch/with_limit.t
+++ b/t/prefetch/with_limit.t
@@ -7,6 +7,7 @@ use Test::More;
 use Test::Exception;
 use lib qw(t/lib);
 use DBICTest;
+use DBIC::SqlMakerTest;
 
 my $schema = DBICTest->init_schema();
 
@@ -90,4 +91,41 @@ is($artist->cds->count, 1, "count on search limiting prefetched has_many");
 my $artist2 = $use_prefetch->search({'cds.title' => { '!=' => $artist_many_cds->cds->first->title } })->slice (0,0)->next;
 is($artist2->cds->count, 2, "count on search limiting prefetched has_many");
 
+# make sure 1:1 joins do not force a subquery (no point to exercise the optimizer, if at all available)
+# get cd's that have any tracks and their artists
+my $single_prefetch_rs = $schema->resultset ('CD')->search (
+  { 'me.year' => 2010, 'artist.name' => 'foo' },
+  { prefetch => ['tracks', 'artist'], rows => 15 },
+);
+is_same_sql_bind (
+  $single_prefetch_rs->as_query,
+  '(
+    SELECT
+        me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track,
+        tracks.trackid, tracks.cd, tracks.position, tracks.title, tracks.last_updated_on, tracks.last_updated_at, tracks.small_dt,
+        artist.artistid, artist.name, artist.rank, artist.charfield
+      FROM (
+        SELECT
+            me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track
+          FROM cd me
+          JOIN artist artist ON artist.artistid = me.artist
+        WHERE ( ( artist.name = ? AND me.year = ? ) )
+        LIMIT 15
+      ) me
+      LEFT JOIN track tracks
+        ON tracks.cd = me.cdid
+      JOIN artist artist
+        ON artist.artistid = me.artist
+    WHERE ( ( artist.name = ? AND me.year = ? ) )
+    ORDER BY tracks.cd
+  )',
+  [
+    [ 'artist.name' => 'foo' ],
+    [ 'me.year'     => 2010  ],
+    [ 'artist.name' => 'foo' ],
+    [ 'me.year'     => 2010  ],
+  ],
+  'No grouping of non-multiplying resultsets',
+);
+
 done_testing;