From: Peter Rabbitson Date: Tue, 5 Nov 2013 08:51:56 +0000 (+0100) Subject: Merge branch 'master' into dq2eb X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=b3577eaeda963fc6f869378cd741042ae6274666;p=dbsrgits%2FDBIx-Class.git Merge branch 'master' into dq2eb --- b3577eaeda963fc6f869378cd741042ae6274666 diff --cc lib/DBIx/Class/ResultSet.pm index bde773e,222e175..e24544c --- a/lib/DBIx/Class/ResultSet.pm +++ b/lib/DBIx/Class/ResultSet.pm @@@ -6,10 -6,10 +6,11 @@@ use base qw/DBIx::Class/ use DBIx::Class::Carp; use DBIx::Class::ResultSetColumn; use Scalar::Util qw/blessed weaken reftype/; + use DBIx::Class::_Util 'fail_on_internal_wantarray'; use Try::Tiny; use Data::Compare (); # no imports!!! guard against insane architecture - +use Data::Query::Constants; +use Data::Query::ExprHelpers; # not importing first() as it will clash with our own method use List::Util (); diff --cc lib/DBIx/Class/ResultSetColumn.pm index ed29a44,3b1f875..4f7b39e --- a/lib/DBIx/Class/ResultSetColumn.pm +++ b/lib/DBIx/Class/ResultSetColumn.pm @@@ -108,7 -110,13 +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), ++ _select => \ ($rsrc->storage->sql_maker->_render_sqla(select_select => $select) =~ /^\s*SELECT\s*(.+)/i)[0], + _as => $column, + _parent_resultset => $new_parent_rs }, $class; return $new; } diff --cc lib/DBIx/Class/Storage/DBIHacks.pm index 260360e,8a3cb02..6f3a57a --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@@ -148,15 -170,14 +172,14 @@@ sub _adjust_select_args_for_complex_pre # We will need to fetch all native columns in the inner subquery, which may # be a part of an *outer* join condition, or an order_by (which needs to be - # preserved outside) + # preserved outside), or wheres. In other words everything but the inner + # selector # We can not just fetch everything because a potential has_many restricting # join collapse *will not work* on heavy data types. - my $connecting_aliastypes = $self->_resolve_aliastypes_from_select_args( - $from, - undef, - $where, - $inner_attrs - ); + my $connecting_aliastypes = $self->_resolve_aliastypes_from_select_args({ + %$inner_attrs, - select => [], ++ select => undef, + }); for (sort map { keys %{$_->{-seen_columns}||{}} } map { values %$_ } values %$connecting_aliastypes) { my $ci = $colinfo->{$_} or next; @@@ -491,20 -416,11 +422,20 @@@ sub _resolve_aliastypes_from_select_arg # generate sql chunks my $to_scan = { restricting => [ - ($where - ? ($sql_maker->_recurse_where($where))[0] - $sql_maker->_recurse_where ($attrs->{where}), - $sql_maker->_parse_rs_attrs ({ having => $attrs->{having} }), ++ ($attrs->{where} ++ ? ($sql_maker->_recurse_where($attrs->{where}))[0] + : () + ), + ($attrs->{having} + ? ($sql_maker->_recurse_where($attrs->{having}))[0] + : () + ), ], grouping => [ - $sql_maker->_parse_rs_attrs ({ group_by => $attrs->{group_by} }), + ($attrs->{group_by} + ? ($sql_maker->_render_sqla(group_by => $attrs->{group_by}))[0] + : (), + ) ], joining => [ $sql_maker->_recurse_from ( @@@ -513,9 -429,7 +444,9 @@@ ), ], selecting => [ - ($select - ? ($sql_maker->_render_sqla(select_select => $select))[0] - $sql_maker->_recurse_fields ($attrs->{select}), ++ ($attrs->{select} ++ ? ($sql_maker->_render_sqla(select_select => $attrs->{select}))[0] + : ()), ], ordering => [ map { $_->[0] } $self->_extract_order_criteria ($attrs->{order_by}, $sql_maker), @@@ -617,36 -528,116 +549,133 @@@ sub _group_over_selection } } - # add any order_by parts *from the main source* that are not already - # present in the group_by - # we need to be careful not to add any named functions/aggregates - # i.e. order_by => [ ... { count => 'foo' } ... ] - my @leftovers; - for ($self->_extract_order_criteria($attrs->{order_by})) { - my @order_by = $self->_extract_order_criteria($attrs->{order_by}) ++ my $sql_maker = $self->sql_maker; ++ my @order_by = $self->_extract_order_criteria($attrs->{order_by}, $sql_maker) + or return (\@group_by, $attrs->{order_by}); + + # add any order_by parts that are not already present in the group_by + # to maintain SQL cross-compatibility and general sanity + # + # also in case the original selection is *not* unique, or in case part + # of the ORDER BY refers to a multiplier - we will need to replace the + # skipped order_by elements with their MIN/MAX equivalents as to maintain + # the proper overall order without polluting the group criteria (and + # possibly changing the outcome entirely) + - my ($leftovers, $sql_maker, @new_order_by, $order_chunks, $aliastypes); ++ my ($leftovers, @new_order_by, $order_chunks, $aliastypes); + + my $group_already_unique = $self->_columns_comprise_identifying_set($colinfos, \@group_by); + + for my $o_idx (0 .. $#order_by) { + + # if the chunk is already a min/max function - there is nothing left to touch + next if $order_by[$o_idx][0] =~ /^ (?: min | max ) \s* \( .+ \) $/ix; + # only consider real columns (for functions the user got to do an explicit group_by) - if (@$_ != 1) { - push @leftovers, $_; - next; + my $chunk_ci; + if ( + @{$order_by[$o_idx]} != 1 + or + # only declare an unknown *plain* identifier as "leftover" if we are called with + # aliastypes to examine. If there are none - we are still in _resolve_attrs, and + # can just assume the user knows what they want + ( ! ( $chunk_ci = $colinfos->{$order_by[$o_idx][0]} ) and $attrs->{_aliastypes} ) + ) { + push @$leftovers, $order_by[$o_idx][0]; } - my $chunk = $_->[0]; - if ( - !$colinfos->{$chunk} + next unless $chunk_ci; + + # no duplication of group criteria + next if $group_index{$chunk_ci->{-fq_colname}}; + + $aliastypes ||= ( + $attrs->{_aliastypes} or - $colinfos->{$chunk}{-source_alias} ne $attrs->{alias} + $self->_resolve_aliastypes_from_select_args({ + from => $attrs->{from}, + order_by => $attrs->{order_by}, + }) + ) if $group_already_unique; + + # check that we are not ordering by a multiplier (if a check is requested at all) + if ( + $group_already_unique + and + ! $aliastypes->{multiplying}{$chunk_ci->{-source_alias}} + and + ! $aliastypes->{premultiplied}{$chunk_ci->{-source_alias}} ) { - push @leftovers, $_; - next; + push @group_by, $chunk_ci->{-fq_colname}; + $group_index{$chunk_ci->{-fq_colname}}++ } + else { + # We need to order by external columns without adding them to the group + # (eiehter a non-unique selection, or a multi-external) + # + # This doesn't really make sense in SQL, however from DBICs point + # of view is rather valid (e.g. order the leftmost objects by whatever + # criteria and get the offset/rows many). There is a way around + # this however in SQL - we simply tae the direction of each piece + # of the external order and convert them to MIN(X) for ASC or MAX(X) + # for DESC, and group_by the root columns. The end result should be + # exactly what we expect + + # FIXME - this code is a joke, will need to be completely rewritten in + # the DQ branch. But I need to push a POC here, otherwise the + # pesky tests won't pass + # wrap any part of the order_by that "responds" to an ordering alias + # into a MIN/MAX - $sql_maker ||= $self->sql_maker; - $order_chunks ||= [ - map { ref $_ eq 'ARRAY' ? $_ : [ $_ ] } $sql_maker->_order_by_chunks($attrs->{order_by}) - ]; + - my ($chunk, $is_desc) = $sql_maker->_split_order_chunk($order_chunks->[$o_idx][0]); ++ $order_chunks ||= do { ++ my @c; ++ my $dq_node = $sql_maker->converter->_order_by_to_dq($attrs->{order_by}); + - $new_order_by[$o_idx] = \[ - sprintf( '%s( %s )%s', - ($is_desc ? 'MAX' : 'MIN'), - $chunk, - ($is_desc ? ' DESC' : ''), - ), - @ {$order_chunks->[$o_idx]} [ 1 .. $#{$order_chunks->[$o_idx]} ] - ]; ++ while (is_Order($dq_node)) { ++ push @c, { ++ is_desc => $dq_node->{reverse}, ++ dq_node => $dq_node->{by}, ++ }; ++ ++ @{$c[-1]}{qw(sql bind)} = $sql_maker->_render_dq($dq_node->{by}); ++ ++ $dq_node = $dq_node->{from}; ++ } + - $chunk = $colinfos->{$chunk}{-fq_colname}; - push @group_by, $chunk unless $group_index{$chunk}++; ++ \@c; ++ }; ++ ++ $new_order_by[$o_idx] = { ++ ($order_chunks->[$o_idx]{is_desc} ? '-desc' : '-asc') => \[ ++ sprintf ( '%s( %s )', ++ ($order_chunks->[$o_idx]{is_desc} ? 'MAX' : 'MIN'), ++ $order_chunks->[$o_idx]{sql}, ++ ), ++ @{ $order_chunks->[$o_idx]{bind} || [] } ++ ] ++ }; + } } - return wantarray - ? (\@group_by, (@leftovers ? \@leftovers : undef) ) - : \@group_by - ; + $self->throw_exception ( sprintf + 'A required group_by clause could not be constructed automatically due to a complex ' + . 'order_by criteria (%s). Either order_by columns only (no functions) or construct a suitable ' + . 'group_by by hand', + join ', ', map { "'$_'" } @$leftovers, + ) if $leftovers; + + # recreate the untouched order parts + if (@new_order_by) { - $new_order_by[$_] ||= \ $order_chunks->[$_] for ( 0 .. $#$order_chunks ); ++ $new_order_by[$_] ||= { ++ ( $order_chunks->[$_]{is_desc} ? '-desc' : '-asc' ) ++ => \ $order_chunks->[$_]{dq_node} ++ } for ( 0 .. $#$order_chunks ); + } + + return ( + \@group_by, + (@new_order_by ? \@new_order_by : $attrs->{order_by} ), # same ref as original == unchanged + ); } sub _resolve_ident_sources { @@@ -839,15 -849,25 +868,25 @@@ sub _extract_order_criteria sub _order_by_is_stable { my ($self, $ident, $order_by, $where) = @_; - my $colinfo = $self->_resolve_column_info($ident, [ + my @cols = ( - (map { $_->[0] } $self->_extract_order_criteria($order_by)), + (map { $_->[0] } $self->_extract_order_criteria($order_by, undef, 1)), $where ? @{$self->_extract_fixed_condition_columns($where)} :(), - ]); + ) or return undef; + + my $colinfo = $self->_resolve_column_info($ident, \@cols); + + return keys %$colinfo + ? $self->_columns_comprise_identifying_set( $colinfo, \@cols ) + : undef + ; + } - return undef unless keys %$colinfo; + sub _columns_comprise_identifying_set { + my ($self, $colinfo, $columns) = @_; my $cols_per_src; - $cols_per_src->{$_->{-source_alias}}{$_->{-colname}} = $_ for values %$colinfo; + $cols_per_src -> {$_->{-source_alias}} -> {$_->{-colname}} = $_ + for grep { defined $_ } @{$colinfo}{@$columns}; for (values %$cols_per_src) { my $src = (values %$_)[0]->{-result_source}; diff --cc t/lib/DBICTest.pm index 0446830,8988db9..1ed8894 --- a/t/lib/DBICTest.pm +++ b/t/lib/DBICTest.pm @@@ -4,7 -4,38 +4,40 @@@ package # hide from PAUS use strict; use warnings; +use lib 't/dqlib'; ++ + # this noop trick initializes the STDOUT, so that the TAP::Harness + # issued IO::Select->can_read calls (which are blocking wtf wtf wtf) + # keep spinning and scheduling jobs + # This results in an overall much smoother job-queue drainage, since + # the Harness blocks less + # (ideally this needs to be addressed in T::H, but a quick patchjob + # broke everything so tabling it for now) + BEGIN { + if ($INC{'Test/Builder.pm'}) { + local $| = 1; + print "#\n"; + } + } + + use Module::Runtime 'module_notional_filename'; + BEGIN { + for my $mod (qw( DBIC::SqlMakerTest SQL::Abstract )) { + if ( $INC{ module_notional_filename($mod) } ) { + # FIXME this does not seem to work in BEGIN - why?! + #require Carp; + #$Carp::Internal{ (__PACKAGE__) }++; + #Carp::croak( __PACKAGE__ . " must be loaded before $mod" ); + + my ($fr, @frame) = 1; + while (@frame = caller($fr++)) { + last if $frame[1] !~ m|^t/lib/DBICTest|; + } + + die __PACKAGE__ . " must be loaded before $mod (or modules using $mod) at $frame[1] line $frame[2]\n"; + } + } + } use DBICTest::RunMode; use DBICTest::Schema; diff --cc t/prefetch/o2m_o2m_order_by_with_limit.t index bc08382,b8a4477..a9326cd --- a/t/prefetch/o2m_o2m_order_by_with_limit.t +++ b/t/prefetch/o2m_o2m_order_by_with_limit.t @@@ -100,10 -100,10 +100,10 @@@ for LEFT JOIN "track" "tracks" ON "tracks"."cd" = "cds_unordered"."cdid" WHERE "me"."rank" = ? - GROUP BY "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track" + GROUP BY "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track", "me"."name" ORDER BY MAX("genre"."name") DESC, - MAX( tracks.title ) DESC, + MAX("tracks"."title") DESC, - MIN("me"."name"), + "me"."name" ASC, "year" DESC, "cds_unordered"."title" DESC LIMIT ? diff --cc xt/standalone_testschema_resultclasses.t index 1e2aa4a,38278c0..c9e218d --- a/xt/standalone_testschema_resultclasses.t +++ b/xt/standalone_testschema_resultclasses.t @@@ -4,8 -4,9 +4,10 @@@ use strict use Test::More; use File::Find; + use DBIx::Class::_Util 'sigwarn_silencer'; + use lib 't/lib'; +use lib 't/dqlib'; find({ wanted => sub {