-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Feature: Subqueries in the FROM section #5510
Conversation
@iRedds This looks really good - I try to stay away from the database layer so I'm calling for backup. 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition.
system/Database/BaseConnection.php
Outdated
@@ -837,7 +837,7 @@ abstract protected function _transRollback(): bool; | |||
/** | |||
* Returns a non-shared new instance of the query builder for this connection. | |||
* | |||
* @param array|string $tableName | |||
* @param array|BaseBuilder|Closure|string $tableName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems to be good, but the method name table()
and the variable name $tableName
don't make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenjis changing parameter name is bc break per php 8.0, ref https://3v4l.org/hpqdm#v8.0.14 vs https://3v4l.org/Gtdk2#v8.0.14
The current change is ok for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I was uncomfortable with the name of this table() method. However, the table() method barely made sense since it specified a table name.
I wondered if we could change the name (add a new method) to a better name if it could specify a subquery.
If no one is uncomfortable with table()
, I won't insist on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you suggest your own version of the method name?
I will say right away that the names of subquery
and fromSubquery
are not correct. In the first case, we get not a subquery, but the main query with the inclusion of a subquery. In the second case, it is associated with the QueryBuilder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createBuilder()
or newBuilder()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree with such method names, but I associate them with creating a new/clean instance, and now this is impossible due to the fact that the class constructor requires a table name.
For example, in Laravel, the DB::table() method takes a string, a closure, a builder in the $ table parameter. That is, in this case it does not differ from my version. Except for the array. One has to scatter a rake.
But there is also an undocumented DB::query() method that returns a clean instance of the QB.
Unfortunately, our implementation already uses the query() method for a raw request. And the inability to create a clean instance of QB is a headache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Laravel has such table()
method, then it might be not so bad to use table()
. Because CI4 has many similar methods with Laravel.
And how about that $db->table(null)
returns a new/clean QB instance?
If QB contructor's $tableName
accepts null, we can do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, it's interesting.
But in fact, the table()
method is an alias for the from()
method.
And then the construction $db->table(null)->from()
looks a little strange )
I'm not up to date on the release planning process, so I'm not clicking the merge button. But this PR looks ready. |
I want to apologize to the team, but I've reworked the implementation and request a review.
*I thought to add the $alias parameter to the table() method in order to correctly pass the subquery with an alias, $db->table($subquery, 'alias'); but for this I would have to change the ConnectionInterface. |
You don't need to apologize for improving on your work! I won't attempt a review right now but your description sounds great. Thanks for taking the extra time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a great addition! I like this way of handling it, also.
Can you add a test making sure that a fromSubQuery
and from
calls play nicely together?
system/Database/BaseConnection.php
Outdated
*/ | ||
public function newQuery(): BaseBuilder | ||
{ | ||
return $this->table('crunch')->from([], true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's with the crunch
table name? This seems to be getting a build instance using crunch
as the table name, not "a new instance of BaseBuilder without a table" as the docblock reads.
I'm assuming this is just to fill a fake table name need? If that's the case let's use some type of random string here instead to ensure we don't accidentally conflict with a table someone might have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All attempts to change the BaseBuilder code to create a really clean instance are just marked as "breaking changes".
I hope that someday we will remove the table name from the constructor.
But so far, as I wrote above, we have no other way to create an instance other than passing a temporary fake table name to the constructor.
What I want to say is that this is a big API decision that cannot be changed later and should be carefully considered by many maintainers/contributors. And my opinion is it seems the changed |
fix method docblock
Added test
Co-authored-by: kenjis <[email protected]>
BaseBuilder::from: skip empty table name
b6117b0
to
013e374
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm keeping my hands off the heavy DB stuff, but the code looks good and this PR is high quality. @kenjis to you
🥳 |
Description
This PR makes it possible to use subqueries created using the QueryBuilder instead of the table in the FROM section.
This is where we add a subquery to an existing table.::
Use the
$db->newQuery()
method to make a subquery the main table.::The following is the first interface. It has been modified along the way.
A subquery can be passed either directly as a variable or as an array
[BaseBuilder, 'alias']
to specify an alias.Only one subquery can be passed at a time.
This solution will add flexibility to the QueryBuilder and prepare the ground for other features.
Checklist: