[Bps-public-commit] dbix-searchbuilder branch distinct-and-count-and-sort created. 1.76-1-gcb59401

BPS Git Server git at git.bestpractical.com
Wed Jun 14 20:29:14 UTC 2023


This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "dbix-searchbuilder".

The branch, distinct-and-count-and-sort has been created
        at  cb59401ee657d7ab71e74e936d1753383fb98a8d (commit)

- Log -----------------------------------------------------------------
commit cb59401ee657d7ab71e74e936d1753383fb98a8d
Author: Ruslan Zakirov <ruz at bestpractical.com>
Date:   Wed Jun 14 23:11:04 2023 +0300

    Change how DistinctQueryAndCount builds query to fix sorting
    
    If order by is on columns from the main table then old query generated by
    DistinctQueryAndCount was like this:
    
        SELECT main.*, COUNT(main.id) OVER () FROM (SELECT ... ORDER BY main...) main;
    
    It works until it doesn't. Order by clause should be on outer most query.
    
    Some evidence:
    
    * SQL standard: ORDER BY command cannot be used in a subquery. Google
      ANSI SQL order by in subquery.
    
    * mariadb explanation: https://mariadb.com/kb/en/why-is-order-by-in-a-from-subquery-ignored/
    
    * SQL server: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-order-by-clause-transact-sql?view=sql-server-ver16
      "The ORDER BY clause is not valid in views, ... and *subqueries*, unless ..."
    
    Even in cases where ORDER BY is allowed in a subquery there is stated
    guaranty that order will preserved in the outer query.
    
    New query:
    
        SELECT main.*, COUNT(main.id) OVER () FROM (SELECT ...) main ORDER BY main...;
    
    Note that in case of empty ORDER BY the query is still wrapped to make
    sure the window function counts distinct records, eg:
    
        SELECT main.*, COUNT(main.id) OVER () FROM (SELECT DISTINCT ...) main;

diff --git a/lib/DBIx/SearchBuilder/Handle.pm b/lib/DBIx/SearchBuilder/Handle.pm
index eb8c7a7..747191c 100755
--- a/lib/DBIx/SearchBuilder/Handle.pm
+++ b/lib/DBIx/SearchBuilder/Handle.pm
@@ -1453,6 +1453,10 @@ sub DistinctQuery {
     my $self = shift;
     my $statementref = shift;
     my $sb = shift;
+    my %args = (
+        Wrap => 0,
+        @_
+    );
 
     my $QueryHint = $sb->QueryHint;
     $QueryHint = $QueryHint ? " /* $QueryHint */ " : " ";
@@ -1460,6 +1464,9 @@ sub DistinctQuery {
     # Prepend select query for DBs which allow DISTINCT on all column types.
     $$statementref = "SELECT" . $QueryHint . "DISTINCT main.* FROM $$statementref";
     $$statementref .= $sb->_GroupClause;
+    if ( $args{'Wrap'} ) {
+        $$statementref = "SELECT * FROM ($$statementref) main";
+    }
     $$statementref .= $sb->_OrderClause;
 }
 
@@ -1475,18 +1482,21 @@ sub DistinctQueryAndCount {
     my $statementref = shift;
     my $sb = shift;
 
-    $self->DistinctQuery($statementref, $sb);
+    # order by clause should be on outer most query
+    # SQL standard: ORDER BY command cannot be used in a Subquery
+    # mariadb explanation: https://mariadb.com/kb/en/why-is-order-by-in-a-from-subquery-ignored/
+    # pg: actually keeps order of a subquery as long as some conditions in outer query are met, but
+    #     it's just a coincidence, not a feature
+    # SQL server: https://learn.microsoft.com/en-us/sql/t-sql/queries/select-order-by-clause-transact-sql?view=sql-server-ver16
+    #             The ORDER BY clause is not valid in views, ... and *subqueries*, unless ...
 
-    # Add the count part.
-    if ( $sb->_OrderClause !~ /(?<!main)\./ ) {
-        # Wrap it with another SELECT to get distinct count.
-        $$statementref
-            = 'SELECT main.*, COUNT(main.id) OVER() AS search_builder_count_all FROM (' . $$statementref . ') main';
-    }
-    else {
-        # if order by other tables, then DistinctQuery already has an outer SELECT, which we can reuse
-        $$statementref =~ s!(?= FROM)!, COUNT(main.id) OVER() AS search_builder_count_all!;
-    }
+    my $order = $sb->_OrderClause;
+    my $wrap = $order !~ /(?<!main)\./;
+
+    $self->DistinctQuery($statementref, $sb, Wrap => $wrap);
+
+    # DistinctQuery already has an outer SELECT, which we can reuse
+    $$statementref =~ s!(?= FROM)!, COUNT(main.id) OVER() AS search_builder_count_all!;
 }
 
 
diff --git a/t/02order_outer.t b/t/02order.t
similarity index 57%
rename from t/02order_outer.t
rename to t/02order.t
index c91c7f3..969e1c7 100644
--- a/t/02order_outer.t
+++ b/t/02order.t
@@ -7,7 +7,7 @@ use Test::More;
 BEGIN { require "./t/utils.pl" }
 our (@AvailableDrivers);
 
-use constant TESTS_PER_DRIVER => 104;
+use constant TESTS_PER_DRIVER => 315;
 
 my $total = scalar(@AvailableDrivers) * TESTS_PER_DRIVER;
 plan tests => $total;
@@ -55,52 +55,103 @@ diag "generate data" if $ENV{TEST_VERBOSE};
     }
 }
 
-# ASC order
+my @fields = (
+    'Name',
+    $d eq 'Oracle' ? 'TO_CHAR(Name)' : $d eq 'mysql' ? 'BINARY(Name)' : 'CAST(Name AS TEXT)'
+);
+
+diag "test ordering objects by fields on Tags table" if $ENV{TEST_VERBOSE};
 foreach my $direction ( qw(ASC DESC) ) {
-    for my $field ( 'Name',
-        $d eq 'Oracle' ? 'TO_CHAR(Name)' : $d eq 'mysql' ? 'BINARY(Name)' : 'CAST(Name AS TEXT)' )
-    {
-        my $objs = TestApp::Objects->new($handle);
-        $objs->UnLimit;
-        my $tags_alias = $objs->Join(
-            TYPE   => 'LEFT',
-            ALIAS1 => 'main',
-            FIELD1 => 'id',
-            TABLE2 => 'Tags',
-            FIELD2 => 'Object',
-        );
-        ok($tags_alias, "joined tags table");
-        # Generated SQL is MIN(Name) or nested functions like MIN(CAST(Name AS TEXT))
-        $objs->OrderBy( ALIAS => $tags_alias, FIELD => $field, ORDER => $direction );
-
-        ok($objs->First, 'ok, we have at least one result');
+foreach my $field (@fields ) {
+foreach my $combine_search_and_count ( 0, 1 ) {
+foreach my $per_page (0, 5) {
+foreach my $page (0, 2) {
+    my $objs = TestApp::Objects->new($handle);
+    $objs->CombineSearchAndCount($combine_search_and_count);
+    $objs->UnLimit;
+    my $tags_alias = $objs->Join(
+        TYPE   => 'LEFT',
+        ALIAS1 => 'main',
+        FIELD1 => 'id',
+        TABLE2 => 'Tags',
+        FIELD2 => 'Object',
+    );
+    ok($tags_alias, "joined tags table");
+    # Generated SQL is MIN(Name) or nested functions like MIN(CAST(Name AS TEXT))
+    $objs->OrderBy( ALIAS => $tags_alias, FIELD => $field, ORDER => $direction );
+    $objs->RowsPerPage($per_page) if $per_page;
+    $objs->GotoPage($page) if $page;
+
+    ok($objs->First, 'ok, we have at least one result');
+    $objs->GotoFirstItem;
+
+    my ($order_ok, $last) = (1, $direction eq 'ASC'? '-': 'zzzz');
+    while ( my $obj = $objs->Next ) {
+        my $tmp;
+        if ( $direction eq 'ASC' ) {
+            $tmp = (substr($last, 0, 1) cmp substr($obj->Name, 0, 1));
+        } else {
+            $tmp = -(substr($last, -1, 1) cmp substr($obj->Name, -1, 1));
+        }
+        if ( $tmp > 0 ) {
+            $order_ok = 0; last;
+        }
+        $last = $obj->Name;
+    }
+    ok($order_ok, "$direction order is correct") or do {
+        diag "Wrong $direction query: ". $objs->BuildSelectQuery;
         $objs->GotoFirstItem;
-
-        my ($order_ok, $last) = (1, $direction eq 'ASC'? '-': 'zzzz');
         while ( my $obj = $objs->Next ) {
-            my $tmp;
-            if ( $direction eq 'ASC' ) {
-                $tmp = (substr($last, 0, 1) cmp substr($obj->Name, 0, 1));
-            } else {
-                $tmp = -(substr($last, -1, 1) cmp substr($obj->Name, -1, 1));
-            }
-            if ( $tmp > 0 ) {
-                $order_ok = 0; last;
-            }
-            $last = $obj->Name;
-        }
-        ok($order_ok, "$direction order is correct") or do {
-            diag "Wrong $direction query: ". $objs->BuildSelectQuery;
-            $objs->GotoFirstItem;
-            while ( my $obj = $objs->Next ) {
-                diag($obj->id .":". $obj->Name);
-            }
+            diag($obj->id .":". $obj->Name);
         }
+    };
+
+    my $got_count = $objs->CountAll;
+    is ($got_count, 30, "CountAll is expected");
+
+}}}}} # foreach variants blocks
+
+my $expected_count = 0;
+
+diag "test ordering objects by object's fields with limit by tags" if $ENV{TEST_VERBOSE};
+foreach my $direction ( qw(ASC DESC) ) {
+foreach my $field ('Name', 'id') {
+foreach my $combine_search_and_count ( 0, 1 ) {
+foreach my $per_page (0, 5) {
+foreach my $page (0, 2) {
+    my $objs = TestApp::Objects->new($handle);
+    $objs->CombineSearchAndCount($combine_search_and_count);
+    my $tags_alias = $objs->Join(
+        ALIAS1 => 'main',
+        FIELD1 => 'id',
+        TABLE2 => 'Tags',
+        FIELD2 => 'Object',
+    );
+    ok($tags_alias, "joined tags table");
+    $objs->OrderBy( FIELD => $field, ORDER => $direction );
+    $objs->RowsPerPage($per_page) if $per_page;
+    $objs->GotoPage($page) if $page;
+
+    $objs->Limit( ALIAS => $tags_alias, FIELD => 'Name', OPERATOR => 'IN', VALUE => ['c', 'a'] );
+
+    my @list;
+    while ( my $obj = $objs->Next ) {
+        push @list, $field eq 'Name'? $obj->Name : $obj->Id;
     }
-}
+    my @correct = sort {$field eq 'Name'? $a cmp $b : $a <=> $b } @list;
+    @correct = reverse @correct if $direction eq 'DESC';
+    is_deeply(\@list, \@correct, "correct order");
+
+    my $got_count = $objs->CountAll;
+    if ($expected_count) {
+        is($got_count, $expected_count, "count is correct");
+    } else {
+        $expected_count = $got_count;
+    }
+}}}}} # foreach variants blocks
 
 	cleanup_schema( 'TestApp', $handle );
-}} # SKIP, foreach blocks
+}} # SKIP, foreach driver block
 
 1;
 
@@ -118,6 +169,7 @@ sub schema_mysql { [
         Name varchar(36),
       	PRIMARY KEY (id)
     )",
+    "CREATE INDEX Tags1 ON Tags (Name)"
 ] }
 
 sub schema_pg { [
@@ -130,6 +182,7 @@ sub schema_pg { [
         Object integer NOT NULL,
         Name varchar(36)
     )",
+    "CREATE INDEX Tags1 ON Tags (Name)"
 ]}
 
 sub schema_sqlite {[
@@ -142,6 +195,7 @@ sub schema_sqlite {[
         Object integer NOT NULL,
         Name varchar(36)
     )",
+    "CREATE INDEX Tags1 ON Tags (Name)"
 ]}
 
 sub schema_oracle { [
@@ -156,6 +210,7 @@ sub schema_oracle { [
         Object integer NOT NULL,
         Name varchar(36)
     )",
+    "CREATE INDEX Tags1 ON Tags (Name)"
 ] }
 
 sub cleanup_schema_oracle { [
diff --git a/t/utils.pl b/t/utils.pl
index b662034..e21e3f9 100644
--- a/t/utils.pl
+++ b/t/utils.pl
@@ -110,6 +110,8 @@ sub connect_mysql
 	return $handle->Connect(
 		Driver => 'mysql',
 		Database => $ENV{'SB_TEST_MYSQL'},
+		Host => $ENV{'SB_TEST_MYSQL_HOST'},
+		Port => $ENV{'SB_TEST_MYSQL_PORT'},
 		User => $ENV{'SB_TEST_MYSQL_USER'} || 'root',
 		Password => $ENV{'SB_TEST_MYSQL_PASS'} || '',
 	);
@@ -121,6 +123,8 @@ sub connect_pg
 	return $handle->Connect(
 		Driver => 'Pg',
 		Database => $ENV{'SB_TEST_PG'},
+		Host => $ENV{'SB_TEST_PG_HOST'},
+		Port => $ENV{'SB_TEST_PG_PORT'},
 		User => $ENV{'SB_TEST_PG_USER'} || 'postgres',
 		Password => $ENV{'SB_TEST_PG_PASS'} || '',
 	);

-----------------------------------------------------------------------


hooks/post-receive
-- 
dbix-searchbuilder


More information about the Bps-public-commit mailing list