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

Zend/Db/Sql/Select - implements multiple combine statements #5142

Closed
wants to merge 5 commits into from
Closed

Zend/Db/Sql/Select - implements multiple combine statements #5142

wants to merge 5 commits into from

Conversation

turrsis
Copy link
Contributor

@turrsis turrsis commented Sep 19, 2013

No description provided.

* @param Select $select
* @param string $type
* @param string $modifier
* @return Select
Copy link
Contributor

Choose a reason for hiding this comment

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

@return self

@ralphschindler
Copy link
Member

There's a lot going on in this PR to have so few details, is this a bug or a feature? And, if a bug, what is it attempting to solve?

@turrsis
Copy link
Contributor Author

turrsis commented Nov 19, 2013

This PR is allow :

SELECT ... UNION SELECT ... UNION SELECT ... UNION SELECT ... UNION SELECT ...

Current implementation allow only one UNION

@ralphschindler
Copy link
Member

Well, Postgresql does not support more than 2 sub-selects from being combined. That is one reason why Select was architected in the way it was.

Mainly though, from the perspective of the API, all of the other methods talk about modifying the state of a particular Select, then allow the merging to an external separate Select. I think a way of building a Multi-UNION Select would be to create a new class called SelectUnion/SelectCombined, where the only api is SelectCombine->addSelect(Select $select, $mode); (with additional methods like perhaps union, unionAll, intersect, minus, etc).

So basically, the new API would encourage this:

$selectA = new Select()->from()->where();
$selectB = new Select()->from()->where();
$selectUnion = new SelectCombine('UNION');
$selectUnion->addSelect($selectA)->addSelect($selectB);

instead of this:

$selectA = new Select()->from()->where();
$selectB = new Select()->from()->where();
$selectA->combine($selectB);

Thoughts?

@turrsis
Copy link
Contributor Author

turrsis commented Nov 19, 2013

Good idea, but :

$selectA = new Select();
if ('rainy weather') {
   $selectA->combine(new Select());
}
$selectA instanceof Select === true // and this is good!

But your idea is good because we can simply auto order columns for order.

@ralphschindler
Copy link
Member

I agree, $selectCombine instanceof Select should be true.
Another note, what are these NULLable columns, what does that have to do with combining sql statements?

@turrsis
Copy link
Contributor Author

turrsis commented Nov 22, 2013

For UNION statement columns should be ordered for all Select statements.
Very offen situation - column exist in one Select and absent in other, for resolve this we should use NULL columns.
We can use Expression for this, but the code will be unreadable.
For this added NULLable columns

* Create union clause
*
* @param Select $select
* @param string $type
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this @param string $type, not at the function parameter.

@weierophinney
Copy link
Member

@ralphschindler where does this stand in regards to milestone? 2.2.6? 2.3.0? later?

@ralphschindler ralphschindler added this to the 3.0.0 milestone Mar 4, 2014
@weierophinney weierophinney modified the milestones: 2.4.0, 3.0.0 May 12, 2014
@weierophinney
Copy link
Member

@turrsis This is looking reasonable. Can you please add docblocks to all class methods? Once that's done, I can merge.

@turrsis
Copy link
Contributor Author

turrsis commented May 13, 2014

@weierophinney docblocks added

@turrsis
Copy link
Contributor Author

turrsis commented May 24, 2014

@weierophinney Can you merge this (docblocks added)?

@Ocramius
Copy link
Member

This feature needs documentation: opened an issue for that.

Ocramius added a commit that referenced this pull request Nov 12, 2014
Ocramius added a commit that referenced this pull request Nov 12, 2014
…andalone functionality and is based on `AbstractSql`
Ocramius added a commit that referenced this pull request Nov 12, 2014
Ocramius added a commit that referenced this pull request Nov 12, 2014
Ocramius added a commit that referenced this pull request Nov 12, 2014
Ocramius added a commit that referenced this pull request Nov 12, 2014
…tion messages, reverting `void` method return value (non-void)
Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius
Copy link
Member

Manually merged in 82504c2

Thanks!

@Ocramius Ocramius closed this Nov 12, 2014
@turrsis turrsis deleted the hotfix/db_sql_multiple_combine_statements branch November 18, 2014 18:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants