SelectQuery::countQuery() incompatible with UNION queries

+

I have also come across this issue in 7.4.

This problem still exists in 7.12.

Here's a possible fix for the bug, added to the tests from #2. I actually tried to fix this by ensuring that each individual SELECT query in the UNION is rewritten down to 1 field in the same way as the first query is. It seemed to work in my testing.

This will need to be moved to Drupal 8, but here are patches for Drupal 7 for starters to let the testbot run against them first.

Patches for Drupal 8.

Tests got moved around when Drupal got PSR-0ed, so the D8 patches no longer apply. It should be just a matter of finding where the database tests ended up and rerolling.

I am going to test the D7 patches, even though they can't go in till the D8 patches are fixed.

#7: core-union-count-query-1145080-7-TESTS-ONLY.patch queued for re-testing.

#7: core-union-count-query-1145080-7.patch queued for re-testing.

I just tested the D7 patches in all three supported databases and it works for all of them. I will be rerolling the D8 patches soon.

Ok, here are the new D8 patches for this. The only modification that I made was to change the path to the new test location. I have tested them in all three supported databases.

Marking 'needs review'.

I knew the first one would fail, why are you marking my post as 'needs work' silly System Message. :)

Looks great to me. Couple minor docs points we can adjust:

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -583,6 +583,21 @@ class Select extends Query implements SelectInterface {
   public function countQuery() {

We should add a docblock for this since we are updating it. Check to see if this method exists in the base class or interface and add the appropriate one-line docblock if so.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -1557,6 +1557,28 @@ class DatabaseSelectTestCase extends DatabaseTestCase {
+   * Test that we can get a count query for a UNION Select query.

Should say "Tests."

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -1557,6 +1557,28 @@ class DatabaseSelectTestCase extends DatabaseTestCase {
+    $query_1 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', array(27, 28), 'IN');
+
+    $query_2 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', 28);
+
+    $query_1->union($query_2, 'ALL');
+    $names = $query_1->execute()->fetchCol();
+
+    $query_3 = $query_1->countQuery();
+    $count = $query_3->execute()->fetchField();

An inline comment or two here might be nice.

-3 days to next Drupal core point release.

A patch with a little more docs.

Update: This is just a patch of the patch isn't it?

#18: core-union-count-query-1145080-14.patch queued for re-testing.

I believe the test fail was related to #1655422: Random test failures in SearchCommentTest, and has nothing to do with this patch.
The new Docs look fine to me, it applies, and I have already done several tests on PostgreSQL and SQLite to make sure it works there, so I think that this can be marked RTBC.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.phpundefined
@@ -282,6 +282,28 @@ class SelectTest extends DatabaseTestBase {
+  function testUnionCount() {
+    $query_1 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', array(27, 28), 'IN');
+
+    $query_2 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', 28);
+
+    $query_1->union($query_2, 'ALL');
+    $names = $query_1->execute()->fetchCol();
+
+    $query_3 = $query_1->countQuery();
+    $count = $query_3->execute()->fetchField();
+
+    // Ensure the counts match.
+    $this->assertEqual(count($names), $count, "The count query's result matched the number of rows in the UNION query.");

Wouldn't this test be better if the second query used a value other than 27 or 28?

Yes, but since that value is hard coded throughout the select tests, the change would be much more far reaching than this patch. I opened #1703418: Use test values other than 27 for age.

I changed the condition in the second query to be 29 and the test still passed

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -282,6 +282,28 @@ class SelectTest extends DatabaseTestBase {
+  function testUnionCount() {
+    $query_1 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', array(27, 28), 'IN');
+
+    $query_2 = db_select('test', 't')
+      ->fields('t', array('name', 'age'))
+      ->condition('age', 29);

@Dries, is that what you wanted?

Wouldn't this test be better if the second query used a value other than 27 or 28?

I don't think so, because in my opinion the test needs at least one duplicate value in there to see that that duplicate value in not duplicated in the result.

However there could be another value in there, such as:

$query_2 = db_select('test', 't')
      ->fields('t', array('name', 'age'))
      ->condition('age', array(28, 29));

Minor points for cleanup during today's office hours:

+++ b/core/lib/Drupal/Core/Database/Query/Select.php
@@ -582,7 +582,28 @@ class Select extends Query implements SelectInterface {
+  /**
+   * Get the equivalent COUNT query of this query as a new query object.
+   *
+   * @return Drupal\Core\Database\Query\SelectInterface
+   *   A new SelectQuery object with no fields or expressions besides COUNT(*).
+   */
   public function countQuery() {
+    $count = $this->prepareCountQuery();

Does this method override a method on the parent class? If not, all we need to do is change "Get" to "Gets". If so, we should move these docs to the parent method instead.

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -282,6 +282,28 @@ class SelectTest extends DatabaseTestBase {
   /**
+   * Test that we can get a count query for a UNION Select query.
+   */
+  function testUnionCount() {

This is the hunk where we should change "Test" to "Tests". :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectTest.php
@@ -344,7 +366,7 @@ class SelectTest extends DatabaseTestBase {
   /**
-   * Test that aliases are renamed when duplicates.
+   * Tests that aliases are renamed when duplicates.
    */

This change is outside the scope of the patch and should be reverted.

Wouldn't this test be better if the second query used a value other than 27 or 28?

We are limited to the values that are provided in the installed schema. @ZenDoodles and I spoke about this last week.

I have fixed everything that xjm mentioned.

Here is the new patch.

Marked as needs review so that the testbot can run.

+1 for this patch it's a show stopper for an effort to re-vitalize the Views Union module #1789796: Pager applied to the View based on the parent Display. Looks like it's RTBC, marking it as such.

D7 version of the #28 patch.

Ugh! No good deed goes unpunished :) I didn't mean to set off the test bot with my D7 version of the patch in #31.

So to be clear
#28 = D8 patch
#31 is D7 patch

restting to RTBC.

This looks fine to me. Committed/pushed to 8.x, moving to 7.x for backport.

#31: core-union-count-query-1145080-31-D7.patch queued for re-testing.

Upon further review the test in the patch for D8 is retuning a false positive. Further the patch in #28 doesn't actually fix the problem as the result of countQuery() will always = 1.

In the test the UNION query will read something like this:

SELECT 1 as expression FROM `test` WHERE age in (27,28)
UNION
SELECT 1 as expression FROM `test` WHERE age in (28);

For both queries the result is 1 which means the result from the query is a single row with the value 1 as the UNION will automatically merge the duplicates. The test compares the count of fields which is always 1 with the value from fetchField() upon the first, and only, row which is a 1. Consequently the test cannot fail. The beauty of the patch in #28 is the # of fields will always be the same thus avoiding the most common issue with UNION statements, but it's simply not realistic :(

patches for D8 and D7 attached

Don't we need to update the tests as well then?

@catch, no as the test subquery kicked out by the countQuery() method will now look like this:

SELECT name, age FROM `test` WHERE age in (27,28)
UNION
SELECT name, age FROM `test` WHERE age in (28);

which will result in a count of 2.

Sorry my post may have mislead a bit. The real issue lies in the #28 patch not the test. It occurs to me we are not calling prepareCountQuery() recursively in my proposed patch so we could pull it out, not sure if it matters though.

These patches take out the prepareCountQuery() method, as it's not necessary. Either #40 or #37 will fix the original issue.

I have a possibly naive protocol question. #1145076: UNION queries don't support ORDER BY clauses is a related issue as it's dealing with DBTNG bugs with UNION queries and in the same file. Should it be merged with this patch?

moving to RTBC.

I'll repost the D7 patch on it's own and re-roll #1145076: UNION queries don't support ORDER BY clauses once this is committed to D8

Please don't RTBC your own patch.

My bad, I guess now I understand why core developers are always begging for people to review patches on podcasts, in blogs and such.

Haha yep :D You might want to stop by #drupal-contribute on IRC sometime, there's also a lot of people who engage in "patch review swapping" to help each other out.

Hm, I'm pretty sure the original patch I posted worked fine in the case of UNION ALL (which admittedly was the only situation I was even thinking about at the time... oops :)

So given that the existing test uses UNION ALL (and therefore should already work fine), perhaps we also need a test for regular UNION (a.k.a. UNION DISTINCT)? It should be possible to write one that fails currently but passes with the new patch.

-    if (!$count->distinct && !isset($having[0])) {
+    if (empty($unions) && !$count->distinct && !isset($having[0])) {
       // When not executing a distinct query, we can zero-out existing fields
       // and expressions that are not used by a GROUP BY or HAVING. Fields
       // listed in a GROUP BY or HAVING clause need to be present in the

I guess this is OK, although I'm not totally clear when it's OK to skip everything inside that if() statement (probably one of the reasons I tried not to change it originally!). Is that if() statement just a performance improvement? If so, it should be fine to skip, although if possible I'd think it would be nice to keep this behavior around in the case of UNION ALL, since as mentioned I above I believe UNION DISTINCT is the only place where there's a bug here anymore; UNION ALL should already work fine.

Either way, the code comment above (as well as the one below it, in the addExpression('1') part) should probably be expanded to mention what's going on with the union queries here. It's some pretty confusing stuff.

I'll work on the comment.

The main reason I skipped the if() is to avoid changing the fields in individual queries which could result in different fields in the individual queries for either UNION ALL or DISTINCT especially in cases where one of the queries has a GROUP BY that is no in others (admittedly an uncommon use case but then so is UNION ALL :)).

In Drupal\KernelTests\Core\Database\SelectTest we have the test:

  /**
   * Tests that we can get a count query for a UNION Select query.
   */
  function testUnionCount() {
    $query_1 = db_select('test', 't')
      ->fields('t', array('name', 'age'))
      ->condition('age', array(27, 28), 'IN');

    $query_2 = db_select('test', 't')
      ->fields('t', array('name', 'age'))
      ->condition('age', 28);

    $query_1->union($query_2, 'ALL');
    $names = $query_1->execute()->fetchCol();

    $query_3 = $query_1->countQuery();
    $count = $query_3->execute()->fetchField();

    // Ensure the counts match.
    $this->assertEqual(count($names), $count, "The count query's result matched the number of rows in the UNION query.");
  }

For D8 is this issue fixed. The tag "needs backport to D7" is attached. So I am putting this one back to D7.

Per #37, I think this is still a bug in Drupal 8. Count queries work for UNION ALL, but not UNION queries.

I've been working on a Drupal 7 project that's using $query->union() and this is also still a bug in Drupal 7 for both UNION ALL and UNION. It prevents you from being able to create a pager query with a union.

To answer the question in #45, as far as I can tell yes, removing those fields from the WHERE clause is to improve performance. The patch in #37 appears to be a more appropriate fix than the one that was committed as it solves for both UNION ALL and UNION.

This is indeed still broken if you use $query->union($secondQuery). The count query always returns 1.

It works fine for $query->union($secondQuery, 'ALL').

Should another bug be raised? I know this is an 8 year old issue that everybody considers resolved...

@Rob320: Please create new issue when you have a bug.

Specifying the "ALL" union type made my D7->D10 migration's counts actually meaningful. This is still a lingering issue.