Clarify what happens on distinct + get_column($aggregate_alias)

diff --git a/Changes b/Changes
index 0ab5d97..010fdde 100644 (file)
--- a/Changes
+++ b/Changes
@@ -9,6 +9,8 @@ Revision history for DBIx::Class
           http://lists.scsys.co.uk/pipermail/dbix-class/2013-June/011374.html)
         - Fix multiple edge cases steming from interaction of a non-selecting
           order_by specification and distinct and/or complex prefetch
+        - Clarify ambiguous behavior of distinct when used with ResultSetColumn
+          i.e. $rs->search({}, { distinct => 1 })->get_column (...)
         - Setting quote_names propagates to SQL::Translator when producing
           SQLite DDL (it is one of the few producers *NOT* quoting by default)
         - Fix incorrect binding of large integers on old versions of

diff --git a/lib/DBIx/Class/ResultSetColumn.pm b/lib/DBIx/Class/ResultSetColumn.pm
index 40cf73e..3b1f875 100644 (file)
--- a/lib/DBIx/Class/ResultSetColumn.pm
+++ b/lib/DBIx/Class/ResultSetColumn.pm
@@ -59,7 +59,7 @@ sub new {
   my $as_index = List::Util::first { ($as_list->[$_] || "") eq $column } 0..$#$as_list;
   my $select = defined $as_index ? $select_list->[$as_index] : $column;
 
-  my ($new_parent_rs, $colmap);
+  my $colmap;
   for ($rsrc->columns, $column) {
     if ($_ =~ /^ \Q$alias\E \. ([^\.]+) $ /x) {
       $colmap->{$_} = $1;
@@ -70,6 +70,7 @@ sub new {
     }
   }
 
+  my $new_parent_rs;
   # analyze the order_by, and see if it is done over a function/nonexistentcolumn
   # if this is the case we will need to wrap a subquery since the result of RSC
   # *must* be a single column select
@@ -79,7 +80,7 @@ sub new {
       ( $rsrc->schema->storage->_extract_order_criteria ($orig_attrs->{order_by} ) )
   ) {
     # nuke the prefetch before collapsing to sql
-    my $subq_rs = $rs->search;
+    my $subq_rs = $rs->search_rs;
     $subq_rs->{attrs}{join} = $subq_rs->_merge_joinpref_attr( $subq_rs->{attrs}{join}, delete $subq_rs->{attrs}{prefetch} );
     $new_parent_rs = $subq_rs->as_subselect_rs;
   }
@@ -109,7 +110,13 @@ sub new {
     }
   }
 
-  my $new = bless { _select => $select, _as => $column, _parent_resultset => $new_parent_rs }, $class;
+  # collapse the selector to a literal so that it survives a possible distinct parse
+  # if it turns out to be an aggregate - at least the user will get a proper exception
+  # instead of silent drop of the group_by altogether
+  my $new = bless {
+    _select => \ $rsrc->storage->sql_maker->_recurse_fields($select),
+    _as => $column,
+    _parent_resultset => $new_parent_rs }, $class;
   return $new;
 }
 

diff --git a/t/88result_set_column.t b/t/88result_set_column.t
index 6eb9892..cc6f68b 100644 (file)
--- a/t/88result_set_column.t
+++ b/t/88result_set_column.t
@@ -217,7 +217,7 @@ is_same_sql_bind (
   'Correct SQL for prefetch/order_by/group_by'
 );
 
-# test aggregate on a function
+# test aggregate on a function (create an extra track on one cd)
 {
   my $tr_rs = $schema->resultset("Track");
   $tr_rs->create({ cd => 2, title => 'dealbreaker' });
@@ -242,4 +242,68 @@ is_same_sql_bind (
   );
 }
 
+# test exotic scenarious (create a track-less cd)
+# "How many CDs (not tracks) have been released per year where a given CD has at least one track and the artist isn't evancarroll?"
+{
+
+  $schema->resultset('CD')->create({ artist => 1, title => 'dealbroker no tracks', year => 2001 });
+
+  my $rs = $schema->resultset ('CD')->search (
+    { 'artist.name' => { '!=', 'evancarrol' }, 'tracks.trackid' => { '!=', undef } },
+    {
+      order_by => 'me.year',
+      join => [qw(artist tracks)],
+      columns => [ 'year', { cnt => { count => 'me.cdid' }} ],
+    },
+  );
+
+  my $rstypes = {
+    'explicitly grouped' => $rs->search_rs({}, { group_by => 'year' }),
+    'implicitly grouped' => $rs->search_rs({}, { distinct => 1 }),
+  };
+
+  for my $type (keys %$rstypes) {
+    is ($rstypes->{$type}->count, 4, "correct cd count with $type column");
+
+    is_deeply (
+      [ $rstypes->{$type}->get_column ('year')->all ],
+      [qw(1997 1998 1999 2001)],
+      "Getting $type column works",
+    );
+  }
+
+  # Why do we test this - we want to make sure that the selector *will* actually make
+  # it to the group_by as per the distinct => 1 contract. Before 0.08251 this situation
+  # would silently drop the group_by entirely, likely ending up with nonsensival results
+  # With the current behavior the user will at least get a nice fat exception from the
+  # RDBMS (or maybe the RDBMS will even decide to handle the situation sensibly...)
+  is_same_sql_bind(
+    $rstypes->{'implicitly grouped'}->get_column('cnt')->as_query,
+    '(
+      SELECT COUNT( me.cdid )
+        FROM cd me
+        JOIN artist artist
+          ON artist.artistid = me.artist
+        LEFT JOIN track tracks
+          ON tracks.cd = me.cdid
+      WHERE artist.name != ? AND tracks.trackid IS NOT NULL
+      GROUP BY COUNT( me.cdid )
+      ORDER BY MIN(me.year)
+    )',
+    [ [ { dbic_colname => 'artist.name', sqlt_datatype => 'varchar', sqlt_size => 100 }
+        => 'evancarrol'
+    ] ],
+    'Expected (though nonsensical) SQL generated on rscol-with-distinct-over-function',
+  );
+
+  {
+    local $TODO = 'multiplying join leaks through to the count aggregate... this may never actually work';
+    is_deeply (
+      [ $rstypes->{'explicitly grouped'}->get_column ('cnt')->all ],
+      [qw(1 1 1 2)],
+      "Get aggregate over group works",
+    );
+  }
+}
+
 done_testing;

diff --git a/t/search/subquery.t b/t/search/subquery.t
index a281fe9..382a46d 100644 (file)
--- a/t/search/subquery.t
+++ b/t/search/subquery.t
@@ -46,7 +46,7 @@ my @tests = (
       ],
     },
     sqlbind => \[
-      "( SELECT (SELECT me.id FROM cd me LIMIT ?) FROM artist me )",
+      "( SELECT (SELECT id FROM cd me LIMIT ?) FROM artist me )",
       [ $ROWS => 1 ],
     ],
   },
@@ -59,7 +59,7 @@ my @tests = (
       ],
     },
     sqlbind => \[
-      "( SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT me.id FROM cd me LIMIT ?) FROM artist me )",
+      "( SELECT me.artistid, me.name, me.rank, me.charfield, (SELECT id FROM cd me LIMIT ?) FROM artist me )",
       [ $ROWS => 1 ],
     ],
   },