From: Peter Rabbitson <ribasushi@cpan.org>
Date: Thu, 29 Mar 2012 02:53:28 +0000 (+0200)
Subject: Simplify skimming limits - simpler query when no offset is given
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=a66b662c77c825952732b8ff4af9d0d1a2ee996d;p=dbsrgits%2FDBIx-Class-Historic.git

Simplify skimming limits - simpler query when no offset is given
---

diff --git a/lib/DBIx/Class/SQLMaker/LimitDialects.pm b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
index 5af9487..dd6bc6d 100644
--- a/lib/DBIx/Class/SQLMaker/LimitDialects.pm
+++ b/lib/DBIx/Class/SQLMaker/LimitDialects.pm
@@ -312,87 +312,93 @@ sub _prep_for_skimming_limit {
   $sq_attrs->{order_by_requested} = $self->_order_by ($requested_order);
   $sq_attrs->{grpby_having} = $self->_parse_rs_attrs ($rs_attrs);
 
-  # make up an order unless supplied or sanity check what we are given
-  my $inner_order;
-  if ($sq_attrs->{order_by_requested}) {
-    $self->throw_exception (
-      'Unable to safely perform "skimming type" limit with supplied unstable order criteria'
-    ) unless $rs_attrs->{_rsroot_rsrc}->schema->storage->_order_by_is_stable(
-      $rs_attrs->{from},
-      $requested_order
-    );
-
-    $inner_order = $requested_order;
+  # without an offset things are easy
+  if (! $rs_attrs->{offset}) {
+    $sq_attrs->{order_by_inner} = $sq_attrs->{order_by_requested};
   }
   else {
-    $inner_order = [ map
-      { "$rs_attrs->{alias}.$_" }
-      ( @{
-        $rs_attrs->{_rsroot_rsrc}->_identifying_column_set
-          ||
-        $self->throw_exception(sprintf(
-          'Unable to auto-construct stable order criteria for "skimming type" limit '
-        . "dialect based on source '%s'", $rs_attrs->{_rsroot_rsrc}->name) );
-      } )
-    ];
-  }
-
-  # localise as we already have all the bind values we need
-  local $self->{order_bind};
-
-  $sq_attrs->{order_by_inner} = $self->_order_by ($inner_order);
-
-  my @out_chunks;
-  for my $ch ($self->_order_by_chunks ($inner_order)) {
-    $ch = $ch->[0] if ref $ch eq 'ARRAY';
+    $sq_attrs->{quoted_rs_alias} = $self->_quote ($rs_attrs->{alias});
+
+    # localise as we already have all the bind values we need
+    local $self->{order_bind};
+
+    # make up an order unless supplied or sanity check what we are given
+    my $inner_order;
+    if ($sq_attrs->{order_by_requested}) {
+      $self->throw_exception (
+        'Unable to safely perform "skimming type" limit with supplied unstable order criteria'
+      ) unless $rs_attrs->{_rsroot_rsrc}->schema->storage->_order_by_is_stable(
+        $rs_attrs->{from},
+        $requested_order
+      );
 
-    $ch =~ s/\s+ ( ASC|DESC ) \s* $//ix;
-    my $dir = uc ($1||'ASC');
+      $inner_order = $requested_order;
+    }
+    else {
+      $inner_order = [ map
+        { "$rs_attrs->{alias}.$_" }
+        ( @{
+          $rs_attrs->{_rsroot_rsrc}->_identifying_column_set
+            ||
+          $self->throw_exception(sprintf(
+            'Unable to auto-construct stable order criteria for "skimming type" limit '
+          . "dialect based on source '%s'", $rs_attrs->{_rsroot_rsrc}->name) );
+        } )
+      ];
+    }
 
-    push @out_chunks, \join (' ', $ch, $dir eq 'ASC' ? 'DESC' : 'ASC' );
-  }
+    $sq_attrs->{order_by_inner} = $self->_order_by ($inner_order);
 
-  $sq_attrs->{quoted_rs_alias} = $self->_quote ($rs_attrs->{alias});
-  $sq_attrs->{order_by_middle} = $self->_order_by (\@out_chunks);
-  $sq_attrs->{selection_middle} = $sq_attrs->{selection_outer};
+    my @out_chunks;
+    for my $ch ($self->_order_by_chunks ($inner_order)) {
+      $ch = $ch->[0] if ref $ch eq 'ARRAY';
 
-  # this is the order supplement magic
-  if (my $extra_order_sel = $sq_attrs->{order_supplement}) {
-    for my $extra_col (sort
-      { $extra_order_sel->{$a} cmp $extra_order_sel->{$b} }
-      keys %$extra_order_sel
-    ) {
-      $sq_attrs->{selection_inner} .= sprintf (', %s AS %s',
-        $extra_col,
-        $extra_order_sel->{$extra_col},
-      );
-
-      $sq_attrs->{selection_middle} .= ', ' . $extra_order_sel->{$extra_col};
+      $ch =~ s/\s+ ( ASC|DESC ) \s* $//ix;
+      my $dir = uc ($1||'ASC');
+      push @out_chunks, \join (' ', $ch, $dir eq 'ASC' ? 'DESC' : 'ASC' );
     }
 
-    # Whatever order bindvals there are, they will be realiased and
-    # reselected, and need to show up at end of the initial inner select
-    push @{$self->{select_bind}}, @{$self->{order_bind}};
-
-    # if this is a part of something bigger, we need to add back all
-    # the extra order_by's, as they may be relied upon by the outside
-    # of a prefetch or something
-    if ($rs_attrs->{_is_internal_subuery}) {
-      $sq_attrs->{selection_outer} .= sprintf ", $extra_order_sel->{$_} AS $_"
-        for sort
-          { $extra_order_sel->{$a} cmp $extra_order_sel->{$b} }
-            grep { $_ !~ /[^\w\-]/ }  # ignore functions
-            keys %$extra_order_sel
-      ;
+    $sq_attrs->{order_by_middle} = $self->_order_by (\@out_chunks);
+
+    # this is the order supplement magic
+    $sq_attrs->{selection_middle} = $sq_attrs->{selection_outer};
+    if (my $extra_order_sel = $sq_attrs->{order_supplement}) {
+      for my $extra_col (sort
+        { $extra_order_sel->{$a} cmp $extra_order_sel->{$b} }
+        keys %$extra_order_sel
+      ) {
+        $sq_attrs->{selection_inner} .= sprintf (', %s AS %s',
+          $extra_col,
+          $extra_order_sel->{$extra_col},
+        );
+
+        $sq_attrs->{selection_middle} .= ', ' . $extra_order_sel->{$extra_col};
+      }
+
+      # Whatever order bindvals there are, they will be realiased and
+      # reselected, and need to show up at end of the initial inner select
+      push @{$self->{select_bind}}, @{$self->{order_bind}};
+
+      # if this is a part of something bigger, we need to add back all
+      # the extra order_by's, as they may be relied upon by the outside
+      # of a prefetch or something
+      if ($rs_attrs->{_is_internal_subuery}) {
+        $sq_attrs->{selection_outer} .= sprintf ", $extra_order_sel->{$_} AS $_"
+          for sort
+            { $extra_order_sel->{$a} cmp $extra_order_sel->{$b} }
+              grep { $_ !~ /[^\w\-]/ }  # ignore functions
+              keys %$extra_order_sel
+        ;
+      }
     }
-  }
 
-  # and this is order re-alias magic
-  for my $map ($sq_attrs->{order_supplement}, $sq_attrs->{outer_renames}) {
-    for my $col (sort { $map->{$a} cmp $map->{$b} } keys %{$map||{}}) {
-      my $re_col = quotemeta ($col);
-      $_ =~ s/$re_col/$map->{$col}/
-        for ($sq_attrs->{order_by_middle}, $sq_attrs->{order_by_requested});
+    # and this is order re-alias magic
+    for my $map ($sq_attrs->{order_supplement}, $sq_attrs->{outer_renames}) {
+      for my $col (sort { $map->{$a} cmp $map->{$b} } keys %{$map||{}}) {
+        my $re_col = quotemeta ($col);
+        $_ =~ s/$re_col/$map->{$col}/
+          for ($sq_attrs->{order_by_middle}, $sq_attrs->{order_by_requested});
+      }
     }
   }
 
@@ -425,7 +431,7 @@ sub _Top {
 
   $sql = sprintf ('SELECT TOP %u %s %s %s %s',
     $rows + ($offset||0),
-    $lim->{selection_inner},
+    $offset ? $lim->{selection_inner} : $lim->{selection_original},
     $lim->{query_leftover},
     $lim->{grpby_having},
     $lim->{order_by_inner},
@@ -480,7 +486,7 @@ sub _FetchFirst {
   my $lim = $self->_prep_for_skimming_limit($sql, $rs_attrs);
 
   $sql = sprintf ('SELECT %s %s %s %s FETCH FIRST %u ROWS ONLY',
-    $lim->{selection_inner},
+    $offset ? $lim->{selection_inner} : $lim->{selection_original},
     $lim->{query_leftover},
     $lim->{grpby_having},
     $lim->{order_by_inner},
@@ -711,6 +717,8 @@ sub _subqueried_limit_attrs {
   # for possible further chaining)
   my ($sel, $renamed);
   for my $node (@sel) {
+    push @{$sel->{original}}, $node->{sql};
+
     if (
       $node->{as} =~ / (?<! ^ $re_alias ) \. /x
         or
diff --git a/t/sqlmaker/limit_dialects/fetch_first.t b/t/sqlmaker/limit_dialects/fetch_first.t
index 7a282e9..3616d6c 100644
--- a/t/sqlmaker/limit_dialects/fetch_first.t
+++ b/t/sqlmaker/limit_dialects/fetch_first.t
@@ -191,12 +191,10 @@ my $rs_selectas_top = $schema->resultset ('BooksInLibrary')->search ({}, {
 
 is_same_sql_bind( $rs_selectas_top->search({})->as_query,
                   '(SELECT
-                      me.id, me.source, me.owner, me.title, me.price,
-                      owner.name AS owner_name
+                      me.id, me.source, me.owner, me.title, me.price, owner.name
                     FROM books me
                     JOIN owners owner ON owner.id = me.owner
                     WHERE ( source = ? )
-                    ORDER BY me.id
                     FETCH FIRST 1 ROWS ONLY
                    )',
                   [ [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' }
diff --git a/t/sqlmaker/limit_dialects/toplimit.t b/t/sqlmaker/limit_dialects/toplimit.t
index 61b5498..31a8b09 100644
--- a/t/sqlmaker/limit_dialects/toplimit.t
+++ b/t/sqlmaker/limit_dialects/toplimit.t
@@ -231,11 +231,10 @@ my $rs_selectas_top = $schema->resultset ('BooksInLibrary')->search ({}, {
 is_same_sql_bind( $rs_selectas_top->search({})->as_query,
                   '(SELECT
                       TOP 1 me.id, me.source, me.owner, me.title, me.price,
-                      owner.name AS owner_name
+                      owner.name
                     FROM books me
                     JOIN owners owner ON owner.id = me.owner
                     WHERE ( source = ? )
-                    ORDER BY me.id
                   )',
                   [ [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' }
                     => 'Library' ] ],
@@ -265,7 +264,7 @@ my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search( { -exists =>
 
 is_same_sql_bind(
   $rs_selectas_rel->as_query,
-  '(SELECT TOP 1 me.id, me.owner  FROM books me WHERE ( ( (EXISTS (SELECT COUNT( * ) FROM owners owner WHERE ( books.owner = owner.id ))) AND source = ? ) )   ORDER BY me.id)',
+  '(SELECT TOP 1 me.id, me.owner  FROM books me WHERE ( ( (EXISTS (SELECT COUNT( * ) FROM owners owner WHERE ( books.owner = owner.id ))) AND source = ? ) ) )',
   [
     [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ],
   ],