Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New: BaseBuilder. Added methods that implement UNION. #4291

Closed
wants to merge 5 commits into from
Closed

New: BaseBuilder. Added methods that implement UNION. #4291

wants to merge 5 commits into from

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented Feb 15, 2021

Description
Methods generate SQL queries using UNION [ALL]

  • BaseBuilder::union()
  • BaseBuilder::unionAll()
  • The orderBy() and limit() methods called after the union() / unionAll() method will be applied to the final result.
$builder = $db->table('movies');

$builder->select('title, year')
    ->unionAll(function (BaseBuilder $builder) {
        return $builder->select('title, year')->from('top_movies');
    })
    ->orderBy('title', 'DESC')
    ->get();

// SELECT title, year FROM movies UNION ALL SELECT title, year FROM top_movies ORDER BY title DESC

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@iRedds iRedds closed this Feb 16, 2021
@iRedds iRedds reopened this Feb 17, 2021
@iRedds iRedds marked this pull request as draft February 23, 2021 06:02
@iRedds iRedds marked this pull request as ready for review February 23, 2021 08:30
@MGatner
Copy link
Member

MGatner commented Feb 23, 2021

Anyone with deeper database experience want to take a look? @michalsn ?

@michalsn
Copy link
Member

Nice feature. It looks good to me. I only have some small doubts about the syntax.

What I mean is that using select() with limit() and order() before union()/unionAll() call. The thing is that the order of the used methods was never relevant to the Query Builder and now it becomes relevant.

Ideally, I would say that union queries should be built exclusively via closures but I think it might be not easy to implement.

To be more concrete, I would like to replace this:

$builder = $this->db->table('movies');
$builder->select('title, year')
	->orderBy('title')
	->unionAll(function (BaseBuilder $builder) {
		return $builder->select('title, year')
			->from('top_movies')
			->orderBy('title');
	});

with something like this:

$builder = $this->db->table('movies');
$builder
	->unionAll(function (BaseBuilder $builder) {
		return $builder->select('title, year')
			->orderBy('title');
	})
	->unionAll(function (BaseBuilder $builder) {
		return $builder->select('title, year')
			->from('top_movies')
			->orderBy('title');
	});

But I'm pretty sure it won't be that easy to implement... Anyway, it's just my first impression. If others think that there are no issues with the proposed syntax then I'm good with it too.

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case for unionAll() in Live tests?

@iRedds
Copy link
Collaborator Author

iRedds commented Feb 25, 2021

@michalsn Initially, there was a unionOrderBy() method to define the final result filter, but I abandoned it in favor of the classic use case with the call order condition.
There was another option

->unoin($closure [, $closure [, $closure....]])

But I decided that with this use the code would become unreadable.

Your version looks good, but there is one point. Some DBMSs support nested union queries.

(SELECT * FROM table UNION SELECT * FROM table) UNION SELECT * FROM table

For such a request, the code turns into closure-hell.

->unoin(function (BaseBuilder $builder) {
    return $builder->unoin(function (BaseBuilder $builder) {
        return $builder->select()->from();
    })->unoin(function (BaseBuilder $builder) {
        return $builder->select()->from();
    });
})->unoin(function (BaseBuilder $builder) {
    return $builder->select()->from();
});

At first glance, your version is not that difficult to implement.

@iRedds
Copy link
Collaborator Author

iRedds commented Feb 25, 2021

One point worries me, but I still haven't found a good solution. Therefore, your opinion interests.

Escaping is required when combining the results of queries using ORDER BY or LIMIT.

#The code will throw an ORDER BY + UNION error 
SELECT field, field2  FROM table ORDER BY field UNION SELECT field, field2 FROM table

#In MуSQL, you can wrap your query in parentheses 
(SELECT field, field2 FROM table ORDER BY field) UNION SELECT field, field2 FROM table 

But other DBMSs don't support this.
And the only option is to wrap it in a request with an alias.

SELECT * FROM (SELECT field, field2 FROM table ORDER BY field) as alias UNION SELECT field, field2 FROM table

In the current implementation, the wrapper looks like

SELECT * FROM (...query...) AS wrapper_alias

#example 
SELECT * FROM (SELECT field, field2 FROM table ORDER BY field) as wrapper_alias 
UNION SELECT * FROM (SELECT field, field2 FROM table ORDER BY field) as wrapper_alias

The code won't work without a wrapper. Dynamic aliases will prevent correct tests.

@michalsn
Copy link
Member

Your version looks good, but there is one point. Some DBMSs support nested union queries.
For such a request, the code turns into closure-hell.

Yes, nested union calls have the potential to become messy but this potential is valid probably for every discussed implementation - there are no huge differences to me.

I'm not saying we have to go with my proposal, but whatever we decide will gonna stay with us for some time, so I would like to hear some more opinions about it from others @MGatner @paulbalandan @samsonasik @kenjis

The code won't work without a wrapper. Dynamic aliases will prevent correct tests.

Up to my knowledge, you have chosen the best way to handle it across many databases. Is there is a scenario where we would need unique aliases names? If so, why not add a simple counter, like wrapper_alias_1, wrapper_alias_2? This should be manageable on the test side.

@iRedds
Copy link
Collaborator Author

iRedds commented Feb 25, 2021

Up to my knowledge, you have chosen the best way to handle it across many databases. Is there is a scenario where we would need unique aliases names? If so, why not add a simple counter, like wrapper_alias_1, wrapper_alias_2? This should be manageable on the test side.

I cannot yet imagine such a scenario with unique aliases.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

The thing is that the order of the used methods was never relevant to the Query Builder and now it becomes relevant.

This is very big change. I don't think this is a good change.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

(SELECT * FROM table UNION SELECT * FROM table) UNION SELECT * FROM table

Because of the complexity of the SQL, it seems inevitable that the code will be complicated to some extent.
Is this hell?

$builder->union(
        function (BaseBuilder $builder) {
            return $builder->union(
                function (BaseBuilder $builder) {
                    return $builder->select()->from();
                }
            )
            ->union(
                function (BaseBuilder $builder) {
                    return $builder->select()->from();
                }
            );
        }
    )
    ->union(
        function (BaseBuilder $builder) {
            return $builder->select()->from();
        }
    );

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

I found another interface example:

$result = $this->db
    ->union_start()
        ->select("'XX' AS country_code, '-- Not specified --' AS country_name")
    ->union_end()
    ->union_start()
        ->select("country_code, country_name")
        ->from('countries')
        ->where_in('country_code', array('US'))
        ->order_by('country_name', 'asc')
    ->union_end()
    ->union_start()
        ->select("country_code, country_name")
        ->from('countries')
        ->where_not_in('country_code', array('US'))
        ->order_by('country_name', 'asc')
    ->union_end()
    // For other cases: A place for clauses that affect the whole result: ORDER BY, LIMIT, etc.
    ->get()
    ->result_array();

https://forum.codeigniter.com/thread-68239-post-344270.html#pid344270

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

CakePHP

$inReview = $articles->find()
    ->where(['need_review' => true]);

$unpublished = $articles->find()
    ->where(['published' => false]);

$unpublished->union($inReview);

https://book.cakephp.org/4/en/orm/query-builder.html#unions

Laravel

$first = DB::table('users')
            ->whereNull('first_name');

$users = DB::table('users')
            ->whereNull('last_name')
            ->union($first)
            ->get();

https://laravel.com/docs/8.x/queries#unions

Yii

$query1 = (new \yii\db\Query())
    ->select("id, category_id AS type, name")
    ->from('post')
    ->limit(10);

$query2 = (new \yii\db\Query())
    ->select('id, type, name')
    ->from('user')
    ->limit(10);

$query1->union($query2);

https://www.yiiframework.com/doc/guide/2.0/en/db-query-builder#union

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

@lonnieezell says

And everyone seems to forget you can always bypass the query builder and simply use query() with query bindings to keep it safe. Simplifies things for everyone. Smile
https://forum.codeigniter.com/thread-68239-post-344274.html#pid344274

@iRedds
Copy link
Collaborator Author

iRedds commented Feb 26, 2021

$result = $this->db
    ->union_start()
        ->select("'XX' AS country_code, '-- Not specified --' AS country_name")
    ->union_end()

This is only a concept, not an implementation.

I could be wrong, but CakePHP doesn't solve the issue with orderBy and limit. Only with a construction like epilog() to put something at the end of the query.

In Laravel and Yii it is possible to get a QueryBuilder instance without binding to a table.
And the issue with orderBy and limit of the final result is solved through a similar construction

$newBuilderInstance->from($unionQueryes)->order()->limit();

There is no such tool in CI now.
And to implement it in the current PR is a little contradicting 1 PR = 1 feature

At the moment I have already prepared the implementation via

$builder->union()->union()->orderBy()->limit();

But I'm waiting for the community's opinion on the final concept.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2021

Can we do it this way?

$builder1 = $db->table('top_movies');
$builder1->select('title, year')->orderBy('title', 'DESC');

$builder2 = $db->table('movies');
$builder2->select('title, year')->unionAll($builder1)->get();

// SELECT title, year FROM movies UNION ALL SELECT title, year FROM top_movies ORDER BY title DESC

@michalsn
Copy link
Member

@kenjis I don't think it would solve a problem with orderBy and limit. We may want to add these to the movies table part or to the final query. So again the order of these methods would matter and we're trying to avoid it.

@iRedds Instantiating the QueryBuilder without binding to a table would be a nice feature. It probably would have to be implemented first, but it's still an option. We should wait and see the feedback.

@iRedds iRedds marked this pull request as draft March 12, 2021 22:58
@iRedds iRedds closed this Mar 20, 2021
@kenjis kenjis mentioned this pull request May 23, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants