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.

Sign up for GitHub

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

Jump to bottom

Add support for DISTINCT statement #3695

Closed

bingo-soft wants to merge 4 commits into doctrine:develop from bingo-soft:develop

Contributor

bingo-soft commented Oct 11, 2019

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

The same functionality as in Doctrine/ORM. New method distinct($flag = true) in QueryBuilder class and new test testSimpleSelectWithDistinct in QueryBuilderTest are provided.

bingo-soft added 3 commits

October 11, 2019 09:36


          Add support for DISTINCT statement

8495d05


          Fixed code style errors in Query Builder

eff22d6


          Fixed distinct method docstring

cd03fe2

Member

greg0ire commented Oct 12, 2019 •

edited

Loading

That functionality is already available by using select('DISTINCT u.id') correct? Is the point to be able to make it conditional easily? Can you provide a code sample that shows in what situation this will make your life easier?

Also, is develop the correct target branch when there are no BC breaks?

Contributor Author

bingo-soft commented Oct 12, 2019 •

edited

Loading

@greg0ire. There is a number of arguments for providing this functionality.

First of all, in SQL DISTINCT is a separate operation, similar to GROUP BY, but with a specific focus on eliminating duplicates. So, if we have groupBy in API, why not to provide a more granular distinct method?

Secondly, DISTINCT is a row operation, not a column function, so if you write higher level query language on top of Doctrine DBAL, which allows users to specify returned fields on the fly, it would be easier to apply distinct=true once, than to inject this logic into the part of the algorithm responsible for handling fields. So, two lines of code in Doctrine DBAL would save numbers of lines of client code (looping through all fields, injecting DISTINCT to the first field - $qb->addSelect("DISTINCT field_name"); if client does not provide the list of fields, then adding DISTINCT to * - like $qb->addSelect(DISTINCT *)). So, what is expected to be rather a row operation, becomes a column operation in DBAL client code.

In short, yes, the point is to make it conditional easily.

greg0ire requested changes

View reviewed changes

Member

greg0ire left a comment

Also, is develop the correct target branch when there are no BC breaks?

Please address this

lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated

+                   */
+                  public function distinct(bool $flag = true)
+                  {
+                      $this->sqlParts['distinct'] = (bool) $flag;

Member

greg0ire Oct 12, 2019

I don't think this cast is needed.

Suggested change

      
                    $this->sqlParts['distinct'] = (bool) $flag;
          
                    $this->sqlParts['distinct'] = $flag;

lib/Doctrine/DBAL/Query/QueryBuilder.php

+                   *
+                   * @return $this This QueryBuilder instance.
+                   */
+                  public function distinct(bool $flag = true)

Member

greg0ire Oct 12, 2019

Suggested change

      
                public function distinct(bool $flag = true)
          
                final public function distinct(bool $flag = true) : self

Contributor Author

bingo-soft Oct 12, 2019

Agree with self part, but not sure of final specificator. It would be the only one final method in QueryBuilder.

Member

greg0ire Oct 12, 2019

Yeah but it is one of the most recent ones, right? We can wait for the advice of another maintainer if you want, I will not merge this without the approval of some other maintainers anyway.

lib/Doctrine/DBAL/Query/QueryBuilder.php

                    */
                   private function getSQLForSelect() : string
                   {
-                      $query = 'SELECT ' . implode(', ', $this->sqlParts['select']);
+                      $query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') .

Member

greg0ire Oct 12, 2019

This is already supposed to be a boolean, and if happens not to be, the bug might still go undetected. If you feel unsafe about it being a boolean, then check that and throw if it isn't, or introduce a private accessor to do the type check. If you feel safe, then simply do this:

Suggested change

      
                    $query = 'SELECT ' . ($this->sqlParts['distinct'] === true ? 'DISTINCT ' : '') .
          
                    $query = 'SELECT ' . ($this->sqlParts['distinct'] ? 'DISTINCT ' : '') .

lib/Doctrine/DBAL/Query/QueryBuilder.php

@@ @@ -62,6 +62,7 @@ class QueryBuilder @@
                    * @var array<string, mixed>
                    */
                   private $sqlParts = [
+                      'distinct' => false,

Member

greg0ire Oct 12, 2019

I'd move it one line below, it looks like there this array is ordered with the same order in which you would find these keywords in an SQL query.

Contributor Author

bingo-soft Oct 12, 2019

Looks like that. You are right

greg0ire reviewed

View reviewed changes

lib/Doctrine/DBAL/Query/QueryBuilder.php

+                   *         ->from('users', 'u');
+                   * </code>
+                   *
+                   * @param bool $flag If true, then DISTINCT is added to the query

Member

greg0ire Oct 12, 2019

In what scenario is that parameter going to be useful?

Contributor Author

bingo-soft Oct 12, 2019

Do you mean, that there is no sense to mention this parameter in the docstring, since it is self evident? And by the way, as for develop branch, should I make a new merge request to the master branch to fix this?

Member

greg0ire Oct 12, 2019

No I mean, maybe we should remove the parameter entirely?

And by the way, as for develop branch, should I make a new merge request to the master branch to fix this?

You can edit your PR to target master, and rebase your branch on it: git rebase --onto master origin/develop develop (you should probably not have named your local branch develop BTW, it makes this a bit confusing :P)

Contributor Author

bingo-soft Oct 12, 2019

Ups, I've never done that magic yet and made another fork for another PR. So, I think I will close this one and start another PR, if you don't mind. And I will take into account all you suggestions, probably except final key word


          Remove unnecessary type casting in QueryBuilder distinct method

Member

greg0ire commented Oct 12, 2019

You should probably add something to https://github.com/doctrine/dbal/blob/master/docs/en/reference/query-builder.rst

bingo-soft closed this

bingo-soft mentioned this pull request

Add support for DISTINCT clause #3696

Merged

github-actions bot locked as resolved and limited conversation to collaborators

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

Labels

None yet