| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | database system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | needs backport to D7 |
Issue Summary
Problem
[Note: issue #369314: Subselects don't work in DB conditions... handled only the IN case]
I was trying to construct a query with a subselect that looks like:
SELECT ...
FROM ...
WHERE ...
AND (SELECT COUNT(*) ...) = value)However, I did not entirely manage to get it to work.
Attempt 1:
$query->condition($count_subquery, $value, '=');Fails because $count_subquery is a SelectQueryInterface which is a QueryConditionInterface. And if the $field argument is a QueryConditionInterface it gets compiled and placed in the main query as is, thereby ignoring the $value and $operator arguments.
Attempt 2:
$query->condition($value, $count_subquery, '=');Fails because no brackets are placed around the subquery.
This leads to a workaround:
$query->condition($value, $count_subquery, 'IN');This happens to work because the IN operator places brackets around the value(s). However, it will not always be possible to use IN. There are times you may want to use a comparison operator other than '=', e.g. <, <=, >, >=. Also, at times, it may be necessary to compare 2 subselects (select objects whose average score is higher than the overall average score).
So, the error can be split into 2 situations:
1) SelectQueryInterface objects as the left hand side ($field operator) are not handled correctly.
2) SelectQueryInterface objects as the right hand side ($value operator) are only handled correctly when the operator happens to be 'IN'.
Proposed resolution
To solve 1)
Method DatabaseCondition::compile(): Add an additional check to this if:
if ($condition['field'] instanceof QueryConditionInterface) {Something like:
if ($condition['field'] instanceof QueryConditionInterface && !($condition['field'] instanceof SelectQueryInterface && isset($condition['value']))) {Subsequently the else branch needs to handle this situation, by adding:
- Compile on the $condition['field'] if it is a SelectQueryInterface.
- Brackets around that result of compile.
To solve 2)
- This will need some refactoring of the above mentioned else branch.
Remaining tasks
Can some experts regarding this part of Drupal confirm this is indeed an error and is a use case that is to be supported. Is the proposed solution in the correct direction. If so, I will try to write a patch including tests.
Comments
#1
Error is also in D8, solve there first.
#2
I can confirm this bug.
#3
Yep, it was never intended to support sub-queries used as scalars. Let's see how to add support for this in Drupal 8.
#4
I have updated the documentation at http://drupal.org/node/310086 to make it clear that for now this only works with IN.
#5
This patch includes tests that should test the proper processing of subqueries as scalars. It is test only, thus supposed to fail. Complete patch follows soon, running all tests once again locally.
@#3: It's OK, if it was not supposed to support these, but the way that placeholders are generated makes it very difficult (read next to impossible) to hack your own queries in by e.g. first processing them. So IMO, it should indeed support as much as possible.
#6
The last submitted patch, 1267508-subselects-test-only.patch, failed testing.
#7
This patch includes a solution as well. It is a rewrite of the compile() method to make it accept more situations. It will accept subconditions or subqueries at almost any place where a value is accepted. Though there is not a test for it, it should e.g. also accept 2 subqueries using the BETWEEN operator.
To make it even more flexible, we could accept arrays as operator, though that would expose the data structure used to represent operators. I'm not sure if we want that. On the other hand, it would allow for conditions using the more exotic operators like ANY, SOME, and ALL.
#8
This patch worked fine for me on manual testing on my use case.
+++ b/includes/database/query.incundefined@@ -1633,7 +1633,7 @@ class DatabaseCondition implements QueryConditionInterface, Countable {
+ if (!isset($operator) && !($field instanceof QueryConditionInterface)) {
Can you explain this change? Isn't it a valid use case to use condition($subquery, $values); ? Perhaps some kind of comment helps as well.
+++ b/includes/database/query.incundefined@@ -1727,67 +1727,97 @@ class DatabaseCondition implements QueryConditionInterface, Countable {
+ $arguments[$placeholder] = $value;
+ $value_fragment[] = $placeholder;
Just for similarity on all other places first the value/field_fragment is created then the argument, this should probably be switch here.
In general the code is now even a bit easier to understand and described in detail.
0 days to next Drupal core point release.
#9
$query->condition($value, $count_subquery, '=');
a subquery returns a set, a set can't be equal to a scalar. What is this issue?
#10
OK I eat my words. Apparently scalar subqueries http://kb.askmonty.org/en/scalar-subqueries are a standard feature where SELECT returns 1 or 0 rows.
Hell knows whether this is ANSI. http://www.akadia.com/services/ora_scalar_subquery.html Oracle 9. http://www.databasejournal.com/features/mssql/article.php/3407911/Scalar... MS SQL back in 2004. And so forth.
#11
Damien tells me in IRC this is standard. SQL--
#12
I changed the patch as indicated in #8:
1)
The first remark is a good catch. My local code was already different... The reason to add the check has to do that compiling the conditions now can handle a condition like "subquery IS NULL". Formerly, $operator was always set to "IS NULL" and subsequently ignored when $field turned out to be a QueryConditionInterface. Now a SelectQueryInterface, which is also a QueryConditionInterface, can be compared to NULL.
The requested comments are added to the documentation of QueryConditionInterface::condition() as that did (already before this patch) not cover all accepted situations.
2)
The 2nd remark has been processed as indicated.
3)
White space removal is not removed from the patch this time as with the previous patch. So, it may be a bit harder to read but the result will be better :).
#13
I updated the patch:
As the methods condition(), where(), exists(), notExists() and conditions() are tightly coupled around the possibilities provided by this query builder, I added/changed also documentation for those methods, even if they may bot be changed in itself. This to further clarify when to use which method and what the options/restrictions of it are.
#14
Can this be rewritten now that #1321540: Convert DBTNG to namespaces; separate Drupal bits has landed or are there more disturbing patches to be expected soon?
#15
#7: 1267508-subselects.patch queued for re-testing.
#16
please ignore the above request.
#17
I want to do this:
SELECT sidFROM `uc_shipments`
WHERE order_id NOT
IN (
SELECT order_id
FROM `uc_orders`
)
...or...maybe better:
SELECT s.sidFROM `uc_shipments` s
WHERE NOT
EXISTS (
SELECT NULL
FROM `uc_orders` o
WHERE s.order_id = o.order_id
)
All of this is a bit over my head. Does the fact that this issue isn't fixed mean that I can't do this with the regular db_select() calls? See http://drupal.stackexchange.com/questions/18497/how-do-i-use-not-in-in-a....
#18
You can certainly do this with notExists($sub_query) ... in general: try to avoid posting somehow support questions in non-support issues. This doesn't really help to move the issue forward, so better test the patch for example.
#19
I would like to have another look at it and rewrite it to current head, but know that my time will be too limited to do it in the near future. I will reassign to me if I do find time to work on it though.