From: Dagfinn Ilmari Mannsåker Date: Wed, 10 Dec 2014 18:04:06 +0000 (+0000) Subject: Fix ordering by 1:M prefetched boolean columns in Pg X-Git-Tag: v0.082840~13 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=576ea48fa3ff6baaa7d539c36e660a9fc73899e9;p=dbsrgits%2FDBIx-Class.git Fix ordering by 1:M prefetched boolean columns in Pg ( cherry-pick of 7fe322c8d ) PostgreSQL doesn't have min/max aggregates for the boolean type, but it has and/or, which are equivalent. So, allow the storage to override the aggregate used for constructing the order by clause based on the column info. --- diff --git a/Changes b/Changes index de7fae5..92c57f4 100644 --- a/Changes +++ b/Changes @@ -16,6 +16,8 @@ Revision history for DBIx::Class $dbh->{HandleError} (GH#101) - Fix parsing of DSNs containing driver arguments (GH#99) - Fix silencing of exceptions thrown by custom inflate_result() methods + - Fix complex prefetch when ordering over foreign boolean columns + ( Pg can't MAX(boolcol) despite being able to ORDER BY boolcol ) - Fix spurious ROLLBACK statements when a TxnScopeGuard fails a commit of a transaction with deferred FK checks: a guard is now inactivated immediately before the commit is attempted (RT#107159) diff --git a/lib/DBIx/Class/Storage/DBI/Pg.pm b/lib/DBIx/Class/Storage/DBI/Pg.pm index 7c20330..e6f19e7 100644 --- a/lib/DBIx/Class/Storage/DBI/Pg.pm +++ b/lib/DBIx/Class/Storage/DBI/Pg.pm @@ -162,6 +162,22 @@ sub sqlt_type { return 'PostgreSQL'; } +# Pg is not able to MAX(boolean_column), sigh... +# +# Generally it would make more sense to have this in the SQLMaker hierarchy, +# so that eventually { -max => ... } DTRT, but plans going forward are +# murky at best +# --ribasushi +# +sub _minmax_operator_for_datatype { + #my ($self, $datatype, $want_max) = @_; + + return ($_[2] ? 'BOOL_OR' : 'BOOL_AND') + if ($_[1] || '') =~ /\Abool(?:ean)?\z/i; + + shift->next::method(@_); +} + sub bind_attribute_by_data_type { my ($self,$data_type) = @_; diff --git a/lib/DBIx/Class/Storage/DBIHacks.pm b/lib/DBIx/Class/Storage/DBIHacks.pm index ce16917..adab1d8 100644 --- a/lib/DBIx/Class/Storage/DBIHacks.pm +++ b/lib/DBIx/Class/Storage/DBIHacks.pm @@ -630,12 +630,9 @@ sub _group_over_selection { # 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 + # both populated on the first loop over $o_idx $sql_maker ||= $self->sql_maker; $order_chunks ||= [ map { ref $_ eq 'ARRAY' ? $_ : [ $_ ] } $sql_maker->_order_by_chunks($attrs->{order_by}) @@ -645,7 +642,7 @@ sub _group_over_selection { $new_order_by[$o_idx] = \[ sprintf( '%s( %s )%s', - ($is_desc ? 'MAX' : 'MIN'), + $self->_minmax_operator_for_datatype($chunk_ci->{data_type}, $is_desc), $chunk, ($is_desc ? ' DESC' : ''), ), @@ -673,6 +670,12 @@ sub _group_over_selection { ); } +sub _minmax_operator_for_datatype { + #my ($self, $datatype, $want_max) = @_; + + $_[2] ? 'MAX' : 'MIN'; +} + sub _resolve_ident_sources { my ($self, $ident) = @_; diff --git a/t/sqlmaker/pg.t b/t/sqlmaker/pg.t new file mode 100644 index 0000000..c222a7d --- /dev/null +++ b/t/sqlmaker/pg.t @@ -0,0 +1,77 @@ +use strict; +use warnings; + +use Test::More; + +use lib 't/lib'; +use DBICTest ':DiffSQL'; + +my $schema = DBICTest->init_schema( + no_deploy => 1, + quote_names => 1, + storage_type => 'DBIx::Class::Storage::DBI::Pg' +); + +my $rs = $schema->resultset('Artist')->search_related('cds_unordered', + { "me.rank" => 13 }, + { + prefetch => 'tracks', + join => 'genre', + order_by => [ 'genre.name', { -desc => \ 'tracks.title' }, { -asc => "me.name" }, { -desc => [qw(year cds_unordered.title)] } ], # me. is the artist, *NOT* the cd + rows => 1, + }, +); + +{ + # THIS IS AN OFFLINE TEST + # We only need this so that the thing can be verified to work without PG_DSN + # Executing it while "lying" this way won't work + local $rs->result_source->related_source('tracks')->column_info('title')->{data_type} = 'bool'; + local $rs->result_source->related_source('genre')->column_info('name')->{data_type} = 'BOOLEAN'; + + is_same_sql_bind( + $rs->as_query, + q{( + SELECT "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track", + "tracks"."trackid", "tracks"."cd", "tracks"."position", "tracks"."title", "tracks"."last_updated_on", "tracks"."last_updated_at" + FROM "artist" "me" + JOIN ( + SELECT "cds_unordered"."cdid", "cds_unordered"."artist", "cds_unordered"."title", "cds_unordered"."year", "cds_unordered"."genreid", "cds_unordered"."single_track" + FROM "artist" "me" + JOIN cd "cds_unordered" + ON "cds_unordered"."artist" = "me"."artistid" + LEFT JOIN "genre" "genre" + ON "genre"."genreid" = "cds_unordered"."genreid" + 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", "me"."name" + ORDER BY BOOL_AND("genre"."name"), + BOOL_OR( tracks.title ) DESC, + "me"."name" ASC, + "year" DESC, + "cds_unordered"."title" DESC + LIMIT ? + ) "cds_unordered" + ON "cds_unordered"."artist" = "me"."artistid" + LEFT JOIN "genre" "genre" + ON "genre"."genreid" = "cds_unordered"."genreid" + LEFT JOIN "track" "tracks" + ON "tracks"."cd" = "cds_unordered"."cdid" + WHERE "me"."rank" = ? + ORDER BY "genre"."name", + tracks.title DESC, + "me"."name" ASC, + "year" DESC, + "cds_unordered"."title" DESC + )}, + [ + [ { sqlt_datatype => 'integer', dbic_colname => 'me.rank' } => 13 ], + [ DBIx::Class::SQLMaker::LimitDialects->__rows_bindtype => 1 ], + [ { sqlt_datatype => 'integer', dbic_colname => 'me.rank' } => 13 ], + ], + 'correct SQL with aggregate boolean order on Pg', + ); +} + +done_testing;