Add support for DISTINCT clause

[bingo-soft]

Q	A
Type	improvement
BC Break	no
Fixed issues

Summary

The same functionality as in Doctrine/ORM. New method distinct() in QueryBuilder class and new test testSimpleSelectWithDistinct in QueryBuilderTest are provided. This PR follows up the previous one with extended discussion - #3695

Closes #3340

[greg0ire]

Please squash all three commits together

[morozov]

Looks good but resetQueryPart() needs to be updated to be able to handle boolean parameters.

[bingo-soft]

resetQueryPart

I've not pushed it, yet. But it seems, like there is no need to tune resetQueryPart. This is the test case, that is ok:

public function testResetQueryPartWithDistinct() : void
{
    $qb = new QueryBuilder($this->conn);

    $qb->select('u.*')->distinct()->from('users', 'u');

    self::assertEquals('SELECT DISTINCT u.* FROM users u', (string) $qb);
    $qb->resetQueryPart('distinct');
    self::assertEquals('SELECT u.* FROM users u', (string) $qb);
}

BTW. In Doctrine ORM, which also supports distinct method, resetDQLPart does not handle specifically boolean parameters - https://github.com/doctrine/orm/blob/master/lib/Doctrine/ORM/QueryBuilder.php#L1456

[greg0ire]

I think @morozov is correct. Even though this works, it is unsafe because now the value is null, and when reading the code people will assume it is always a boolean.

BTW. In Doctrine ORM, which also supports distinct method, resetDQLPart does not handle specifically boolean parameters

We might have sinned in the past, let's not invoke consistency and repeat the same mistake. Our lives will be simpler if boolean parameters have 2 values and not 3.

[bingo-soft]

I will not insist, bet let me put my two cents in. I PHP context, null means that some variable is unset. And when reading the code people may also assume, that the variable is not true or false, but simply set or unset. And so, it is perfectly fine even for some boolean variable to be unset or null. So, probably we should make sqlParts['distinct'] => null by default? And check if it is not null in getSQLForSelect method? Many other sqlParts, like where or having are also compared to null, even though they are strings. And I think there is nothing special about boolean variable. So, my proposal is to change this

 private $sqlParts = [
    'select'  => [],
    'distinct'  => false,
    'from'    => [],
    ...
]; 

to this:

private $sqlParts = [
    'select'  => [],
    'distinct'  => null,
    'from'    => [],
    ...
]; 

and this

$query = 'SELECT ' . ($this->sqlParts['distinct'] ? 'DISTINCT ' : '') .
              implode(', ', $this->sqlParts['select']);

to this:

$query = 'SELECT ' . ($this->sqlParts['distinct'] !== null ? 'DISTINCT ' : '') .
              implode(', ', $this->sqlParts['select']);

[greg0ire]

And I think there is nothing special about boolean variable.

This is where we disagree. To me, boolean variables are very special: they are part of a finite set of only 2 values: true or false. To me null means something else: it means we don't have the data to make a decision. But in SQL, if DISTINCT is not specified, then it will not be applied. We can easily represent that by using false as the default value for distinct.

If you use bool as a return type for a method, then you have the guarantee you can use it in an if clause without additional type: if ($this->isDistinct()) {, and it reads very nicely. In #3340 , something similar was done, but what's not great about it is that it will not be reset by a call to resetDqlParts()

Instead of handling booleans in a generic way (you don't know if a boolean should default to true or false depending on if it's named $isGuilty or $isInnocent), here is what you could could do in:

    private const SQL_PARTS_DEFAULTS = [
        'select'   => [],
        'distinct' => false,
        'from'     => [],
        'join'     => [],
        'set'      => [],
        'where'    => null,
        'groupBy'  => [],
        'having'   => null,
        'orderBy'  => [],
        'values'   => [],
    ];

    /**
     * The array of SQL parts collected.
     *
     * @var mixed[]
     */
    private $sqlParts = self::SQL_PARTS_DEFAULTS;

    public function resetQueryPart($queryPartName)
    {
        $this->sqlParts[$queryPartName] = self::SQL_PARTS_DEFAULTS[$queryPartName];

        $this->state = self::STATE_DIRTY;

        return $this;
    }

resetQueryPart is nicely simplified :)

[bingo-soft]

resetQueryPart is nicely simplified :)

This was one of two ideas I had - 1) switching to null in $sqlParts or 2) creating a constant array. I think, both variants have their pros and cons. Let's see what other reviewers think and make final solution.

[greg0ire]

What cons do you see with 2) ?

[bingo-soft]

What cons do you see with 2) ?

Extra lines of code)

[greg0ire]

As you can see in my previous answer, it actually means fewer lines of code, doesn't it?

[bingo-soft]

As you can see in my previous answer, it actually means fewer lines of code, doesn't it?

Comparing to the current PR, switching to null for distinct in $sqlParts requires no extra lines of code. Your solution looks good, but introduses one new constant.

if ($this->isDistinct()) {

BTW. I'm not sure, that this public or private method is needed. We do not have similar check methods for other sql parts. But of course, I may be wrong. I'm still not sure whether null or false is better as a default value for distinct part of sql.

[greg0ire]

but introduses one new constant.

Yeah but the new constant is private, so no big deal, and it simplifies both $sqlParts and resetQueryPart()

BTW. I'm not sure, that this public or private method is needed. We do not have similar check methods for other sql parts. But of course, I may be wrong. I'm still not sure whether null or false is better as a default value for distinct part of sql.

I wasn't suggesting you add that method, I think it's fine to use the array directly

[greg0ire]

👌

[bingo-soft]

👌

@greg0ire. Should we squash these two commits into one?

[morozov]

👍

[morozov]

Thank you @bingo-soft and @greg0ire!