[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