From: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org>
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;hp=0844bb396f5702db73a8eea947116475b4d89e9a;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;