Merge 'trunk' into 'on_connect_call'
Rafael Kitover [Fri, 19 Jun 2009 21:09:48 +0000 (21:09 +0000)]
r5559@hlagh (orig r6713):  caelum | 2009-06-18 16:03:01 -0700
fix broken link in manual
r5571@hlagh (orig r6725):  ribasushi | 2009-06-19 08:25:19 -0700
 r6706@Thesaurus (orig r6705):  ribasushi | 2009-06-18 12:30:08 +0200
 Branch to attempt prefetch with limit fix
 r6709@Thesaurus (orig r6708):  ribasushi | 2009-06-18 15:54:38 +0200
 This seems to be the prefetch+limit fix - ugly as hell but appears to work
 r6710@Thesaurus (orig r6709):  ribasushi | 2009-06-18 16:13:31 +0200
 More comments
 r6717@Thesaurus (orig r6716):  ribasushi | 2009-06-19 15:39:43 +0200
 single() throws with has_many prefetch
 r6718@Thesaurus (orig r6717):  ribasushi | 2009-06-19 15:40:38 +0200
 Rename test
 r6719@Thesaurus (orig r6718):  ribasushi | 2009-06-19 15:44:26 +0200
 cleanup svn attrs
 r6720@Thesaurus (orig r6719):  ash | 2009-06-19 16:31:11 +0200
 Add extra test for prefetch+has_many

 r6721@Thesaurus (orig r6720):  ribasushi | 2009-06-19 16:33:49 +0200
 no need to slice as use_prefetch already has a limit
 r6722@Thesaurus (orig r6721):  ribasushi | 2009-06-19 16:36:08 +0200
 throw in an extra limit, sophisticate test a bit
 r6723@Thesaurus (orig r6722):  ribasushi | 2009-06-19 16:36:54 +0200
 Fix the fix
 r6725@Thesaurus (orig r6724):  ribasushi | 2009-06-19 17:24:23 +0200
 Fix dubious optimization

Changes
lib/DBIx/Class/Manual/Joining.pod
lib/DBIx/Class/ResultSet.pm
lib/DBIx/Class/Storage/DBI.pm
t/cdbi/DeepAbstractSearch/01_search.t [changed mode: 0755->0644]
t/lib/DBICTest/Schema/TwoKeys.pm [changed mode: 0755->0644]
t/prefetch/rows_bug.t [deleted file]
t/prefetch/with_limit.t [new file with mode: 0644]

diff --git a/Changes b/Changes
index dc8821f..0ff9a38 100644 (file)
--- a/Changes
+++ b/Changes
@@ -4,6 +4,7 @@ Revision history for DBIx::Class
           a resultset
         - Fixed HRI returning too many empty results on multilevel
           nonexisting prefetch
+        - Fixed the prefetch with limit bug
 
 0.08107 2009-06-14 08:21:00 (UTC)
         - Fix serialization regression introduced in 0.08103 (affects
index 700a1df..2a03c1a 100644 (file)
@@ -34,7 +34,7 @@ to fetch the tracks, or you can use a join. Compare:
 So, joins are a way of extending simple select statements to include
 fields from other, related, tables. There are various types of joins,
 depending on which combination of the data you wish to retrieve, see
-L<MySQL's doc on JOINs|http://dev.mysql.com/doc/refman/5.0/en/join.html>.
+MySQL's doc on JOINs: L<http://dev.mysql.com/doc/refman/5.0/en/join.html>.
 
 =head1 DEFINING JOINS AND RELATIONSHIPS
 
index 8ad1f7d..6faf118 100644 (file)
@@ -698,10 +698,14 @@ a warning:
 
   Query returned more than one row
 
-In this case, you should be using L</first> or L</find> instead, or if you really
+In this case, you should be using L</next> or L</find> instead, or if you really
 know what you are doing, use the L</rows> attribute to explicitly limit the size
 of the resultset.
 
+This method will also throw an exception if it is called on a resultset prefetching
+has_many, as such a prefetch implies fetching multiple rows from the database in
+order to assemble the resulting object.
+
 =back
 
 =cut
@@ -714,6 +718,12 @@ sub single {
 
   my $attrs = $self->_resolved_attrs_copy;
 
+  if (keys %{$attrs->{collapse}}) {
+    $self->throw_exception(
+      'single() can not be used on resultsets prefetching has_many. Use find( \%cond ) or next() instead'
+    );
+  }
+
   if ($where) {
     if (defined $attrs->{where}) {
       $attrs->{where} = {
@@ -2598,8 +2608,7 @@ sub _resolved_attrs {
   $attrs->{_virtual_order_by} = [ $self->result_source->primary_columns ];
 
 
-  my $collapse = $attrs->{collapse} || {};
-
+  $attrs->{collapse} ||= {};
   if ( my $prefetch = delete $attrs->{prefetch} ) {
     $prefetch = $self->_merge_attr( {}, $prefetch );
 
@@ -2608,20 +2617,20 @@ sub _resolved_attrs {
     my $join_map = $self->_joinpath_aliases ($attrs->{from}, $attrs->{seen_join});
 
     my @prefetch =
-      $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $collapse );
+      $source->_resolve_prefetch( $prefetch, $alias, $join_map, $prefetch_ordering, $attrs->{collapse} );
 
     push( @{ $attrs->{select} }, map { $_->[0] } @prefetch );
     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}} ];
   }
 
-  $attrs->{collapse} = $collapse;
-
   # 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 93ebe86..a6006dc 100644 (file)
@@ -1391,21 +1391,13 @@ sub _select_args_to_query {
 }
 
 sub _select_args {
-  my ($self, $ident, $select, $condition, $attrs) = @_;
+  my ($self, $ident, $select, $where, $attrs) = @_;
 
   my $sql_maker = $self->sql_maker;
-  $sql_maker->{for} = delete $attrs->{for};
-
-  my $order = { map
-    { $attrs->{$_} ? ( $_ => $attrs->{$_} ) : ()  }
-    (qw/order_by group_by having _virtual_order_by/ )
-  };
-
-
-  my $bind_attrs = {};
-
   my $alias2source = $self->_resolve_ident_sources ($ident);
 
+  # calculate bind_attrs before possible $ident mangling
+  my $bind_attrs = {};
   for my $alias (keys %$alias2source) {
     my $bindtypes = $self->source_bind_attributes ($alias2source->{$alias}) || {};
     for my $col (keys %$bindtypes) {
@@ -1418,15 +1410,7 @@ sub _select_args {
     }
   }
 
-  # This would be the point to deflate anything found in $condition
-  # (and leave $attrs->{bind} intact). Problem is - inflators historically
-  # expect a row object. And all we have is a resultsource (it is trivial
-  # to extract deflator coderefs via $alias2source above).
-  #
-  # I don't see a way forward other than changing the way deflators are
-  # invoked, and that's just bad...
-
-  my @args = ('select', $attrs->{bind}, $ident, $bind_attrs, $select, $condition, $order);
+  my @limit;
   if ($attrs->{software_limit} ||
       $sql_maker->_default_limit_syntax eq "GenericSubQ") {
         $attrs->{software_limit} = 1;
@@ -1436,9 +1420,165 @@ sub _select_args {
 
     # MySQL actually recommends this approach.  I cringe.
     $attrs->{rows} = 2**48 if not defined $attrs->{rows} and defined $attrs->{offset};
-    push @args, $attrs->{rows}, $attrs->{offset};
+
+    if ($attrs->{rows} && keys %{$attrs->{collapse}}) {
+      ($ident, $select, $where, $attrs)
+        = $self->_adjust_select_args_for_limited_prefetch ($ident, $select, $where, $attrs);
+    }
+    else {
+      push @limit, $attrs->{rows}, $attrs->{offset};
+    }
+  }
+
+###
+  # This would be the point to deflate anything found in $where
+  # (and leave $attrs->{bind} intact). Problem is - inflators historically
+  # expect a row object. And all we have is a resultsource (it is trivial
+  # to extract deflator coderefs via $alias2source above).
+  #
+  # I don't see a way forward other than changing the way deflators are
+  # invoked, and that's just bad...
+###
+
+  my $order = { map
+    { $attrs->{$_} ? ( $_ => $attrs->{$_} ) : ()  }
+    (qw/order_by group_by having _virtual_order_by/ )
+  };
+
+
+  $sql_maker->{for} = delete $attrs->{for};
+
+  return ('select', $attrs->{bind}, $ident, $bind_attrs, $select, $where, $order, @limit);
+}
+
+sub _adjust_select_args_for_limited_prefetch {
+  my ($self, $from, $select, $where, $attrs) = @_;
+
+  if ($attrs->{group_by} and @{$attrs->{group_by}}) {
+    $self->throw_exception ('Prefetch with limit (rows/offset) is not supported on resultsets with a group_by attribute');
   }
-  return @args;
+
+  $self->throw_exception ('Prefetch with limit (rows/offset) is not supported on resultsets with a custom from attribute')
+    if (ref $from ne 'ARRAY');
+
+  # separate attributes
+  my $sub_attrs = { %$attrs };
+  delete $attrs->{$_} for qw/where bind rows offset/;
+  delete $sub_attrs->{$_} for qw/for collapse select order_by/;
+
+  my $alias = $attrs->{alias};
+
+  # create subquery select list
+  my $sub_select = [ grep { $_ =~ /^$alias\./ } @{$attrs->{select}} ];
+
+  # bring over all non-collapse-induced order_by into the inner query (if any)
+  # the outer one will have to keep them all
+  if (my $ord_cnt = @{$attrs->{order_by}} - @{$attrs->{_collapse_order_by}} ) {
+    $sub_attrs->{order_by} = [
+      @{$attrs->{order_by}}[ 0 .. ($#{$attrs->{order_by}} - $ord_cnt - 1) ]
+    ];
+  }
+
+
+  # mangle the head of the {from}
+  my $self_ident = shift @$from;
+
+  my %join_info = map { $_->[0]{-alias} => $_->[0] } (@$from);
+
+  my (%inner_joins);
+
+  # decide which parts of the join will remain on the inside
+  #
+  # this is not a very viable optimisation, but it was written
+  # before I realised this, so might as well remain. We can throw
+  # away _any_ branches of the join tree that are:
+  # 1) not mentioned in the condition/order
+  # 2) left-join leaves (or left-join leaf chains)
+  # Most of the join ocnditions will not satisfy this, but for real
+  # complex queries some might, and we might make some RDBMS happy.
+  #
+  #
+  # since we do not have introspectable SQLA, we fall back to ugly
+  # scanning of raw SQL for WHERE, and for pieces of ORDER BY
+  # in order to determine what goes into %inner_joins
+  # It may not be very efficient, but it's a reasonable stop-gap
+  {
+    # produce stuff unquoted, so it can be scanned
+    my $sql_maker = $self->sql_maker;
+    local $sql_maker->{quote_char};
+
+    my @order_by = (map
+      { ref $_ ? $_->[0] : $_ }
+      $sql_maker->_order_by_chunks ($sub_attrs->{order_by})
+    );
+
+    my $where_sql = $sql_maker->where ($where);
+
+    # sort needed joins
+    for my $alias (keys %join_info) {
+
+      # any table alias found on a column name in where or order_by
+      # gets included in %inner_joins
+      # Also any parent joins that are needed to reach this particular alias
+      for my $piece ($where_sql, @order_by ) {
+        if ($piece =~ /\b$alias\./) {
+          $inner_joins{$alias} = 1;
+        }
+      }
+    }
+  }
+
+  # scan for non-leaf/non-left joins and mark as needed
+  # also mark all ancestor joins that are needed to reach this particular alias
+  # (e.g.  join => { cds => 'tracks' } - tracks will bring cds too )
+  #
+  # traverse by the size of the -join_path i.e. reverse depth first
+  for my $alias (sort { @{$join_info{$b}{-join_path}} <=> @{$join_info{$a}{-join_path}} } (keys %join_info) ) {
+
+    my $j = $join_info{$alias};
+    $inner_joins{$alias} = 1 if (! $j->{-join_type} || ($j->{-join_type} !~ /^left$/i) );
+
+    if ($inner_joins{$alias}) {
+      $inner_joins{$_} = 1 for (@{$j->{-join_path}});
+    }
+  }
+
+  # construct the inner $from for the subquery
+  my $inner_from = [ $self_ident ];
+  if (keys %inner_joins) {
+    for my $j (@$from) {
+      push @$inner_from, $j if $inner_joins{$j->[0]{-alias}};
+    }
+
+    # 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;
+      }
+    }
+  }
+
+  # generate the subquery
+  my $subq = $self->_select_args_to_query (
+    $inner_from,
+    $sub_select,
+    $where,
+    $sub_attrs
+  );
+
+  # put it back in $from
+  unshift @$from, { $alias => $subq };
+
+  # 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
+  #
+  # OTOH it can be seen as a plus: <ash> (notes that this query would make a DBA cry ;)
+  return ($from, $select, $where, $attrs);
 }
 
 sub _resolve_ident_sources {
old mode 100755 (executable)
new mode 100644 (file)
old mode 100755 (executable)
new mode 100644 (file)
diff --git a/t/prefetch/rows_bug.t b/t/prefetch/rows_bug.t
deleted file mode 100644 (file)
index 21bb6a2..0000000
+++ /dev/null
@@ -1,68 +0,0 @@
-# Test to ensure we get a consistent result set wether or not we use the
-# prefetch option in combination rows (LIMIT).
-use strict;
-use warnings;
-
-use Test::More;
-use lib qw(t/lib);
-use DBICTest;
-
-plan skip_all => 'fix pending';
-#plan tests => 4;
-
-my $schema = DBICTest->init_schema();
-my $no_prefetch = $schema->resultset('Artist')->search(
-  undef,
-  { rows => 3 }
-);
-
-my $use_prefetch = $schema->resultset('Artist')->search(
-  undef,
-  {
-    prefetch => 'cds',
-    rows     => 3
-  }
-);
-
-is($no_prefetch->count, $use_prefetch->count, '$no_prefetch->count == $use_prefetch->count');
-is(
-  scalar ($no_prefetch->all),
-  scalar ($use_prefetch->all),
-  "Amount of returned rows is right"
-);
-
-
-
-my $artist_many_cds = $schema->resultset('Artist')->search ( {}, {
-  join => 'cds',
-  group_by => 'me.artistid',
-  having => \ 'count(cds.cdid) > 1',
-})->first;
-
-
-$no_prefetch = $schema->resultset('Artist')->search(
-  { artistid => $artist_many_cds->id },
-  { rows => 1 }
-);
-
-$use_prefetch = $schema->resultset('Artist')->search(
-  { artistid => $artist_many_cds->id },
-  {
-    prefetch => 'cds',
-    rows     => 1
-  }
-);
-
-my $prefetch_artist = $use_prefetch->first;
-my $normal_artist = $no_prefetch->first;
-
-is(
-  $prefetch_artist->cds->count,
-  $normal_artist->cds->count,
-  "Count of child rel with prefetch + rows => 1 is right"
-);
-is (
-  scalar ($prefetch_artist->cds->all),
-  scalar ($normal_artist->cds->all),
-  "Amount of child rel rows with prefetch + rows => 1 is right"
-);
diff --git a/t/prefetch/with_limit.t b/t/prefetch/with_limit.t
new file mode 100644 (file)
index 0000000..08df104
--- /dev/null
@@ -0,0 +1,93 @@
+# Test to ensure we get a consistent result set wether or not we use the
+# prefetch option in combination rows (LIMIT).
+use strict;
+use warnings;
+
+use Test::More;
+use Test::Exception;
+use lib qw(t/lib);
+use DBICTest;
+
+plan tests => 9;
+
+my $schema = DBICTest->init_schema();
+
+
+my $no_prefetch = $schema->resultset('Artist')->search(
+  undef,
+  { rows => 3 }
+);
+
+my $use_prefetch = $schema->resultset('Artist')->search(
+  [   # search deliberately contrived
+    { 'artwork.cd_id' => undef },
+    { 'tracks.title' => { '!=' => 'blah-blah-1234568' }}
+  ],
+  {
+    prefetch => 'cds',
+    join => { cds => [qw/artwork tracks/] },
+    rows     => 3,
+    order_by => { -desc => 'name' },
+  }
+);
+
+is($no_prefetch->count, $use_prefetch->count, '$no_prefetch->count == $use_prefetch->count');
+is(
+  scalar ($no_prefetch->all),
+  scalar ($use_prefetch->all),
+  "Amount of returned rows is right"
+);
+
+my $artist_many_cds = $schema->resultset('Artist')->search ( {}, {
+  join => 'cds',
+  group_by => 'me.artistid',
+  having => \ 'count(cds.cdid) > 1',
+})->first;
+
+
+$no_prefetch = $schema->resultset('Artist')->search(
+  { artistid => $artist_many_cds->id },
+  { rows => 1 }
+);
+
+$use_prefetch = $no_prefetch->search ({}, { prefetch => 'cds' });
+
+my $normal_artist = $no_prefetch->single;
+my $prefetch_artist = $use_prefetch->find({ name => $artist_many_cds->name });
+my $prefetch2_artist = $use_prefetch->first;
+
+is(
+  $prefetch_artist->cds->count,
+  $normal_artist->cds->count,
+  "Count of child rel with prefetch + rows => 1 is right (find)"
+);
+is(
+  $prefetch2_artist->cds->count,
+  $normal_artist->cds->count,
+  "Count of child rel with prefetch + rows => 1 is right (first)"
+);
+
+is (
+  scalar ($prefetch_artist->cds->all),
+  scalar ($normal_artist->cds->all),
+  "Amount of child rel rows with prefetch + rows => 1 is right (find)"
+);
+is (
+  scalar ($prefetch2_artist->cds->all),
+  scalar ($normal_artist->cds->all),
+  "Amount of child rel rows with prefetch + rows => 1 is right (first)"
+);
+
+throws_ok (
+  sub { $use_prefetch->single },
+  qr/resultsets prefetching has_many/,
+  'single() with multiprefetch is illegal',
+);
+
+my $artist = $use_prefetch->search({'cds.title' => $artist_many_cds->cds->first->title })->next;
+is($artist->cds->count, 1, "count on search limiting prefetched has_many");
+
+# try with double limit
+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");
+