Merge 'trunk' into 'mssql_storage_minor_refactor'
Peter Rabbitson [Wed, 5 Aug 2009 06:37:48 +0000 (06:37 +0000)]
r7208@Thesaurus (orig r7205):  ribasushi | 2009-08-05 08:34:25 +0200
Bump dependencies:
Test::More for the new no_plan/done_testing goodies
File::Temp as per RT#48431
r7210@Thesaurus (orig r7207):  ribasushi | 2009-08-05 08:36:32 +0200
 r7156@Thesaurus (orig r7153):  robkinyon | 2009-07-30 20:06:04 +0200
 Create prefetch_redux branch
 r7164@Thesaurus (orig r7161):  robkinyon | 2009-07-31 22:41:01 +0200
 Added MooseX::Traits to Makefile.PL
 r7172@Thesaurus (orig r7169):  robkinyon | 2009-08-03 05:49:59 +0200
 Added two tests and marked one todo_skip
 r7187@Thesaurus (orig r7184):  ribasushi | 2009-08-03 17:24:41 +0200
 Use goto to preserve correct error-at-line reporting
 r7189@Thesaurus (orig r7186):  ribasushi | 2009-08-04 12:34:58 +0200
 Add an extra test specifically for distinct/prefetch
 Remove duplicate test in count/prefetch

 Switch to as_query instead of debug overloading
 r7190@Thesaurus (orig r7187):  ribasushi | 2009-08-04 12:35:57 +0200
 Fix how a distinct-induced group_by is calculated, taking in consideration the new prefetch mechanism
 r7197@Thesaurus (orig r7194):  ribasushi | 2009-08-04 17:31:33 +0200
 Traits not needed by anything currently in dbic
 r7198@Thesaurus (orig r7195):  ribasushi | 2009-08-04 17:41:14 +0200
 Move around tests a bit
 r7199@Thesaurus (orig r7196):  mo | 2009-08-04 21:10:57 +0200
 prefetch-grouped fails, again
 r7204@Thesaurus (orig r7201):  ribasushi | 2009-08-04 22:50:51 +0200
 Split the search_related prefetch tests into a standalone testfile
 r7205@Thesaurus (orig r7202):  ribasushi | 2009-08-04 23:05:03 +0200
 Move norbi's test to prefetch_redux - it's the same idea
 r7209@Thesaurus (orig r7206):  ribasushi | 2009-08-05 08:35:48 +0200
 Tadaaaa (even more prefetch insanity)

Makefile.PL
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/Storage/DBI.pm
t/count/prefetch.t
t/lib/DBIC/SqlMakerTest.pm
t/prefetch/grouped.t
t/prefetch/via_search_related.t [new file with mode: 0644]

index d21951b..79da35f 100644 (file)
@@ -13,9 +13,11 @@ all_from 'lib/DBIx/Class.pm';
 test_requires 'Test::Builder'       => 0.33;
 test_requires 'Test::Deep'          => 0;
 test_requires 'Test::Exception'     => 0;
-test_requires 'Test::More'          => 0.82;
+test_requires 'Test::More'          => 0.92;
 test_requires 'Test::Warn'          => 0.11;
 
+test_requires 'File::Temp'          => 0.22;
+
 # Core
 requires 'List::Util'               => 0;
 requires 'Scalar::Util'             => 0;
index 9fba9fc..e24dafe 100644 (file)
@@ -1264,7 +1264,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
-  delete $sub_attrs->{$_} for qw/collapse prefetch_select select as order_by/;
+  delete $sub_attrs->{$_} for qw/collapse select _prefetch_select as order_by/;
 
   # if we prefetch, we group_by primary keys only as this is what we would get out of the rs via ->next/->all
   # clobber old group_by regardless
@@ -2875,6 +2875,12 @@ sub _resolved_attrs {
     $attrs->{group_by} = [ $attrs->{group_by} ];
   }
 
+  # generate the distinct induced group_by early, as prefetch will be carried via a
+  # subquery (since a group_by is present)
+  if (delete $attrs->{distinct}) {
+    $attrs->{group_by} ||= [ grep { !ref($_) || (ref($_) ne 'HASH') } @{$attrs->{select}} ];
+  }
+
   $attrs->{collapse} ||= {};
   if ( my $prefetch = delete $attrs->{prefetch} ) {
     $prefetch = $self->_merge_attr( {}, $prefetch );
@@ -2886,19 +2892,16 @@ sub _resolved_attrs {
     my @prefetch =
       $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $attrs->{collapse} );
 
-    $attrs->{prefetch_select} = [ map { $_->[0] } @prefetch ];
-    push @{ $attrs->{select} }, @{$attrs->{prefetch_select}};
+    # we need to somehow mark which columns came from prefetch
+    $attrs->{_prefetch_select} = [ map { $_->[0] } @prefetch ];
+
+    push @{ $attrs->{select} }, @{$attrs->{_prefetch_select}};
     push @{ $attrs->{as} }, (map { $_->[1] } @prefetch);
 
     push( @{$attrs->{order_by}}, @$prefetch_ordering );
     $attrs->{_collapse_order_by} = \@$prefetch_ordering;
   }
 
-
-  if (delete $attrs->{distinct}) {
-    $attrs->{group_by} ||= [ grep { !ref($_) || (ref($_) ne 'HASH') } @{$attrs->{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
index 1dd8d8d..84969f6 100644 (file)
@@ -1452,7 +1452,7 @@ sub _select_args {
     ( $attrs->{rows} && keys %{$attrs->{collapse}} )
        ||
     ( $attrs->{group_by} && @{$attrs->{group_by}} &&
-      $attrs->{prefetch_select} && @{$attrs->{prefetch_select}} )
+      $attrs->{_prefetch_select} && @{$attrs->{_prefetch_select}} )
   ) {
     ($ident, $select, $where, $attrs)
       = $self->_adjust_select_args_for_complex_prefetch ($ident, $select, $where, $attrs);
@@ -1497,14 +1497,15 @@ sub _adjust_select_args_for_complex_prefetch {
   # separate attributes
   my $sub_attrs = { %$attrs };
   delete $attrs->{$_} for qw/where bind rows offset group_by having/;
-  delete $sub_attrs->{$_} for qw/for collapse prefetch_select _collapse_order_by select as/;
+  delete $sub_attrs->{$_} for qw/for collapse _prefetch_select _collapse_order_by select as/;
 
-  my $alias = $attrs->{alias};
+  my $select_root_alias = $attrs->{alias};
   my $sql_maker = $self->sql_maker;
 
   # create subquery select list - consider only stuff *not* brought in by the prefetch
   my $sub_select = [];
-  for my $i (0 .. @{$attrs->{select}} - @{$attrs->{prefetch_select}} - 1) {
+  my $sub_group_by;
+  for my $i (0 .. @{$attrs->{select}} - @{$attrs->{_prefetch_select}} - 1) {
     my $sel = $attrs->{select}[$i];
 
     # alias any functions to the dbic-side 'as' label
@@ -1526,29 +1527,15 @@ sub _adjust_select_args_for_complex_prefetch {
     ];
   }
 
-  # mangle {from}
+  # mangle {from}, keep in mind that $from is "headless" from here on
   my $join_root = shift @$from;
-  my @outer_from = @$from;
 
   my %inner_joins;
   my %join_info = map { $_->[0]{-alias} => $_->[0] } (@$from);
 
-  # in complex search_related chains $alias may *not* be 'me'
-  # so always include it in the inner join, and also shift away
-  # from the outer stack, so that the two datasets actually do
-  # meet
-  if ($join_root->{-alias} ne $alias) {
-    $inner_joins{$alias} = 1;
-
-    while (@outer_from && $outer_from[0][0]{-alias} ne $alias) {
-      shift @outer_from;
-    }
-    if (! @outer_from) {
-      $self->throw_exception ("Unable to find '$alias' in the {from} stack, something is wrong");
-    }
-
-    shift @outer_from; # the new subquery will represent this alias, so get rid of it
-  }
+  # in complex search_related chains $select_root_alias may *not* be
+  # 'me' so always include it in the inner join
+  $inner_joins{$select_root_alias} = 1 if ($join_root->{-alias} ne $select_root_alias);
 
 
   # decide which parts of the join will remain on the inside
@@ -1617,13 +1604,15 @@ sub _adjust_select_args_for_complex_prefetch {
 
   # if a multi-type join was needed in the subquery ("multi" is indicated by
   # presence in {collapse}) - add a group_by to simulate the collapse in the subq
-  for my $alias (keys %inner_joins) {
-
-    # the dot comes from some weirdness in collapse
-    # remove after the rewrite
-    if ($attrs->{collapse}{".$alias"}) {
-      $sub_attrs->{group_by} ||= $sub_select;
-      last;
+  unless ($sub_attrs->{group_by}) {
+    for my $alias (keys %inner_joins) {
+
+      # the dot comes from some weirdness in collapse
+      # remove after the rewrite
+      if ($attrs->{collapse}{".$alias"}) {
+        $sub_attrs->{group_by} ||= $sub_select;
+        last;
+      }
     }
   }
 
@@ -1634,14 +1623,42 @@ sub _adjust_select_args_for_complex_prefetch {
     $where,
     $sub_attrs
   );
-
-  # put it in the new {from}
-  unshift @outer_from, {
-    -alias => $alias,
+  my $subq_joinspec = {
+    -alias => $select_root_alias,
     -source_handle => $join_root->{-source_handle},
-    $alias => $subq,
+    $select_root_alias => $subq,
   };
 
+  # Generate a new from (really just replace the join slot with the subquery)
+  # Before we would start the outer chain from the subquery itself (i.e.
+  # SELECT ... FROM (SELECT ... ) alias JOIN ..., but this turned out to be
+  # a bad idea for search_related, as the root of the chain was effectively
+  # lost (i.e. $artist_rs->search_related ('cds'... ) would result in alias
+  # of 'cds', which would prevent from doing things like order_by artist.*)
+  # See t/prefetch/via_search_related.t for a better idea
+  my @outer_from;
+  if ($join_root->{-alias} eq $select_root_alias) { # just swap the root part and we're done
+    @outer_from = (
+      $subq_joinspec,
+      @$from,
+    )
+  }
+  else {  # this is trickier
+    @outer_from = ($join_root);
+
+    for my $j (@$from) {
+      if ($j->[0]{-alias} eq $select_root_alias) {
+        push @outer_from, [
+          $subq_joinspec,
+          @{$j}[1 .. $#$j],
+        ];
+      }
+      else {
+        push @outer_from, $j;
+      }
+    }
+  }
+
   # 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
index 2032a4b..8012e10 100644 (file)
@@ -6,9 +6,6 @@ use lib qw(t/lib);
 use Test::More;
 use DBICTest;
 use DBIC::SqlMakerTest;
-use DBIC::DebugObj;
-
-plan tests => 6;
 
 my $schema = DBICTest->init_schema();
 
@@ -20,20 +17,58 @@ my $schema = DBICTest->init_schema();
                 { prefetch => [qw/tracks artist/] },
             );
   is ($rs->all, 5, 'Correct number of objects');
+  is ($rs->count, 5, 'Correct count');
 
+  is_same_sql_bind (
+    $rs->count_rs->as_query,
+    '(
+      SELECT COUNT( * )
+        FROM (
+          SELECT cds.cdid
+            FROM artist me
+            JOIN cd cds ON cds.artist = me.artistid
+            LEFT JOIN track tracks ON tracks.cd = cds.cdid
+            JOIN artist artist ON artist.artistid = cds.artist
+          WHERE tracks.position = ? OR tracks.position = ?
+          GROUP BY cds.cdid
+        ) count_subq
+    )',
+    [ map { [ 'tracks.position' => $_ ] } (1, 2) ],
+  );
+}
 
-  my ($sql, @bind);
-  $schema->storage->debugobj(DBIC::DebugObj->new(\$sql, \@bind));
-  $schema->storage->debug(1);
-
-
-  is ($rs->count, 5, 'Correct count');
+# collapsing prefetch with distinct
+{
+  my $first_cd = $schema->resultset('Artist')->first->cds->first;
+  $first_cd->update ({
+    genreid => $first_cd->create_related (
+      genre => ({ name => 'vague genre' })
+    )->id
+  });
+
+  my $rs = $schema->resultset("Artist")->search(undef, {distinct => 1})
+            ->search_related('cds')->search_related('genre',
+                { 'genre.name' => { '!=', 'foo' } },
+                { prefetch => q(cds) },
+            );
+  is ($rs->all, 1, 'Correct number of objects');
+  is ($rs->count, 1, 'Correct count');
 
   is_same_sql_bind (
-    $sql,
-    \@bind,
-    'SELECT COUNT( * ) FROM (SELECT cds.cdid FROM artist me JOIN cd cds ON cds.artist = me.artistid LEFT JOIN track tracks ON tracks.cd = cds.cdid JOIN artist artist ON artist.artistid = cds.artist WHERE tracks.position = ? OR tracks.position = ? GROUP BY cds.cdid) count_subq',
-    [ qw/'1' '2'/ ],
+    $rs->count_rs->as_query,
+    '(
+      SELECT COUNT( * )
+        FROM (
+          SELECT genre.genreid
+            FROM artist me
+            JOIN cd cds ON cds.artist = me.artistid
+            JOIN genre genre ON genre.genreid = cds.genreid
+            LEFT JOIN cd cds_2 ON cds_2.genreid = genre.genreid
+          WHERE ( genre.name != ? )
+          GROUP BY genre.genreid
+        ) count_subq
+    )',
+    [ [ 'genre.name' => 'foo' ] ],
   );
 }
 
@@ -47,17 +82,20 @@ my $schema = DBICTest->init_schema();
   is ($rs->all, 10, 'Correct number of objects');
 
 
-  my ($sql, @bind);
-  $schema->storage->debugobj(DBIC::DebugObj->new(\$sql, \@bind));
-  $schema->storage->debug(1);
-
-
   is ($rs->count, 10, 'Correct count');
 
   is_same_sql_bind (
-    $sql,
-    \@bind,
-    'SELECT COUNT( * ) FROM cd me JOIN track tracks ON tracks.cd = me.cdid JOIN cd disc ON disc.cdid = tracks.cd LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid WHERE ( ( position = ? OR position = ? ) )',
-    [ qw/'1' '2'/ ],
+    $rs->count_rs->as_query,
+    '(
+      SELECT COUNT( * )
+        FROM cd me
+        JOIN track tracks ON tracks.cd = me.cdid
+        JOIN cd disc ON disc.cdid = tracks.cd
+        LEFT JOIN lyrics lyrics ON lyrics.track_id = tracks.trackid
+      WHERE position = ? OR position = ?
+    )',
+    [ map { [ position => $_ ] } (1, 2) ],
   );
 }
+
+done_testing;
index 1098e25..44ccb4b 100644 (file)
@@ -41,7 +41,8 @@ sub is_same_sql_bind {
   croak "Unexpected argument(s) supplied to is_same_sql_bind: " . join ('; ', @_)
     if @_;
 
-  SQL::Abstract::Test::is_same_sql_bind (@args);
+  @_ = @args;
+  goto &SQL::Abstract::Test::is_same_sql_bind;
 }
 
 *is_same_sql = \&SQL::Abstract::Test::is_same_sql;
index 0a61f97..92f383c 100644 (file)
@@ -1,14 +1,13 @@
 use strict;
 use warnings;
+
 use Test::More;
+use Test::Exception;
 
 use lib qw(t/lib);
 use DBICTest;
 use DBIC::SqlMakerTest;
 
-#plan tests => 6;
-plan 'no_plan';
-
 my $schema = DBICTest->init_schema();
 my $sdebug = $schema->storage->debug;
 
@@ -205,3 +204,35 @@ for ($cd_rs->all) {
   $schema->storage->debugcb (undef);
   $schema->storage->debug ($sdebug);
 }
+
+# make sure that distinct still works
+{
+  my $rs = $schema->resultset("CD")->search({}, {
+    prefetch => 'tags',
+    order_by => 'cdid',
+    distinct => 1,
+  });
+
+  is_same_sql_bind (
+    $rs->as_query,
+    '(
+      SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track,
+             tags.tagid, tags.cd, tags.tag 
+        FROM (
+          SELECT me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track
+            FROM cd me
+          GROUP BY me.cdid, me.artist, me.title, me.year, me.genreid, me.single_track
+          ORDER BY cdid
+        ) me
+        LEFT JOIN tags tags ON tags.cd = me.cdid
+      ORDER BY cdid, tags.cd, tags.tag
+    )',
+    [],
+    'Prefetch + distinct resulted in correct group_by',
+  );
+
+  is ($rs->all, 5, 'Correct number of CD objects');
+  is ($rs->count, 5, 'Correct count of CDs');
+}
+
+done_testing;
diff --git a/t/prefetch/via_search_related.t b/t/prefetch/via_search_related.t
new file mode 100644 (file)
index 0000000..041c341
--- /dev/null
@@ -0,0 +1,93 @@
+use strict;
+use warnings;
+
+use Test::More;
+use Test::Exception;
+
+use lib qw(t/lib);
+use DBICTest;
+
+my $schema = DBICTest->init_schema();
+
+lives_ok ( sub {
+  my $no_prefetch = $schema->resultset('Track')->search_related(cd =>
+    {
+      'cd.year' => "2000",
+    },
+    {
+      join => 'tags',
+      order_by => 'me.trackid',
+      rows => 1,
+    }
+  );
+
+  my $use_prefetch = $no_prefetch->search(
+    {},
+    {
+      prefetch => 'tags',
+    }
+  );
+
+  is($use_prefetch->count, $no_prefetch->count, 'counts with and without prefetch match');
+  is(
+    scalar ($use_prefetch->all),
+    scalar ($no_prefetch->all),
+    "Amount of returned rows is right"
+  );
+
+}, 'search_related prefetch with order_by works');
+
+
+lives_ok (sub {
+    my $rs = $schema->resultset("Artwork")->search(undef, {distinct => 1})
+              ->search_related('artwork_to_artist')->search_related('artist',
+                undef,
+                { prefetch => 'cds' },
+              );
+    is($rs->all, 0, 'prefetch without WHERE (objects)');
+    is($rs->count, 0, 'prefetch without WHERE (count)');
+
+    $rs = $schema->resultset("Artwork")->search(undef, {distinct => 1})
+              ->search_related('artwork_to_artist')->search_related('artist',
+                { 'cds.title' => 'foo' },
+                { prefetch => 'cds' },
+              );
+    is($rs->all, 0, 'prefetch with WHERE (objects)');
+    is($rs->count, 0, 'prefetch with WHERE (count)');
+
+
+# test where conditions at the root of the related chain
+    my $artist_rs = $schema->resultset("Artist")->search({artistid => 11});
+
+
+    $rs = $artist_rs->search_related('cds')->search_related('genre',
+                    { 'genre.name' => 'foo' },
+                    { prefetch => 'cds' },
+                 );
+    is($rs->all, 0, 'prefetch without distinct (objects)');
+    is($rs->count, 0, 'prefetch without distinct (count)');
+
+
+
+    $rs = $artist_rs->search(undef, {distinct => 1})
+                ->search_related('cds')->search_related('genre',
+                    { 'genre.name' => 'foo' },
+                 );
+    is($rs->all, 0, 'distinct without prefetch (objects)');
+    is($rs->count, 0, 'distinct without prefetch (count)');
+
+
+
+    $rs = $artist_rs->search({}, {distinct => 1})
+                ->search_related('cds')->search_related('genre',
+                    { 'genre.name' => 'foo' },
+                    { prefetch => 'cds' },
+                 );
+    is($rs->all, 0, 'distinct with prefetch (objects)');
+    is($rs->count, 0, 'distinct with prefetch (count)');
+
+
+
+}, 'distinct generally works with prefetch on deep search_related chains');
+
+done_testing;