Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Union - updated functionality and unit test #1629

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
8 participants

This is an update to pull request joomla/joomla-platform#1539 and implements the changes required to make the general syntax

$q->select(a query)->union(another query)

generate correct SQL for a union query, which is not currently the case.

I've added a unit test for correct SQL in this instance, and modified the "testUnionClear" test to "testUnionNotClear", since "ORDER" can be used in this version of an SQL query.

Contributor

eddieajau commented Nov 12, 2012

This branch can't be merged. Please update.

I'm closing this pull request for now until the requested changes are uploaded. Please re-open the pull request (just post a reply with "reopen" in the message) at that time so we can review it again. Thanks.

@eddieajau eddieajau closed this Nov 12, 2012

Reopen.

Hi, could you tell me why this is marked as a "New Feature"? It isn't. It's fixing broken existing code in the core.
Whilst it is a slightly more esoteric feature than most people use, it is used in the core, in the extended search for example, which presumably doesn't work because of it. I haven't been able to work out how to test that though.

@elinw elinw and 1 other commented on an outdated diff Dec 8, 2012

tests/suites/unit/joomla/database/JDatabaseQueryTest.php
@@ -1575,12 +1593,13 @@ public function testUnionChain()
/**
* Tests the JDatabaseQuery::union method.
+ * does not clear the order clause
@elinw

elinw Dec 8, 2012

Contributor

Is this supposed to be part of the previous line? If not it should start with upper case. It seems like it might be a note actually and you may want a @note tag. Also, wouldn't it be best to have the option to clear or not clear if you are saying what is in the mysql manual doesn't apply any more?

@MarkRS-UK

MarkRS-UK Dec 10, 2012

Thanks, I'll correct the comment.

It's not that what's in the manual doesn't apply, I think the original situation arose out of a misunderstanding of the docs.
An order clause will have no effect when applied to one of the clauses, but it works just fine when applied to the entire result set, ie after any sub-clauses have been evaluated. It has always been like this.

@elinw elinw and 1 other commented on an outdated diff Dec 8, 2012

tests/suites/unit/joomla/database/JDatabaseQueryTest.php
@@ -380,6 +380,24 @@ public function test__toStringUnion()
}
/**
+ * Test use of union clause produces a full select
+ *
+ * @return void
+ */
+ public function test__toStringFullUnion()
+ {
+ $q = new JDatabaseQueryInspector($this->dbo);
+
+ $q->select('id FROM a')->union('SELECT id FROM b');
+
+ $this->assertThat(
+ (string) $q,
+ $this->equalTo("(\nSELECT id FROM a)\nUNION (SELECT id FROM b)"),
@elinw

elinw Dec 8, 2012

Contributor

I think we are moving to using PHP_EOL.

What woudl happen if you had
$q->select($db->qn('id)');
$q->from($db->qn('a');

rather than writing out the whole thing inside the select?

@MarkRS-UK

MarkRS-UK Dec 10, 2012

Fair point.

I have updated the test to use this syntax for both parts and then to create the union with $q->union($q1);
Is that what you meant? There are of course three variants, I could put them all in the test if you think it's a good idea. I notice there are some other "multiple" tests.

I have now my gitHub repository with the single test mentioned above, do I need to do anything else for you to see that?

As per elinw's comments I have updated the union test to use individual query methods rather than just writing the SQL in full. I hope this is what you had in mind. As noted elsewhere, since some test functions include more than one test, a later version could include all variants, if you felt this would be worth it.

In the meantime, job completed as requested. Over to you.

Could someone tell me if there's any likelihood of this code fix being put into the codebase in the near future.
I understand that you guys are busy, but so am I. It now needs rebasing, which I have already done once since I first submitted this request two months ago. If it's unlikely to get in before it needs doing again, it seems unhelpful for me to spend the time working on it.

Many thanks
M

Contributor

elinw commented Jan 19, 2013

I was thinking about this pr because I've been using the union method in a project.
First, I'm a bit confused about the order issue. As described on the referenced page at mysql You can have order inside any of the selects, and you can add order after the last parenthetical union but you definitely cannot have an order by as part of the union statement, which is what is removed with the clear in the current code here. As they say it has no effect, since any ordering must be done by the select or, of course, after you are done putting everything together. Now, what does have an effect is order + a limit (as discussed in the comments on the referenced page) so I guess I'm not against taking out the clear but I'd really like to also see the limit added although I'm not saying you should do it in his PR.

I think it's great that you are solving the annoying problem of having to set up the first query in the series which is indeed frustrating. As long as it doesn't break anything in the current implementation I think that's a good contribution.

By the way here's the code I did that made me think about this issue and how helpful it would be to fix the first statement problem.

https://github.com/elinw/joomla-cms/blob/tags2/components/com_tags/models/tag.php#L127

Having had to put the whole thing into a string then makes the surrounding parts less nice.

Hmm, no "LIMIT" method for Query? Of course, there isn't, why didn't I notice that before. Yes, it would be a good idea wouldn't it.

Your use of the UNION clause is exactly as I understand it. Isn't it presented to the database engine basically as

(SELECT STATEMENT) UNION ( SELECT STATEMENT) {... UNION (SELECT STATEMENT)} ORDER BY clause

?

The results of the entire selection can be ordered.
That's what your "'ORDER BY' . $db->qn($this->state->params->get('orderby', 'title')) ... " does.
Doesn't that mean, using your example, we should be able to write

                     $queryu->order($db->qn($this->state->params->get('orderby', 'title')) ...);

?
This would simplify things greatly because it would mean setQuery can then look after itself in this instance and manual string conversions wouldn't be required.

Adding a LIMIT method looks like it ought to be a very useful, and relatively simple, thing, would you like me to do that (in a separate pr)? At that point, of course, it's vital that "union" doesn't remove any order by clause.

In addition, are you sure that your query (with union) is producing the results you expect? This pr isn't really about the order clause, it's really to make sure that union statements work.

You can see that the current union functionality doesn't work at all by running the following example (that is designed to be as simple, and non-general, as possible) in a standalone environment.

$q2 = new JDatabaseQueryMysql();
$sq22 = new JDatabaseQueryMysql();
$q2->select('a')->from('test')->union($sq22->select('b')->from('test'));
echo '<p> . $q2 . '</p>';

I think you'll see that the union clause is not generated at all.

I'd be very interested to hear your results.

Contributor

dongilbert commented Jan 19, 2013

These are all reasons I use raw SQL when writing complex queries. I just created an Ajax autocomplete that queries 3 different tables for the entry title, and them returned the title and JRoute'd link. Piece of cake with raw SQL, couldn't imagine what it would have been with the query builder.

But those are the advantages of knowing the database you're going to be using. J! Core couldn't do that, because it supports multiple database types.

Sent from my iPhone

On Jan 19, 2013, at 9:46 AM, MarkRS-UK notifications@github.com wrote:

In addition, are you sure that your query (with union) is producing the results you expect? This pr isn't really about the order clause, it's really to make sure that union statements work.

You can see that the current union functionality doesn't work at all by running the following example (that is designed to be as simple, and non-general, as possible) in a standalone environment.

$q2 = new JDatabaseQueryMysql();
$sq22 = new JDatabaseQueryMysql();
$q2->select('a')->from('test')->union($sq22->select('b')->from('test'));
echo '

'.$q2.'

';
I think you'll see that the union clause is not generated at all.

I'd be very interested to hear your results.


Reply to this email directly or view it on GitHub.

Contributor

rvsjoen commented Jan 21, 2013

This union is not working as expected, the following query is returned

SELECT a FROM test

A brief look at the code indicates that the "type" value is not set to "union" when the member is called, and as a result the object is not treated as a union member. I have not tested the proposed fix but it does seem to address the problem with the missing query type.

Mark Stanton added some commits Oct 13, 2012

Mark Stanton Union functionality enhancement a1eede5
Mark Stanton Tests for updated union functionality 03cdff1
Mark Stanton Union functionality enhancement - correcting union method
Maintaining "order" and tidying up format according to official CS
71b970d
Mark Stanton Union functionality enhancement - correcting union method
Maintaining "order", tidying up format according to official CS, and providing better class usage.
ed29847
Contributor

elinw commented Jan 30, 2013

@rvsjoen Rune, the union works as expected for me, it should be returning UNION ( SELECT b FROM test) is what you are expecting for altogether SELECT a FROM test UNION (SELECT b FROM test) right?

Contributor

elinw commented Jan 30, 2013

It generates the query I expect which is set of unions.
e.g
SELECT id AS id,title AS title,state AS published,alias AS alias,created AS created_date,modified AS modified_data,introtext AS body,hits AS hits,publish_up AS publish_up,publish_down AS publish_down,access AS access FROM rbg48_content WHERE id IN ( 1,2,24 ) UNION ( SELECT id AS id,title AS title,state AS published,alias AS alias,created AS created_date,modified AS modified_data,description AS body,hits AS hits,publish_up AS publish_up,publish_down AS publish_down,access AS access FROM rbg48_weblinks WHERE id IN ( 11 )) UNION ( SELECT id AS id,name AS title,published AS published,alias AS alias,created AS created_date,modified AS modified_date,description AS body,hits AS hits,publish_up AS publish_up,publish_down AS publish_down,access AS access FROM rbg48_newsfeeds WHERE id IN ( 1 )) UNION ( SELECT id AS id,title AS title,published AS published,alias AS alias,created_time AS created_date,modified_time AS modified_date,description AS body,hits AS hits,null AS publish_up,null AS publish_down,access AS access FROM rbg48_categories WHERE id IN ( 36,19,27,17,32 ))

So it looks to me like the issue is really when people are using union inside of a chain, is that right?
And no, union itself does not enclose the first query in parentheses, though it would be okay to figure out a way to do that. The problem is basically you can easily put a set of parentheses after the names of many elements but you can't put the parentheses around the clause itself.

Contributor

rvsjoen commented Jan 30, 2013

You are right indeed that the problem occurs when call-chaining. I used the following piece of code for testing.

class Test extends JApplicationCli
{
    public function execute()
    {
        $q1 = new JDatabaseQueryMysql();
        $q2 = new JDatabaseQueryMysql();
        $q2->select('a')->from('test')->union($q1->select('b')->from('test'));
        var_dump($q2->__toString());
    }
}
Contributor

elinw commented Jan 30, 2013

Try this (updated)

    $q1 = new JDatabaseQueryMysql();
    $q2 = new JDatabaseQueryMysql();
    $q3 = new JDatabaseQueryMysql();
    $q4 = new JDatabaseQueryMysql();

    $q1->select('b')->from('test');
    $q3->union($q2->select('a')->from('test'));
            $q2->clear('select');$q2->clear('from');
    $q4->union(array($q2->select('a')->from('test'),$q1));
    echo '<p> $q1: ' . $q1 . '</p>';
    echo '<p> $q2: ' . $q2 . '</p>';
    echo '<p> $q3: ' . $q3 . '</p>';
    echo '<p> $q3->union: ' . $q3->union . '</p>';

    echo '<p> $q1 with $q3->union: ' . $q1 . ' ' . $q3->union . '</p>';
    echo '<p> $q4: ' . $q4->union . '</p>';

You can't really chain unions in that unions operate on complete queries, all it does it really take two complete queries and wrap a UNION () around the second (and any additional) ones. Chaining is going to attempt to add to the original query. UNION is an operator not a clause or a statement. Would you put AND in a chain like that?
But to me the thing I had to work around is that if you feed it an array all of the elements are prepended with UNION ( and I wonder if it shouldn't be changed to have the first element not prefixed with UNION ( but perhaps just with ( . Since you end up having to concatenate anyway maybe it makes more sense just to do that.

At the moment you get:
UNION (select x from y) UNION (select x from y)
thats wrong it should be
(select x from y) UNION (select x from y)
my patch uses the '()' prefix to remove "UNION" Prefixing.

Contributor

elinw commented Feb 11, 2013

You mean if you go through an array? As mentioned in the other thread I'd be happy to see that changed so you can run through an array of selects. It's a non b/c change of the api but one that I think makes sense. You don't actually need the first () but I think it makes it more readable.

You need the $name to be () because of line 70
https://github.com/ITronic/joomla-platform/blob/9f5b55344521ea2aa039f5e20ba0c106ab6fba46/libraries/joomla/database/query.php#L70

I think giving an array as parameter to union is useless, it would be better to use chaining

$q1 = new JDatabaseQueryMysql();
$q2 = new JDatabaseQueryMysql();
$q3 = new JDatabaseQueryMysql();
$union = new JDatabaseQueryMysql();

$q1->select('*')->from('x');
$q2->select('*')->from('y');
$q3->select('*')->from('z');
$union->union($q1)->union($q2)->union($q3);
Contributor

elinw commented Feb 12, 2013

It's not useless at all, as you can see in the linked code, it's very handy to have a generated array of selects and be able to use a foreach to union them to each other, which is why I like the idea of letting the first one not start with UNION. UNION supports both array and chaining by the way. How would you handle your use case if you did not know the number of selects in advance, you will still end up making an array.

What that () does is with a given element if you put () at the end of the name it takes the name and then puts () around the whole thing. Howevver that doesn't work for union because it's not that kind of element. It's an operator.

I know the () thing ;-)

I would not got the way with an array I would add the query directly to the union object

$union = new JDatabaseQuery();
$query = new JDatabaseQuery();

$query->select('id,name,transation');
$query->where('name like "%needed%"');
for($i = 2000; $i <= date('Y'); $i++) {
  $query->from('#__table_'.$i);
  $union->union(clone $query); //or toString()
}
$db->setQuery($union);

I don't see any real difference between chaining and feeding it an array of queries to create a union with an initial query. This is exactly what efEris' example above does, surely. It it was re-written to generate arbitrarily named queries for the union parts and then fed them all to the first query what would the difference be to the above?

And when you say (efEris) "At the moment you get UNION (something) UNION (something else)", do you mean with the code for this pull request? I do hope not.

Personally I don't like the first query inside parentheses, but that's just my opinion, it is within the MySQL syntax.

I'd say that UNION is an operator in the same sense that "AND" (for example) is, in which case

This
UNION that
UNION theOther

is perfectly sensible using chaining.

I personally don't your approach appending a query to a select query, I think you loose the readability but thats only my opinion

atm you get nothing if you use the union function of jdatabasequery
if my patch you get that whats the help text says

$db = JFactory::getDbo();
$union = $db->getQuery(true);
$query = $db->getQuery(true);
print_r($union);
$query->select('*');
$query->from('#_users');

$union->union($query);
$union->union($query);

print_r((string)$union); //ends in nothing atm

//adding $this->type = 'union'; to the union function results in
// UNION ( SELECT * FROM #_users) UNION ( SELECT * FROM #_users)
//thats wrong

//using my patch results in
//( SELECT * FROM #_users) UNION ( SELECT * FROM #_users)

And just for your information you could always give union an array with multiple queries because it is using the append function that handles arrays as expected.

Contributor

elinw commented Feb 13, 2013

Not sure what you mean by "get nothing" it works as described in the doclbocks which is appending whatever is that if you feed it a query it will wrap UNION () around it and append it to the existing query.
But I agree with coming up with a way to make the first one in an array not have UNION at least optionally because then you don't need the first query.
You have to remember that UNION is a different kind of thing than the elements that go inside of a SELECT because it encloses a select rather than being part of a select.

Ok, my fault I missed to read the first word in the docblock ;-)

But if you do this now you don't get a union statement:

$db = JFactory::getDbo();
$union = $db->getQuery(true);
$query = $db->getQuery(true);

$query->select('*');
$query->from('#_users');

$union->select('*');
$union->from('#__users');

$union->union($query);

print_r((string)$union);
// SELECT * FROM #__users
// but this should be 
// SELECT * FROM #__users UNION (SELECT * FROM #__users) 
// the problem is that the jquery::typ 'SELECT' doesn't do anything with the union part in the toString() function

So the patch from MarkRS-UK does the correct thing, except it could not handle the union only query (without select in it) for this it have to change the $name variable to '()'

Has anyone used the union function anywhere?

Yes, I'm trying to use the function query in a component I'm developing, which is how comes I've put in this fix.

A union without a preceding select is, of course, invalid SQL.
I'd say it's better that Joomla also fails if this is entered. What do you(s) think?

I would not think that Joomla should fail if you only have union queries in the jdatabasequery.

I would vote for your patch if you include the $name = '()'; from my patch, then both is working.

A union statement without a select section is invalid SQL.
Joomla shouldn't be correcting SQL errors.

Joomla should help the developer and should not confuse the the developer and produce invalid sql.

I you check my example its better to create a $union jdatabasequery and fill it with select queries, thats easy and clean.

But if you need to handle the first select query separately its more work for the developer and harder to read.

for example:

$union = new JDatabaseQuery();
$query = new JDatabaseQuery();

$query->select('id,name,transation');
$query->where('name like "%needed%"');

for($i = 2000; $i <= date('Y'); $i++) {
  $query->from('#__table_'.$i);
  $union->union(clone $query);
}

// without having only union queries you have to do this

$union = new JDatabaseQuery();
$query = new JDatabaseQuery();

$query->select('id,name');
$query->where('name like "%needed%"');
$query->from('#__table_2000');

$union->select('id,name');
$union->where('name like "%needed%"');

for($i = 2001; $i <= date('Y'); $i++) {
  $query->from('#__table_'.$i);
  $union->union(clone $query);
}

you have 4 lines more of code to write and it has duplicated code that could errors and is less readable.

And union with only one query would generate a select query with () around it without UNION operator and thats not invalid sql.

my opinion.

Contributor

dongilbert commented Mar 19, 2013

Is there a consensus on what to do here?

I'm not sure.
I don't see disagreement with what I've submitted, but also it's not been included, so I'm a little confused.
I s'pose you might say "he would think it's right", because the change seems to me self-evidently correct, but of course I definitely would. :)

Contributor

elinw commented Mar 22, 2013

Well the current implementation creates valid sql if you use it correctly but many people don't understand it and you are forced to do a toString one way or another. I was thinking that what we should really do is add union() and unionAll() --similar to our usage elsewhere-- so that with an array of items they would be able to get the sql people want and it would follow our standard usage from elsewhere in the class.

Also in terms of using, we use is in com_finder in the CMS. I was using unionAll in an earlier version of com_tags but it went away when we moved to the single table model instead.

Contributor

dongilbert commented Apr 14, 2013

This is the last PR preventing the tagging of 13.1 version of the platform. This is really up to the CMS on whether or not they would like this merged. As such, I will defer to @elinw, @mbabker or @dextercowley on this issue. Can you guys come to a conclusion as to what to do with this? IMO, it's a good fix that should be merged.

If we can get this decided, we can tag 13.1 and move on. :)

@dongilbert dongilbert closed this Apr 23, 2013

Contributor

chrisdavenport commented Dec 28, 2013

New PR to fix this issue: joomla/joomla-cms#2735

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment