From: Peter Rabbitson Date: Mon, 15 Feb 2010 18:45:05 +0000 (+0000) Subject: Clarify what happens on distinct + get_column($aggregate_alias) X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=aca094b4dff8c1b930a52a56853e06b65b508032;p=dbsrgits%2FDBIx-Class-Historic.git Clarify what happens on distinct + get_column($aggregate_alias) --- diff --git a/Changes b/Changes index 0ab5d97..010fdde 100644 --- 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 --- 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 --- 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 --- 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 ], ], },