From: Peter Rabbitson Date: Sat, 23 Mar 2013 11:02:44 +0000 (+0100) Subject: Overhaul GenericSubq limit - add support for multicol order X-Git-Tag: v0.08250~31^2~10 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=318e3d94c5332d5af335706b26fc5b6f6fc5c703;p=dbsrgits%2FDBIx-Class.git Overhaul GenericSubq limit - add support for multicol order A lot of other cleanup as well - this should be the end of it ;) --- diff --git a/Changes b/Changes index 4cb852a..16f2251 100644 --- a/Changes +++ b/Changes @@ -6,6 +6,7 @@ Revision history for DBIx::Class - Fix update/delete operations on resultsets *joining* the updated table failing on MySQL. Resolves oversights in the fixes for RT#81378 and RT#81897 + - Even more robust behavior of GenericSubQuery limit dialect - Stop Sybase ASE storage from generating invalid SQL in subselects when a limit without offset is encountered - Correctly recognize root source unqualified columns when diff --git a/lib/DBIx/Class/SQLMaker/LimitDialects.pm b/lib/DBIx/Class/SQLMaker/LimitDialects.pm index a5ac467..1f06377 100644 --- a/lib/DBIx/Class/SQLMaker/LimitDialects.pm +++ b/lib/DBIx/Class/SQLMaker/LimitDialects.pm @@ -535,60 +535,110 @@ sub _GenericSubQ { my ($self, $sql, $rs_attrs, $rows, $offset) = @_; my $root_rsrc = $rs_attrs->{_rsroot_rsrc}; - my $root_tbl_name = $root_rsrc->name; - my ($first_order_by) = do { + # Explicitly require an order_by + # GenSubQ is slow enough as it is, just emulating things + # like in other cases is not wise - make the user work + # to shoot their DBA in the foot + my $supplied_order = delete $rs_attrs->{order_by} or $self->throw_exception ( + 'Generic Subquery Limit does not work on resultsets without an order. Provide a stable, ' + . 'root-table-based order criteria.' + ); + + my $usable_order_ci = $root_rsrc->storage->_main_source_order_by_portion_is_stable( + $root_rsrc, + $supplied_order, + $rs_attrs->{where}, + ) or $self->throw_exception( + 'Generic Subquery Limit can not work with order criteria based on sources other than the current one' + ); + +### +### +### we need to know the directions after we figured out the above - reextract *again* +### this is eyebleed - trying to get it to work at first + my @order_bits = do { local $self->{quote_char}; local $self->{order_bind}; - map { ref $_ ? $_->[0] : $_ } $self->_order_by_chunks ($rs_attrs->{order_by}) - } or $self->throw_exception ( - 'Generic Subquery Limit does not work on resultsets without an order. Provide a single, ' - . 'unique-column order criteria.' - ); + map { ref $_ ? $_->[0] : $_ } $self->_order_by_chunks ($supplied_order) + }; - my $direction = ( - $first_order_by =~ s/\s+ ( ASC|DESC ) \s* $//ix - ) ? lc($1) : 'asc'; + # truncate to what we'll use + $#order_bits = ( (keys %$usable_order_ci) - 1 ); - my ($first_ord_alias, $first_ord_col) = $first_order_by =~ /^ (?: ([^\.]+) \. )? ([^\.]+) $/x; + # @order_bits likely will come back quoted (due to how the prefetch + # rewriter operates + # Hence supplement the column_info lookup table with quoted versions + if ($self->quote_char) { + $usable_order_ci->{$self->_quote($_)} = $usable_order_ci->{$_} + for keys %$usable_order_ci; + } + +# calculate the condition + my $count_tbl_alias = 'rownum__emulation'; + my $root_alias = $rs_attrs->{alias}; + my $root_tbl_name = $root_rsrc->name; - $self->throw_exception(sprintf - "Generic Subquery Limit order criteria can be only based on the root-source '%s'" - . " (aliased as '%s')", $root_rsrc->source_name, $rs_attrs->{alias}, - ) if ($first_ord_alias and $first_ord_alias ne $rs_attrs->{alias}); + my (@unqualified_names, @qualified_names, @is_desc, @new_order_by); - $first_ord_alias ||= $rs_attrs->{alias}; + for my $bit (@order_bits) { - $self->throw_exception( - "Generic Subquery Limit first order criteria '$first_ord_col' must be unique" - ) unless $root_rsrc->_identifying_column_set([$first_ord_col]); - - my $sq_attrs = do { - # perform the mangling only using the very first order crietria - # (the one we care about) - local $rs_attrs->{order_by} = $first_order_by; - $self->_subqueried_limit_attrs ($sql, $rs_attrs); - }; + my $is_desc = ( + $bit =~ s/\s+ ( ASC|DESC ) \s* $//ix + and + uc($1) eq 'DESC' + ) ? 1 : 0; - my $cmp_op = $direction eq 'desc' ? '>' : '<'; - my $count_tbl_alias = 'rownum__emulation'; + push @is_desc, $is_desc; + push @unqualified_names, $usable_order_ci->{$bit}{-colname}; + push @qualified_names, $usable_order_ci->{$bit}{-fq_colname}; - my ($order_sql, @order_bind) = do { - local $self->{order_bind}; - my $s = $self->_order_by (delete $rs_attrs->{order_by}); - ($s, @{$self->{order_bind}}); + push @new_order_by, { ($is_desc ? '-desc' : '-asc') => $usable_order_ci->{$bit}{-fq_colname} }; }; - my $group_having_sql = $self->_parse_rs_attrs($rs_attrs); - my $in_sel = $sq_attrs->{selection_inner}; + my (@where_cond, @skip_colpair_stack); + for my $i (0 .. $#order_bits) { + my $ci = $usable_order_ci->{$order_bits[$i]}; + + my ($subq_col, $main_col) = map { "$_.$ci->{-colname}" } ($count_tbl_alias, $root_alias); + my $cur_cond = { $subq_col => { ($is_desc[$i] ? '>' : '<') => { -ident => $main_col } } }; + + push @skip_colpair_stack, [ + { $main_col => { -ident => $subq_col } }, + ]; + + # we can trust the nullability flag because + # we already used it during _id_col_set resolution + # + if ($ci->{is_nullable}) { + push @{$skip_colpair_stack[-1]}, { $main_col => undef, $subq_col=> undef }; + + $cur_cond = [ + { + ($is_desc[$i] ? $subq_col : $main_col) => { '!=', undef }, + ($is_desc[$i] ? $main_col : $subq_col) => undef, + }, + { + $subq_col => { '!=', undef }, + $main_col => { '!=', undef }, + -and => $cur_cond, + }, + ]; + } - # add the order supplement (if any) as this is what will be used for the outer WHERE - $in_sel .= ", $_" for keys %{$sq_attrs->{order_supplement}}; + push @where_cond, { '-and', => [ @skip_colpair_stack[0..$i-1], $cur_cond ] }; + } + +# reuse the sqlmaker WHERE, this will not be returning binds + my $counted_where = do { + local $self->{where_bind}; + $self->where(\@where_cond); + }; +# construct the rownum condition by hand my $rownum_cond; if ($offset) { $rownum_cond = 'BETWEEN ? AND ?'; - push @{$self->{limit_bind}}, [ $self->__offset_bindtype => $offset ], [ $self->__total_bindtype => $offset + $rows - 1] @@ -596,30 +646,51 @@ sub _GenericSubQ { } else { $rownum_cond = '< ?'; - push @{$self->{limit_bind}}, [ $self->__rows_bindtype => $rows ] ; } - # even though binds in order_by make no sense here (the rs needs to be - # ordered by a unique column first) - pass whatever there may be through - # anyway - push @{$self->{limit_bind}}, @order_bind; +# and what we will order by inside + my $inner_order_sql = do { + local $self->{order_bind}; + + my $s = $self->_order_by (\@new_order_by); + + $self->throw_exception('Inner gensubq order may not contain binds... something went wrong') + if @{$self->{order_bind}}; + + $s; + }; + +### resume originally scheduled programming +### +### + + # we need to supply the order for the supplements to be properly calculated + my $sq_attrs = $self->_subqueried_limit_attrs ( + $sql, { %$rs_attrs, order_by => \@new_order_by } + ); + + my $in_sel = $sq_attrs->{selection_inner}; + + # add the order supplement (if any) as this is what will be used for the outer WHERE + $in_sel .= ", $_" for sort keys %{$sq_attrs->{order_supplement}}; + + my $group_having_sql = $self->_parse_rs_attrs($rs_attrs); + return sprintf (" SELECT $sq_attrs->{selection_outer} FROM ( SELECT $in_sel $sq_attrs->{query_leftover}${group_having_sql} ) %s -WHERE ( SELECT COUNT(*) FROM %s %s WHERE %s $cmp_op %s ) $rownum_cond -$order_sql +WHERE ( SELECT COUNT(*) FROM %s %s $counted_where ) $rownum_cond +$inner_order_sql ", map { $self->_quote ($_) } ( $rs_attrs->{alias}, $root_tbl_name, $count_tbl_alias, - "$count_tbl_alias.$first_ord_col", - "$first_ord_alias.$first_ord_col", )); } diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index 225ab01..58df6a1 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -196,6 +196,7 @@ sub _adjust_select_args_for_complex_prefetch { or ! first { $inner_aliastypes->{ordering}{$_} } @multipliers ) { + my $unprocessed_order_chunks; ($inner_attrs->{group_by}, $unprocessed_order_chunks) = $self->_group_over_selection ( $inner_from, $inner_select, $inner_attrs->{order_by} @@ -233,6 +234,7 @@ sub _adjust_select_args_for_complex_prefetch { # as they will have to be a part of the group_by to colapse # things properly my $cur_sel = { map { $_ => 1 } @$inner_select }; + my @pks = map { "$root_alias.$_" } $root_node->{-rsrc}->primary_columns or $self->throw_exception( sprintf 'Unable to perform complex limited prefetch off %s without declared primary key', @@ -268,10 +270,10 @@ sub _adjust_select_args_for_complex_prefetch { $chunk =~ s/\sASC$//i; # maybe our own unqualified column - my ($ord_bit) = ($lquote and $sep) - ? $chunk =~ /^ $lquote ([^$sep]+) $rquote $/x - : $chunk - ; + my $ord_bit = ( + $lquote and $sep and $chunk =~ /^ $lquote ([^$sep]+) $rquote $/x + ) ? $1 : $chunk; + next if ( $ord_bit and @@ -382,7 +384,6 @@ sub _adjust_select_args_for_complex_prefetch { } if ( $need_outer_group_by and $attrs->{_grouped_by_distinct} ) { - my $unprocessed_order_chunks; ($outer_attrs->{group_by}, $unprocessed_order_chunks) = $self->_group_over_selection ( \@outer_from, $outer_select, $outer_attrs->{order_by} @@ -444,7 +445,7 @@ sub _resolve_aliastypes_from_select_args { ); } - # get a column to source/alias map (including unqualified ones) + # get a column to source/alias map (including unambiguous unqualified ones) my $colinfo = $self->_resolve_column_info ($from); # set up a botched SQLA @@ -501,7 +502,17 @@ sub _resolve_aliastypes_from_select_args { # throw away empty chunks $_ = [ map { $_ || () } @$_ ] for values %$to_scan; - # first loop through all fully qualified columns and get the corresponding + # first see if we have any exact matches (qualified or unqualified) + for my $type (keys %$to_scan) { + for my $piece (@{$to_scan->{$type}}) { + if ($colinfo->{$piece} and my $alias = $colinfo->{$piece}{-source_alias}) { + $aliases_by_type->{$type}{$alias} ||= { -parents => $alias_list->{$alias}{-join_path}||[] }; + $aliases_by_type->{$type}{$alias}{-seen_columns}{$colinfo->{$piece}{-fq_colname}} = $piece; + } + } + } + + # now loop through all fully qualified columns and get the corresponding # alias (should work even if they are in scalarrefs) for my $alias (keys %$alias_list) { my $al_re = qr/ @@ -530,7 +541,7 @@ sub _resolve_aliastypes_from_select_args { for my $type (keys %$to_scan) { for my $piece (@{$to_scan->{$type}}) { - if (my @matches = $piece =~ /$col_re/g) { + if ( my @matches = $piece =~ /$col_re/g) { my $alias = $colinfo->{$col}{-source_alias}; $aliases_by_type->{$type}{$alias} ||= { -parents => $alias_list->{$alias}{-join_path}||[] }; $aliases_by_type->{$type}{$alias}{-seen_columns}{"$alias.$_"} = $_ @@ -851,7 +862,8 @@ sub _main_source_order_by_portion_is_stable { ; return unless @ord_cols; - my $colinfos = $self->_resolve_column_info($main_rsrc, \@ord_cols); + my $colinfos = $self->_resolve_column_info($main_rsrc); + for (0 .. $#ord_cols) { if ( ! $colinfos->{$ord_cols[$_]} @@ -866,25 +878,43 @@ sub _main_source_order_by_portion_is_stable { # we just truncated it above return unless @ord_cols; - # since all we check here are the start of the order_by belonging to the - # top level $rsrc, a present identifying set will mean that the resultset - # is ordered by its leftmost table in a stable manner - # - # single source - safely use both qualified and unqualified name my $order_portion_ci = { map { $colinfos->{$_}{-colname} => $colinfos->{$_}, $colinfos->{$_}{-fq_colname} => $colinfos->{$_}, } @ord_cols }; - $where = $where ? $self->_resolve_column_info( - $main_rsrc, $self->_extract_fixed_condition_columns($where) - ) : {}; + # since all we check here are the start of the order_by belonging to the + # top level $rsrc, a present identifying set will mean that the resultset + # is ordered by its leftmost table in a stable manner + # + # RV of _identifying_column_set contains unqualified names only + my $unqualified_idset = $main_rsrc->_identifying_column_set({ + ( $where ? %{ + $self->_resolve_column_info( + $main_rsrc, $self->_extract_fixed_condition_columns($where) + ) + } : () ), + %$order_portion_ci + }) or return; + + my $ret_info; + my %unqualified_idcols_from_order = map { + $order_portion_ci->{$_} ? ( $_ => $order_portion_ci->{$_} ) : () + } @$unqualified_idset; + + # extra optimization - cut the order_by at the end of the identifying set + # (just in case the user was stupid and overlooked the obvious) + for my $i (0 .. $#ord_cols) { + my $col = $ord_cols[$i]; + my $unqualified_colname = $order_portion_ci->{$col}{-colname}; + $ret_info->{$col} = { %{$order_portion_ci->{$col}}, -idx_in_order_subset => $i }; + delete $unqualified_idcols_from_order{$ret_info->{$col}{-colname}}; + + # we didn't reach the end of the identifying portion yet + return $ret_info unless keys %unqualified_idcols_from_order; + } - return ( - $main_rsrc->_identifying_column_set({ %$where, %$order_portion_ci }) - ? $order_portion_ci - : undef - ); + die 'How did we get here...'; } # returns an arrayref of column names which *definitely* have som diff --git a/t/prefetch/lazy_cursor.t b/t/prefetch/lazy_cursor.t index 090d464..de6e936 100644 --- a/t/prefetch/lazy_cursor.t +++ b/t/prefetch/lazy_cursor.t @@ -78,7 +78,7 @@ is ($unordered_rs->next, undef, 'End of RS not lost'); { my $non_uniquely_ordered_constrained = $schema->resultset('CD')->search( { artist => 1 }, - { order_by => 'me.title', prefetch => 'tracks' }, + { order_by => [qw( me.genreid me.title me.year )], prefetch => 'tracks' }, ); isa_ok ($non_uniquely_ordered_constrained->next, 'DBICTest::CD' ); diff --git a/t/sqlmaker/limit_dialects/generic_subq.t b/t/sqlmaker/limit_dialects/generic_subq.t index 5ed89c0..ef899ff 100644 --- a/t/sqlmaker/limit_dialects/generic_subq.t +++ b/t/sqlmaker/limit_dialects/generic_subq.t @@ -3,6 +3,7 @@ use warnings; use Test::More; use lib qw(t/lib); +use List::Util 'min'; use DBICTest; use DBIC::SqlMakerTest; use DBIx::Class::SQLMaker::LimitDialects; @@ -42,7 +43,7 @@ is_same_sql_bind( FROM books rownum__emulation WHERE rownum__emulation.title < me.title ) < ? - ORDER BY me.title + ORDER BY me.title ASC )', [ [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ], @@ -86,7 +87,7 @@ is_same_sql_bind( FROM "books" "rownum__emulation" WHERE "rownum__emulation"."title" > "me"."title" ) BETWEEN ? AND ? - ORDER BY "title" DESC + ORDER BY "me"."title" DESC )', [ [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ], @@ -114,7 +115,7 @@ is_same_sql_bind( '( SELECT "owner_name" FROM ( - SELECT "owner"."name" AS "owner_name", "title" + SELECT "owner"."name" AS "owner_name", "me"."title" FROM "books" "me" JOIN "owners" "owner" ON "owner"."id" = "me"."owner" WHERE ( "source" = ? ) @@ -125,7 +126,7 @@ is_same_sql_bind( FROM "books" "rownum__emulation" WHERE "rownum__emulation"."title" < "me"."title" ) BETWEEN ? AND ? - ORDER BY "title" + ORDER BY "me"."title" ASC )', [ [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ], @@ -140,6 +141,177 @@ is_deeply ( 'Correct columns selected with rows', ); +$rs = $schema->resultset('CD')->search({}, { + columns => [qw( me.cdid me.title me.genreid me.year tracks.position tracks.title )], + join => 'tracks', + collapse => 1, + order_by => [ { -asc => 'me.genreid' }, { -desc => 'year' }, 'me.title', \ 'single_track DESC', { -desc => [qw( me.cdid tracks.position )] } ], +}); + +my @full_res = @{$rs->all_hri}; + +is (@full_res, 5, 'Expected amount of CDs'); + +is_deeply ( + \@full_res, + [ + { cdid => 2, genreid => undef, title => "Forkful of bees", year => 2001, tracks => [ + { position => 3, title => "Sticky Honey" }, + { position => 2, title => "Stripy" }, + { position => 1, title => "Stung with Success" }, + ] }, + { cdid => 4, genreid => undef, title => "Generic Manufactured Singles", year => 2001, tracks => [ + { position => 3, title => "No More Ideas" }, + { position => 2, title => "Boring Song" }, + { position => 1, title => "Boring Name" }, + ] }, + { cdid => 5, genreid => undef, title => "Come Be Depressed With Us", year => 1998, tracks => [ + { position => 3, title => "Suicidal" }, + { position => 2, title => "Under The Weather" }, + { position => 1, title => "Sad" }, + ] }, + { cdid => 3, genreid => undef, title => "Caterwaulin' Blues", year => 1997, tracks => [ + { position => 3, title => "Fowlin" }, + { position => 2, title => "Howlin" }, + { position => 1, title => "Yowlin" }, + ] }, + { cdid => 1, genreid => 1, title => "Spoonful of bees", year => 1999, tracks => [ + { position => 3, title => "Beehind You" }, + { position => 2, title => "Apiary" }, + { position => 1, title => "The Bees Knees" }, + ] }, + ], + 'Complex ordered gensubq limited cds and tracks in expected sqlite order' +); + +for my $slice ( + [0, 10], + [3, 5 ], + [4, 6 ], + [0, 2 ], + [1, 3 ], +) { + + my $rownum_cmp_op = $slice->[0] + ? 'BETWEEN ? AND ?' + : ' < ?' + ; + + is_deeply( + $rs->slice(@$slice)->all_hri, + [ @full_res[ $slice->[0] .. min($#full_res, $slice->[1]) ] ], + "Expected array slice on complex ordered limited gensubq ($slice->[0] : $slice->[1])", + ); + + is_same_sql_bind( + $rs->slice(@$slice)->as_query, + qq{( + SELECT "me"."cdid", "me"."title", "me"."genreid", "me"."year", + "tracks"."position", "tracks"."title" + FROM ( + SELECT "me"."cdid", "me"."title", "me"."genreid", "me"."year", "me"."single_track" + FROM ( + SELECT "me"."cdid", "me"."title", "me"."genreid", "me"."year", "me"."single_track" + FROM cd "me" + LEFT JOIN "track" "tracks" + ON "tracks"."cd" = "me"."cdid" + GROUP BY "me"."cdid", "me"."title", "me"."genreid", "me"."year", "me"."single_track" + ) "me" + WHERE ( + SELECT COUNT( * ) + FROM cd "rownum__emulation" + WHERE ( + ( "me"."genreid" IS NOT NULL AND "rownum__emulation"."genreid" IS NULL ) + OR + ( + "rownum__emulation"."genreid" < "me"."genreid" + AND + "me"."genreid" IS NOT NULL + AND + "rownum__emulation"."genreid" IS NOT NULL + ) + OR + ( + ( + "me"."genreid" = "rownum__emulation"."genreid" + OR + ( "me"."genreid" IS NULL AND "rownum__emulation"."genreid" IS NULL ) + ) + AND + "rownum__emulation"."year" > "me"."year" + ) + OR + ( + ( + "me"."genreid" = "rownum__emulation"."genreid" + OR + ( "me"."genreid" IS NULL AND "rownum__emulation"."genreid" IS NULL ) + ) + AND + "me"."year" = "rownum__emulation"."year" + AND + "rownum__emulation"."title" < "me"."title" + ) + OR + ( + ( + "me"."genreid" = "rownum__emulation"."genreid" + OR + ( "me"."genreid" IS NULL AND "rownum__emulation"."genreid" IS NULL ) + ) + AND + "me"."year" = "rownum__emulation"."year" + AND + "me"."title" = "rownum__emulation"."title" + AND + ( + ("me"."single_track" IS NULL AND "rownum__emulation"."single_track" IS NOT NULL ) + OR + ( + "rownum__emulation"."single_track" > "me"."single_track" + AND + "me"."single_track" IS NOT NULL + AND + "rownum__emulation"."single_track" IS NOT NULL + ) + ) + ) + OR + ( + ( + "me"."genreid" = "rownum__emulation"."genreid" + OR + ( "me"."genreid" IS NULL AND "rownum__emulation"."genreid" IS NULL ) + ) + AND + "me"."year" = "rownum__emulation"."year" + AND + "me"."title" = "rownum__emulation"."title" + AND + ( + ( "me"."single_track" = "rownum__emulation"."single_track" ) + OR + ( "me"."single_track" IS NULL AND "rownum__emulation"."single_track" IS NULL ) + ) + AND + "rownum__emulation"."cdid" > "me"."cdid" + ) + ) + ) $rownum_cmp_op + ORDER BY "me"."genreid" ASC, "me"."year" DESC, "me"."title" ASC, "me"."single_track" DESC, "me"."cdid" DESC + ) "me" + LEFT JOIN "track" "tracks" + ON "tracks"."cd" = "me"."cdid" + ORDER BY "me"."genreid" ASC, "year" DESC, "me"."title", single_track DESC, "me"."cdid" DESC, "tracks"."position" DESC + )}, + [ + ( $slice->[0] ? [ $OFFSET => $slice->[0] ] : () ), + [ $TOTAL => $slice->[1] + ($slice->[0] ? 0 : 1 ) ], + ], + "Expected sql on complex ordered limited gensubq ($slice->[0] : $slice->[1])", + ); +} + { $rs = $schema->resultset('Artist')->search({}, { columns => 'artistid', @@ -155,40 +327,4 @@ is_deeply ( ); } -# this is a nonsensical order_by, we are just making sure the bind-transport is correct -# (not that it'll be useful anywhere in the near future) -my $attr = {}; -my $rs_selectas_rel = $schema->resultset('BooksInLibrary')->search(undef, { - columns => 'me.id', - offset => 3, - rows => 4, - '+columns' => { bar => \['? * ?', [ $attr => 11 ], [ $attr => 12 ]], baz => \[ '?', [ $attr => 13 ]] }, - order_by => [ 'id', \['? / ?', [ $attr => 1 ], [ $attr => 2 ]], \[ '?', [ $attr => 3 ]] ], - having => \[ '?', [ $attr => 21 ] ], -}); - -is_same_sql_bind( - $rs_selectas_rel->as_query, - '( - SELECT "me"."id", "bar", "baz" - FROM ( - SELECT "me"."id", ? * ? AS "bar", ? AS "baz" - FROM "books" "me" - WHERE ( "source" = ? ) - HAVING ? - ) "me" - WHERE ( SELECT COUNT(*) FROM "books" "rownum__emulation" WHERE "rownum__emulation"."id" < "me"."id" ) BETWEEN ? AND ? - ORDER BY "id", ? / ?, ? - )', - [ - [ $attr => 11 ], [ $attr => 12 ], [ $attr => 13 ], - [ { sqlt_datatype => 'varchar', sqlt_size => 100, dbic_colname => 'source' } => 'Library' ], - [ $attr => 21 ], - [ {%$OFFSET} => 3 ], - [ {%$TOTAL} => 6 ], - [ $attr => 1 ], [ $attr => 2 ], [ $attr => 3 ], - ], - 'Pagination with sub-query in ORDER BY works' -); - done_testing; diff --git a/t/sqlmaker/limit_dialects/torture.t b/t/sqlmaker/limit_dialects/torture.t index 75d7554..517444b 100644 --- a/t/sqlmaker/limit_dialects/torture.t +++ b/t/sqlmaker/limit_dialects/torture.t @@ -694,11 +694,11 @@ my $tests = { }, GenericSubQ => { - limit => [ + ordered_limit => [ '( SELECT me.id, owner__id, owner__name, bar, baz FROM ( - SELECT me.id, owner.id AS owner__id, owner.name AS owner__name, ? * ? AS bar, ? AS baz + SELECT me.id, owner.id AS owner__id, owner.name AS owner__name, ? * ? AS bar, ? AS baz, me.price FROM books me JOIN owners owner ON owner.id = me.owner @@ -709,9 +709,28 @@ my $tests = { WHERE ( SELECT COUNT( * ) FROM books rownum__emulation - WHERE rownum__emulation.id < me.id - ) < ? - ORDER BY me.id + WHERE + ( me.price IS NULL AND rownum__emulation.price IS NOT NULL ) + OR + ( + rownum__emulation.price > me.price + AND + me.price IS NOT NULL + AND + rownum__emulation.price IS NOT NULL + ) + OR + ( + ( + me.price = rownum__emulation.price + OR + ( me.price IS NULL AND rownum__emulation.price IS NULL ) + ) + AND + rownum__emulation.id < me.id + ) + ) < ? + ORDER BY me.price DESC, me.id ASC )', [ @select_bind, @@ -721,11 +740,11 @@ my $tests = { [ { sqlt_datatype => 'integer' } => 4 ], ], ], - limit_offset => [ + ordered_limit_offset => [ '( SELECT me.id, owner__id, owner__name, bar, baz FROM ( - SELECT me.id, owner.id AS owner__id, owner.name AS owner__name, ? * ? AS bar, ? AS baz + SELECT me.id, owner.id AS owner__id, owner.name AS owner__name, ? * ? AS bar, ? AS baz, me.price FROM books me JOIN owners owner ON owner.id = me.owner @@ -736,9 +755,28 @@ my $tests = { WHERE ( SELECT COUNT( * ) FROM books rownum__emulation - WHERE rownum__emulation.id < me.id - ) BETWEEN ? AND ? - ORDER BY me.id + WHERE + ( me.price IS NULL AND rownum__emulation.price IS NOT NULL ) + OR + ( + rownum__emulation.price > me.price + AND + me.price IS NOT NULL + AND + rownum__emulation.price IS NOT NULL + ) + OR + ( + ( + me.price = rownum__emulation.price + OR + ( me.price IS NULL AND rownum__emulation.price IS NULL ) + ) + AND + rownum__emulation.id < me.id + ) + ) BETWEEN ? AND ? + ORDER BY me.price DESC, me.id ASC )', [ @select_bind, @@ -755,18 +793,28 @@ my $tests = { FROM ( SELECT me.name, me.id FROM ( - SELECT me.name, me.id FROM owners me + SELECT me.name, me.id + FROM owners me ) me - WHERE ( - SELECT COUNT(*) - FROM owners rownum__emulation - WHERE rownum__emulation.id < me.id - ) BETWEEN ? AND ? - ORDER BY me.id + WHERE + ( + SELECT COUNT(*) + FROM owners rownum__emulation + WHERE ( + rownum__emulation.name < me.name + OR + ( + me.name = rownum__emulation.name + AND + rownum__emulation.id > me.id + ) + ) + ) BETWEEN ? AND ? + ORDER BY me.name ASC, me.id DESC ) me LEFT JOIN books books ON books.owner = me.id - ORDER BY me.id + ORDER BY me.name ASC, me.id DESC )', [ [ { sqlt_datatype => 'integer' } => 1 ], @@ -793,7 +841,6 @@ for my $limtype (sort keys %$tests) { '+columns' => { bar => \['? * ?', [ $attr => 11 ], [ $attr => 12 ]], baz => \[ '?', [ $attr => 13 ]] }, group_by => \[ '(me.id / ?), owner.id', [ $attr => 21 ] ], having => \[ '?', [ $attr => 31 ] ], - ($limtype =~ /GenericSubQ/ ? ( order_by => 'me.id' ) : () ), # needs a simple-column stable order to be happy }); # @@ -827,7 +874,10 @@ for my $limtype (sort keys %$tests) { # order + limit, no offset $rs = $rs->search(undef, { - order_by => [ \['? / ?', [ $attr => 1 ], [ $attr => 2 ]], \[ '?', [ $attr => 3 ]] ], + order_by => ( $limtype =~ /GenericSubQ/ + ? [ { -desc => 'price' }, 'me.id', \[ 'owner.name + ?', [ {} => 'bah' ] ] ] # needs a same-table stable order to be happy + : [ \['? / ?', [ $attr => 1 ], [ $attr => 2 ]], \[ '?', [ $attr => 3 ]] ] + ), }); if ($tests->{$limtype}{ordered_limit}) { @@ -860,7 +910,10 @@ for my $limtype (sort keys %$tests) { offset => 1, columns => 'name', # only the owner name, still prefetch all the books prefetch => 'books', - ($limtype =~ /GenericSubQ/ ? ( order_by => 'me.id' ) : () ), # needs a simple-column stable order to be happy + ($limtype !~ /GenericSubQ/ ? () : ( + # needs a same-table stable order to be happy + order_by => [ { -asc => 'me.name' }, \'me.id DESC' ] + )), }); is_same_sql_bind (