Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Added DISTINCT on Zend\Db\Sql\Select #3455

Closed
wants to merge 3 commits into from

Conversation

twmobius
Copy link

I've added DISTINCT on Zend\Db\Sql\Select as a new method to support queries like 'SELECT DISTINCT * FROM foo'

Also updated the unit tests to pass after the changes and added a new test for the DISTINCT feature.

This fixes partially #2509

@@ -91,6 +93,11 @@ class Select extends AbstractSql implements SqlInterface, PreparableSqlInterface
protected $prefixColumnsWithTable = true;

/**
* @var null|bool
*/
protected $distinct = null;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be false by default?

@twmobius
Copy link
Author

Yes I guess it makes more sense.

*/
public function distinct()
{
$this->distinct = true;
Copy link
Member

Choose a reason for hiding this comment

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

Argument to set to true / false

@marc-mabe
Copy link
Member

Because MySQL supports more than that flags I would prefer to implement it as list of flags addable to a select like it was done by propel: [add|has|remove]Modifier() and [get|reset|add|remove]Modifiers().
Than it's possible to create all selects like SELECT [{MODIFIERS}] ...

Other servers supports at least the DISTINCT and ALL flags

@twmobius
Copy link
Author

Well this is probably more correct, however I've tried to replicate the behaviour of Zend_Db_Select of ZF1.

MSSQL has also the TOP %d modifier instead of LIMIT.

@ralphschindler
Copy link
Member

So that "thing" is called a "Set Quantifier", and I think the API should reflect that probably:

public function quantifier($quantifier = 'ALL')
{

}

quantifiers can be strings (literal expressions), predicates or arrays

DISTINCT and ALL are part of SQL92, but I don't think we should make a list as most server implementations will have their own. Usage would look like:

$select->quantifier('DISTINCT');

or for sql server, for example:
$select->quantifier(array('TOP ?' => 10));

Thoughts?

@marc-mabe
Copy link
Member

@ralphschindler If this isn't in conflict with limit it looks good to me.

$select->quantifier(array('TOP ?' => 10));
$select->limit(500);
// = ?

@ralphschindler
Copy link
Member

From a dialect/sql parser standpoint, there is no conflict. Now, for Sql Server, I can't promise the engine won't barf when it tries to build a query plan. Also, limit/offset is a sensitive subject on Sql Server specifically with regards to whatever version you're using. Sql Server got limit/offset support in Denali (latest version).

using $select->quantifier(array('TOP ?' => 10)); is using a SQL Server specific quantifier anyway, in much the same way that you could use any number of MySQL specific functions through an expression.

@weierophinney
Copy link
Member

@twmobius @marc-mabe and @ralphschindler Can the three of you come up with a plan of action, please? It looks like a different direction needs to be taken, but the solution is also getting quite out-of-date with master; we either need to move forward or close...

@ghost ghost assigned ralphschindler Feb 5, 2013
@twmobius
Copy link
Author

twmobius commented Feb 5, 2013

Sorry for taking too long to respond, but I've been really busy lately.
I like the approach proposed by @ralphschindler. I'll see if I can craft something up today.

@twmobius
Copy link
Author

twmobius commented Feb 5, 2013

I started tackling with this and first of, I see that there is a problem with having a Predicate\Expression to supply quantifiers like 'TOP %d'. The processExpression method quotes the values in an expression resulting in TOP [10] instead of the Literal TOP 10 that Sql Server expects.

Also to my understanding ALL | DISTINCT are mutually exclusive, therefore there shouldn't be any need for multiple quantifiers. (array as parameter)

@ralphschindler
Copy link
Member

@twmobius, I think all quantifiers are mutually exclusive, no? I will work up a PR for this.

@twmobius
Copy link
Author

twmobius commented Feb 5, 2013

@ralphschindler Well I've looked up transact-sql and you could have DISTINCT TOP(%d) but that could still be one single quantifier.

@ralphschindler
Copy link
Member

Ok, to start, lets go with a single quantifier, let me take a stab at this.

@ralphschindler
Copy link
Member

Please see the implementation in #3682

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants