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

Closed
wants to merge 5 commits into
from

Projects

None yet

5 participants

@turrsis

No description provided.

@samsonasik samsonasik commented on an outdated diff Sep 21, 2013
library/Zend/Db/Sql/Select.php
@@ -459,6 +462,48 @@ public function combine(Select $select, $type = self::COMBINE_UNION, $modifier =
}
/**
+ * Create union clause
+ *
+ * @param Select $select
+ * @param string $type
+ * @param string $modifier
+ * @return Select
@samsonasik samsonasik commented on an outdated diff Sep 21, 2013
library/Zend/Db/Sql/Select.php
+ * @param string $modifier
+ * @return Select
+ */
+ public function except(Select $select, $modifier = '')
+ {
+ $this->combine($select, self::COMBINE_EXCEPT, $modifier);
+ return $this;
+ }
+
+ /**
+ * Create intersect clause
+ *
+ * @param Select $select
+ * @param string $type
+ * @param string $modifier
+ * @return Select
@ralphschindler
Zend Framework 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

This PR is allow :

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

Current implementation allow only one UNION

@ralphschindler
Zend Framework 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

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
Zend Framework 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

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

@samsonasik samsonasik commented on an outdated diff Nov 22, 2013
library/Zend/Db/Sql/Select.php
@@ -459,6 +462,48 @@ public function combine(Select $select, $type = self::COMBINE_UNION, $modifier =
}
/**
+ * Create union clause
+ *
+ * @param Select $select
+ * @param string $type
@samsonasik
samsonasik Nov 22, 2013

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

@samsonasik samsonasik commented on an outdated diff Nov 22, 2013
library/Zend/Db/Sql/Select.php
+ * @param Select $select
+ * @param string $type
+ * @param string $modifier
+ * @return self
+ */
+ public function union(Select $select, $modifier = '')
+ {
+ $this->combine($select, self::COMBINE_UNION, $modifier);
+ return $this;
+ }
+
+ /**
+ * Create except clause
+ *
+ * @param Select $select
+ * @param string $type
@samsonasik
samsonasik Nov 22, 2013

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

@samsonasik samsonasik commented on an outdated diff Nov 22, 2013
library/Zend/Db/Sql/Select.php
+ * @param Select $select
+ * @param string $type
+ * @param string $modifier
+ * @return self
+ */
+ public function except(Select $select, $modifier = '')
+ {
+ $this->combine($select, self::COMBINE_EXCEPT, $modifier);
+ return $this;
+ }
+
+ /**
+ * Create intersect clause
+ *
+ * @param Select $select
+ * @param string $type
@samsonasik
samsonasik Nov 22, 2013

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

@weierophinney
Zend Framework 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 milestone: 2.4.0, 3.0.0 May 12, 2014
@weierophinney
Zend Framework member

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

@turrsis

@weierophinney docblocks added

@turrsis

@weierophinney Can you merge this (docblocks added)?

@Ocramius Ocramius assigned Ocramius and unassigned weierophinney Nov 12, 2014
@Ocramius Ocramius referenced this pull request in zendframework/zf2-documentation Nov 12, 2014
Open

Document new features introduced in zendframework/zf2#5142 #1390

@Ocramius
Zend Framework member

This feature needs documentation: opened an issue for that.

@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - cleaning up docblocks 3375ad0
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - turned `protected` members into privates, as the class has st…
…andalone functionality and is based on `AbstractSql`
b98a01e
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - fixed static analysis warnings 88066b5
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - Expecting exceptions when passing invalid select statements t…
…o `Zend\Db\Sql\Combine#combine()`
6d59e88
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - Cleaning up static introspection analysis results in `Combine…
…Test` implementation
33955c3
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius #5142 - Reverting inherited property visibility, more detail in excep…
…tion messages, reverting `void` method return value (non-void)
d21a772
@Ocramius Ocramius added a commit that referenced this pull request Nov 12, 2014
@Ocramius Ocramius Merge branch 'feature/#5142-allow-multiple-combined-union-statements'…
… into develop

Close #5142
82504c2
@Ocramius
Zend Framework member

Manually merged in zendframework/zendframework@82504c2

Thanks!

@Ocramius Ocramius closed this Nov 12, 2014
@turrsis turrsis deleted the unknown repository branch Nov 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment