Keep order for set operators such as UNION / UNION ALL #116

csthomas · 2018-04-16T16:33:10Z

Pull Request for Issue #115

Summary of Changes

Fix UNION/UNION ALL
Allow to mix UNION with UNION ALL and preserve the order.
Allow to support (in the future) more set operators like EXCEPT and INTERSECT
A few B/C breaks
- Method unionDistinct() - removed, use union() instead.
- Protected variables $union and $unionAll replaced by variable $merge (array) to preserve the order.
- Array is not accepted in the $query parameter of union() and unionAll() methods.
New methods DatabaseQuery::querySet($query) and DatabaseQuery::toQuerySet(), see Keep order for set operators such as UNION / UNION ALL #116 (comment)

Testing Instructions

Go to database/src folder.
Run php -a or create a short script like below and execute it. Replace dots ....

require '../vendor/autoload.php';
$db = Joomla\Database\DatabaseDriver::getInstance(['driver' => 'mysqli', 'host' => 'localhost', 'user' => '...', 'password' => '...', 'database' => '...', 'prefix' => '...']);
print_r($db->setQuery($db->getQuery(true)->select('1')->union($db->getQuery(true)->select('2'))->unionAll($db->getQuery(true)->select('3')))->loadColumn());
Array
(
    [0] => 1
    [1] => 2
    [2] => 3
)
print_r($db->setQuery($db->getQuery(true)->select('2')->union($db->getQuery(true)->select('1'))->unionAll($db->getQuery(true)->select('1')))->loadColumn());
Array
(
    [0] => 2
    [1] => 1
    [2] => 1
)
print_r($db->setQuery($db->getQuery(true)->select('2')->unionAll($db->getQuery(true)->select('1'))->union($db->getQuery(true)->select('1')))->loadColumn());
Array
(
    [0] => 2
    [1] => 1
)

Documentation Changes Required

`Joomla\Database\DatabaseQuery` general changes

Changes in methods `union()`, `unionAll()` and `unionDistinct()`

Method unionDistinct() has been removed. Use union() instead.
Argument $query stops accepting the array. Only DatabaseQuery object or string.
Argument $distinct has been removed from unionAll.
Argument $glue has been removed from both.

Method union() by default has $distinct = true.
If $distinct is false then generates UNION ALL sql statement.

Class variables $union and $unionAll has been merged into one variable $merge.
The new variable represents an ordered array of individual elements.

Stop supporting $type = 'union' in method __toString().

Mix of union types

SELECT 1 UNION SELECT 2 UNION ALL SELECT 1 -- > return 3 rows

SELECT 1 UNION ALL SELECT 2 UNION SELECT 1 -- > return 2 rows

You can combine multiple queries using the set operators UNION, UNION ALL, INTERSECT, and MINUS. All set operators have equal precedence. If a SQL statement contains multiple set operators, then Oracle Database evaluates them from the left to right unless parentheses explicitly specify another order.

Source https://docs.oracle.com/cd/B19306_01/server.102/b14200/queries004.htm

mbabker · 2018-04-16T18:00:39Z

Seems fair to me. Just add a note to https://github.com/joomla-framework/database/blob/2.0-dev/docs/v1-to-v2-update.md and I'd say this should be fine, we just need to sort out deprecation notices then in the CMS repo and this repo's master (1.x) branch.

csthomas · 2018-04-16T18:18:31Z

I'm not sure about variable name '$merge' but I have not found a better name.

csthomas · 2018-04-19T20:29:25Z

If necessary, correct my documentation.

alikon · 2018-04-25T17:44:44Z

look nice to me too, IIRC UNION in core is used only in com_finder

csthomas · 2018-04-25T18:51:04Z

Nice, I think about it "too much" and now I have another solution, where we can put ORDER BY and LIMIT before UNION statement. ~~I will prepare another PR for that.~~

[UPDATED]
Ups, I did it on this PR:)

alikon · 2018-04-25T18:54:52Z

👍
despite i'm not a fan of "LIMIT" clause on such kind of analytics query 😨

Quy · 2018-04-27T05:11:21Z

docs/v1-to-v2-update.md

+Method `union()` by default has `$distinct = true`.
+If `$distinct` is `false` then generates `UNION ALL` sql statement.
+
+Class variables `$union` and `$unionAll` has been merged into one variable `$merge`.


Change has to have

csthomas · 2018-04-27T15:59:08Z

To get ORDER BY or/and LIMIT before the first UNION statement I added to DatabaseQuery class a new type querySet. This is something like subquery in DatabaseQuery object.

Option 1:

$tbl = $db->quoteName('#__foo');

$query = $db->getQuery(true);
$query2 = $db->getQuery(true);
$query3 = $db->getQuery(true);

$query->querySet($query2->select('name')->from($tbl)->order('id DESC')->setLimit(1))
	->unionAll($query3->select('name')->from($tbl)->order('id')->setLimit(1))
	->order('name')
	->setLimit(1)

Option 2:

$tbl = $db->quoteName('#__foo');

$query = $db->getQuery(true);
$query2 = $db->getQuery(true);

$query->select('name')->from($tbl)->order('id DESC')->setLimit(1)
	->toQuerySet()
	->unionAll($query2->select('name')->from($tbl)->order('id')->setLimit(1))
	->order('name')
	->setLimit(1);

Common result:

(
	SELECT name FROM `#__foo` ORDER BY id DESC LIMIT 1
)
UNION (
	SELECT name FROM `#__foo` ORDER BY id LIMIT 1
)
ORDER BY name LIMIT 1

csthomas · 2018-04-27T21:39:59Z

At the end I added new unit tests for PostgreSQL and SQLite.

SqlSrv may still does not work if you use ORDER BY or LIMIT in union query. This is more complicated fix and I would not add it to this PR.

What do you thing about this? @mbabker @alikon @Quy

mbabker · 2018-05-01T01:43:17Z

Seems fine to me.

SqlSrv may still does not work if you use ORDER BY or LIMIT in union query. This is more complicated fix and I would not add it to this PR.

Well luckily the testing infrastructure here is a lot better off (and you can somewhat copy the current Travis config to get a working SQL Server instance to test against). No tests are failing for now so let's roll with it and we can work on updating the test suite to better cover cases like this.

simbunch · 2018-05-02T11:36:19Z

Tested successfully.

csthomas · 2018-05-09T10:47:24Z

I will spend a little time to fix sqlsrv stuff, I need to back to my previous PR #104

…ex unions

csthomas · 2018-05-10T18:27:16Z

My tests:

require 'vendor/autoload.php';
$db = Joomla\Database\DatabaseDriver::getInstance(['driver' => 'sqlsrv', 'database' => '', 'user' => '', 'password' => '', 'prefix' => 'j38_']);
$q = $db->getQuery(1)->select('id')->from('j38_modules')->where('id <= 5')->order('id DESC')->setLimit(2, 1)->toQuerySet()->unionAll($db->getQuery(1)->select('id')->from('j38_modules')->where('id = 3 OR id = 4')->order('0+id DESC')->setLimit(1, 1))->order('id DESC');

echo $q;

SELECT * FROM (
SELECT * FROM (
SELECT * FROM (
SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS RowNumber_4c1bcee9724ef8f6f37135d9596d299a
FROM (
SELECT TOP 3 id
FROM j38_modules
WHERE id <= 5
ORDER BY id DESC) AS A
) AS A
WHERE RowNumber_4c1bcee9724ef8f6f37135d9596d299a > 1) AS merge_0
UNION ALL SELECT * FROM (
SELECT * FROM (
SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS RowNumber_b0af95c67b1589f1db7914ea2e26f658
FROM (
SELECT TOP 2 id
FROM j38_modules
WHERE id = 3 OR id = 4
ORDER BY 0+id DESC) AS A
) AS A
WHERE RowNumber_b0af95c67b1589f1db7914ea2e26f658 > 1) AS merge_1
) AS merges
ORDER BY id DESC

echo $q->setLimit(5, 2);

SELECT * FROM (
SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS RowNumber_e89d72665d4b94533c8b495e1eebf7ee
FROM (
SELECT TOP 7 * FROM (
SELECT * FROM (
SELECT * FROM (
SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS RowNumber_4c1bcee9724ef8f6f37135d9596d299a
FROM (
SELECT TOP 3 id
FROM j38_modules
WHERE id <= 5
ORDER BY id DESC) AS A
) AS A
WHERE RowNumber_4c1bcee9724ef8f6f37135d9596d299a > 1) AS merge_0
UNION ALL SELECT * FROM (
SELECT * FROM (
SELECT *, ROW_NUMBER() OVER (ORDER BY (SELECT 0)) AS RowNumber_b0af95c67b1589f1db7914ea2e26f658
FROM (
SELECT TOP 2 id
FROM j38_modules
WHERE id = 3 OR id = 4
ORDER BY 0+id DESC) AS A
) AS A
WHERE RowNumber_b0af95c67b1589f1db7914ea2e26f658 > 1) AS merge_1
) AS merges
ORDER BY id DESC) AS A
) AS A
WHERE RowNumber_e89d72665d4b94533c8b495e1eebf7ee > 2

It looks a little ugly but we can only remove ROW_NUMBER() after we bump to SQL Server 2012

csthomas · 2018-05-10T22:56:15Z

Since the minimum version of SQL Server is now 2012, it would be good to wait for the new SqlsrvQuery::processList () from #124

…all, limit and offset

…ll, limit and offset

…all, limit and offset

…nteger

…all, limit and offset

csthomas · 2018-05-11T23:01:37Z

Now unit tests execute real query tests for UNIONs on sqlite, mysqli, pgsql and sqlsrv.

csthomas · 2018-05-11T23:18:11Z

Unit tests passed.

mbabker · 2018-05-11T23:21:35Z

src/Pdo/PdoDriver.php

@@ -681,7 +681,7 @@ protected function prepareStatement(string $query): StatementInterface
 		}
 		catch (\PDOException $exception)
 		{
-			throw new PrepareStatementFailureException($exception->getMessage(), $exception->getCode(), $exception);
+			throw new PrepareStatementFailureException($exception->getMessage() . $query, (int) $exception->getCode(), $exception);


If we really need the query as part of the Exception, it should be a separate property similar to the query failure exception so the query isn't shown plain text as part of the message.

Also, we shouldn't cast the exception code to integer here. It is correct that the DB drivers don't strictly use numeric codes and Exception codes aren't type enforced.

This should also be consistently done for all drivers (i.e. the non-PDO ones), not just PDO.

Ups, it was only for debug, I will remove it

csthomas · 2018-05-11T23:46:40Z

Also, we shouldn't cast the exception code to integer here. It is correct that the DB drivers don't strictly use numeric codes and Exception codes aren't type enforced.

Yes, but try to do a bug in code:

diff --git a/src/Sqlite/SqliteQuery.php b/src/Sqlite/SqliteQuery.php
index 940d030..b196fcc 100644
--- a/src/Sqlite/SqliteQuery.php
+++ b/src/Sqlite/SqliteQuery.php
@@ -73,7 +73,7 @@ class SqliteQuery extends PdoQuery
                        case 'querySet':
                                $query = $this->querySet;
 
-                               if ($query->order || $query->limit || $query->offset)
+                               if (0)//$query->order || $query->limit || $query->offset)

Then run phpunit by php phpunit Tests/Sqlite/SqliteDriverTest.php

Then I got:

Joomla\Database\Tests\Sqlite\SqliteDriverTest::testQuerySetWithUnionAll
Error: Wrong parameters for Joomla\Database\Exception\PrepareStatementFailureException([string $message [, long $code [, Throwable $previous = NULL]]])

Maybe override constructor for PrepareStatementFailureException could be better?

mbabker · 2018-05-11T23:57:43Z

If it really comes down to that yes the constructor should be overloaded. Now I'm curious what is overloading core PHP's RuntimeException, or even root Exception, definition because last I was unaware the root constructor was not typed.

csthomas · 2018-05-12T00:01:12Z

php7.1 -a
Interactive mode enabled

php > new RuntimeException('', '');
PHP Warning:  Uncaught Error: Wrong parameters for RuntimeException([string $message [, long $code [, Throwable $previous = NULL]]]) in php shell code:1
Stack trace:
#0 php shell code(1): Exception->__construct('', '')
#1 {main}
  thrown in php shell code on line 1

mbabker · 2018-05-12T00:06:46Z

:eyeroll:

csthomas · 2018-05-12T14:51:23Z

It seems to be a known issue http://php.net/manual/en/class.pdoexception.php#95812

Should I add the constructor for PrepareStatementFailureException at this PR?

mbabker · 2018-05-12T14:56:06Z

Might as well.

csthomas · 2018-05-12T16:05:32Z

Done

csthomas mentioned this pull request Apr 16, 2018

Union once again #115

Closed

csthomas changed the title ~~[WIP] Keep order for set operators such as UNION / UNION ALL~~ Keep order for set operators such as UNION / UNION ALL Apr 20, 2018

This was referenced Apr 23, 2018

Add a deprecated flag in database query for union queries joomla/joomla-cms#20219

Merged

union query failed to implement #105

Closed

Handle union in DatabaseQuery #30

Closed

Fix union() query #12

Closed

alikon approved these changes Apr 25, 2018

View reviewed changes

Quy suggested changes Apr 27, 2018

View reviewed changes

mbabker requested a review from wilsonge May 1, 2018 01:41

alikon mentioned this pull request May 2, 2018

[4.0] JDatabaseQuery UNION gives unexpected result joomla/joomla-cms#20276

Closed

Tomasz Narloch added 9 commits May 10, 2018 10:28

Keep order for set operators such as UNION / UNION ALL

49f49c2

Add doc

2efd401

Fix doc

e76cc45

Improve unit tests

41f1801

Fix code comments and a few small fixes

4416df0

Introduce new methods querySet() and toQuerySet() to build more compl…

4f228d2

…ex unions

Fix code comments

49ace76

Add support for postgreSQL

9e1fbaf

Add support for SqlSrv

91274f8

Tomasz Narloch added 3 commits May 10, 2018 10:28

Add unit tests for PostgreSQL and SQLite

8e0244f

Fix docs in sqlite unit tests

35bec34

Working UNIONs with ORDER BY/OFFSET/LIMIT for SQL Server 2008

89f337c

csthomas force-pushed the db_union branch from 0632ccc to 89f337c Compare May 10, 2018 18:19

Tomasz Narloch added 10 commits May 11, 2018 15:24

Merge branch '2.0-dev' into db_union

6c58eb9

Fix unit tests

f919d72

Remove unnecessary wrapped query

816c1ff

Mysqli: add functional unit tests for querySet/toQuerySet with union …

ea4dc73

…all, limit and offset

PQSQL: add functional unit tests for querySet/toQuerySet with union a…

2419726

…ll, limit and offset

SQLite: add functional unit tests for querySet/toQuerySet with union …

dce137c

…all, limit and offset

PDOException::getCode() may return string like 'HY000', add cast to i…

d72a2fc

…nteger

SQLite - more bug fixes

552f1f6

SqlSrv: add functional unit tests for querySet/toQuerySet with union …

b231966

…all, limit and offset

Fix for sqlsrv query

b8dacd8

mbabker reviewed May 11, 2018

View reviewed changes

Remove debug info

4ffa603

Add a constructor to PrepareStatementFailureException

81db6ee

mbabker merged commit 83f016d into joomla-framework:2.0-dev May 12, 2018

csthomas deleted the db_union branch May 12, 2018 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep order for set operators such as UNION / UNION ALL #116

Keep order for set operators such as UNION / UNION ALL #116

csthomas commented Apr 16, 2018 •

edited

Loading

mbabker commented Apr 16, 2018

csthomas commented Apr 16, 2018

csthomas commented Apr 19, 2018

alikon commented Apr 25, 2018

csthomas commented Apr 25, 2018 •

edited

Loading

alikon commented Apr 25, 2018

Quy Apr 27, 2018

csthomas commented Apr 27, 2018

csthomas commented Apr 27, 2018

mbabker commented May 1, 2018

simbunch commented May 2, 2018

csthomas commented May 9, 2018

csthomas commented May 10, 2018

csthomas commented May 10, 2018

csthomas commented May 11, 2018

csthomas commented May 11, 2018

mbabker May 11, 2018

mbabker May 11, 2018

csthomas May 11, 2018

csthomas commented May 11, 2018

mbabker commented May 11, 2018

csthomas commented May 12, 2018

mbabker commented May 12, 2018

csthomas commented May 12, 2018

mbabker commented May 12, 2018

csthomas commented May 12, 2018

Keep order for set operators such as UNION / UNION ALL #116

Keep order for set operators such as UNION / UNION ALL #116

Conversation

csthomas commented Apr 16, 2018 • edited Loading

Summary of Changes

Testing Instructions

Documentation Changes Required

Joomla\Database\DatabaseQuery general changes

Changes in methods union(), unionAll() and unionDistinct()

Mix of union types

mbabker commented Apr 16, 2018

csthomas commented Apr 16, 2018

csthomas commented Apr 19, 2018

alikon commented Apr 25, 2018

csthomas commented Apr 25, 2018 • edited Loading

alikon commented Apr 25, 2018

Quy Apr 27, 2018

Choose a reason for hiding this comment

csthomas commented Apr 27, 2018

Option 1:

Option 2:

Common result:

csthomas commented Apr 27, 2018

mbabker commented May 1, 2018

simbunch commented May 2, 2018

csthomas commented May 9, 2018

csthomas commented May 10, 2018

csthomas commented May 10, 2018

csthomas commented May 11, 2018

csthomas commented May 11, 2018

mbabker May 11, 2018

Choose a reason for hiding this comment

mbabker May 11, 2018

Choose a reason for hiding this comment

csthomas May 11, 2018

Choose a reason for hiding this comment

csthomas commented May 11, 2018

mbabker commented May 11, 2018

csthomas commented May 12, 2018

mbabker commented May 12, 2018

csthomas commented May 12, 2018

mbabker commented May 12, 2018

csthomas commented May 12, 2018

csthomas commented Apr 16, 2018 •

edited

Loading

`Joomla\Database\DatabaseQuery` general changes

Changes in methods `union()`, `unionAll()` and `unionDistinct()`

csthomas commented Apr 25, 2018 •

edited

Loading