Combining explicit join syntax with multiple from-clause argument-list entries will generate broken SQL on MySQL

Here's a patched Doctrine\ORM\Query\SqlWalker that implements the additional parantheses around the from-clause argument-list.

Consider this prototype quality, for testing and verification purposes. I am sure it could be implemented more cleanly.

I have not yet taken the time to get the big picture of where this can be sustainably implemented in Doctrine 2 (specially being probably MySQL-specific), but I needed this to work quickly, so this is a first shot at it.

Fixed formating

It actually seems to be an SQL 2003 compliance thing.

http://bugs.mysql.com/bug.php?id=13551

See post from 8 Oct 2005 12:12.

It's becoming even worse since the problem is actually more general. Consider this query:

SELECT wpomOrder.documentDate, wpomOrder.documentID,
                                SUM(orderLine.value), SUM(orderLine.totalValue), SUM(orderLine.purchaseValue), SUM(orderLine.totalPurchaseValue)
                           FROM \persistentData\model\import\webProductionOrderManager\WPOMOrder wpomOrder,
                                \persistentData\model\import\webProductionOrderManager\XMLDocument xmlDocument
                           JOIN wpomOrder.inputComponentDataVersion webProductionOrderManagerInputComponentDataVersion
                           JOIN wpomOrder.orderLines orderLine
                          WHERE :dataVersion MEMBER OF webProductionOrderManagerInputComponentDataVersion.dataVersions
                            AND wpomOrder.destinationOutlet = :outlet
                            AND xmlDocument.wpomOrder = wpomOrder
                            AND wpomOrder.orderCreatorListXMLRow IN (SELECT orderCreatorListXMLRow
                                                                       FROM \persistentData\model\import\orderCreatorListXML\OrderCreatorListXMLRow orderCreatorListXMLRow
                                                                       JOIN orderCreatorListXMLRow.orderCreatorListXML orderCreatorListXML
                                                                       JOIN orderCreatorListXML.inputComponentDataVersion orderCreatorListXMLInputComponentDataVersion
                                                                      WHERE :dataVersion MEMBER OF orderCreatorListXMLInputComponentDataVersion.dataVersions
                                                                        AND orderCreatorListXMLRow.ABRECHNUNGSTYP = \'' . self::$creatorModeInvoices . '\')
                          
                          ORDER BY wpomOrder.documentDate

I do not want to bother you with the details of this data model. Suffices to say that WPOMOrder inherits from VersionedDataObject.

This will be compiled down to:

SELECT w0_.documentDate AS documentDate0, w0_.documentID AS documentID1, SUM(o1_.value) AS sclr2, SUM(o1_.totalValue) AS sclr3, SUM(o1_.purchaseValue) AS sclr4, SUM(o1_.totalPurchaseValue) AS sclr5
 FROM (WPOMOrder w0_) INNER JOIN VersionedDataObject v2_ ON w0_.dbID = v2_.dbID, XMLDocument x3_ INNER JOIN Document d4_ ON x3_.dbID = d4_.dbID
 INNER JOIN InputComponentDataVersion i5_ ON v2_.inputComponentDataVersion_dbID = i5_.dbID INNER JOIN OrderLine o1_ ON w0_.dbID = o1_.wpomOrder_dbID
 WHERE EXISTS (SELECT 1 FROM DataVersion_inputComponentDataVersions d6_ INNER JOIN DataVersion d7_ ON d6_.inputComponentDataVersion_dbID = d7_.dbID WHERE d6_.dataVersion_dbID = i5_.dbID AND d7_.dbID = '1') AND
 w0_.destinationOutlet_dbID = '1' AND d4_.wpomOrder_dbID = v2_.dbID AND w0_.orderCreatorListXMLRow_dbID IN (SELECT o8_.dbID FROM OrderCreatorListXMLRow o8_ INNER JOIN OrderCreatorListXML o9_ ON o8_.orderCreatorListXML_dbID = o9_.dbID LEFT JOIN VersionedDataObject v10_ ON o9_.dbID = v10_.dbID INNER JOIN InputComponentDataVersion i11_ ON v10_.inputComponentDataVersion_dbID = i11_.dbID
 WHERE EXISTS (SELECT 1 FROM DataVersion_inputComponentDataVersions d6_ INNER JOIN DataVersion d7_ ON d6_.inputComponentDataVersion_dbID = d7_.dbID WHERE d6_.dataVersion_dbID = i11_.dbID AND d7_.dbID = '1') AND o8_.ABRECHNUNGSTYP = 'Rechnungen')
 ORDER BY w0_.documentDate ASC

... which introduces ambiguity, because the inheritance is inserted before the second FROM-clause entry is added. Which means, the "first shot" fix I attached will not work for this case.

The solution would have to make sure that this kind of ambiguity on SQL 2003-compliant systems is generally eliminated from the generated queries, by proper use of parantheses in all situations.

Note that the second example runs on the patched SQLWalker, which still does not cover the inheritance declaration properly.
Without the patch, it would already produce wrong SQL in the first example, of course.

The query shown above needlessly queries the entity XMLDocument, which, fortunately, is no longer needed in this use case. This being fixed, to me it is no blocker bug, but there are cases where inheritance will actually be used in conjunction with multiple from-clause entries, and I am pretty sure such scenarios will arise, so I leave this query here in the comments to illustrate the problem.

Increased priority to critical.

Mind you, this bug causes Doctrine to regularly generate broken queries on MySQL. Without my local patch, it would be unusable for my use case.

I believe a fix for this would be crucial. Doctrine absolutely needs to generate SQL 2003 compatible FROM clauses.

Attached new patched SQLWalker, now extended to add parantheses around FROM clauses within subqueries. Again, this is prototype quality, but it apparently fixes the problem.

Increased priority to blocker.

Now I have a query that relies on multiple FROM-clause argument-list entries, combined with JOIN expressions, that relies on an inheritance join as well. This is the type of query I described in my comment from 01/Mar/11 05:56 PM. In such a case, Doctrine 2 will generate invalid code leading to the following MySQL Server Error:

SQLSTATE[42S22]: Column not found: 1054 Unknown column [...] in 'on clause.

The column mentioned exists, though, MySQL Server just can not properly understand the aliases given, because the generated query is ambiguous, and MySQL Server is smart enough to complain about it.

A quick fix would be to generally move inheritance join expressions past the regular FROM-clause entries, and use proper parantheses around the list of regular (i . e. non-join table-reference) FROM-clause argument-list entries.

Please fix this ASAP. This is fundamental.

This fails on PostgreSQL aswell, not on SQLite however. I cant run Oracle currently as my machine with that is broken.

The problem here is twofold:

1. Joins generated for inheritance hierachies have to always be wrapped in parenthesis.
2. Support for explicit parenthesis in FROM and JOIN has to be supported.

Support for SQL 2003 is not about magic parenthis work magically for use-case 2, it should just be possible to do:

SELECT p FROM (Doctrine\Tests\Models\Company\CompanyPerson p, Doctrine\Tests\Models\Company\CompanyEmployee s)
JOIN p.spouse s2
WHERE p.spouse = s.id

It is already possible to do this:

SELECT p FROM Doctrine\Tests\Models\Company\CompanyPerson p
JOIN p.spouse s2, Doctrine\Tests\Models\Company\CompanyEmployee s
WHERE p.spouse = s.id

Just tested a bit, it is not even possible to do automatic parenthesis around inheritance hierachies as the aliases go out of scope for the WHERE clause, in PostgreSQL there are other scoping errors with your patch. This all has to be explicit, which makes a fix for this more complicated.

Wrapped SQL Codes new, very annoying that they dont overflow (Jira annoyances)

Changed back to critical, this is not a blocker, you can always use Native SQL to get around limitations of DQL. Its not pretty, but its not a blocker to usage, given that you can rearrange the FROM and JOIN clauses one of your issues should be solved by existing code, only the inheritance persists (not sure if that not goes away also).

Thanks for taking care.

This way you should never have to deal with blockers, just by providing a way to directly use PDO, which you always should be able to use directly anyway.

My query is quite complex because I had to replicate certain sections to compensate for the lack of support for CASE expressions. So it would be a pain to translate it to SQL, not even talking of maintaining it. But yes, technically nothing is really a blocker, though it is fair to say that workarounds can be impractical.

I will have a try at the solution you proposed in your comment from 20/Mar/11 05:43 AM. The basic parantheses problem is solved by the local SQLWalker patch. But it does not cover the inheritance join, which gets generated right amid the regular FROM-clause entries, instead of at the end with the other JOIN expressions, thus generating broken SQL.
So a sustainable fix would have to fix the inheritance join problem. My guess is it would help to handle the JOIN expression of the inheritance join the same way as other JOIN expressions, and render the statements for both after the regular FROM-clause entries.

The complexity of this query does not stem from the select clause, but from the query and matching logic. That is not much more complicated in plain sql than in DQL. The complexity of using NativeSQL over DQL is imho complex fetch join or inheritance scenarios.

I consider something a blocker only if its a bug that has no temporary workaround.

Your SQL Walker patch breaks the testsuite (not the sql generation queries, but real functional tests), so i cannot apply it.

Inheritance i think cannot just be put to the end of all from clauses, i'd rather have explicit mechanisms such as:

FROM (InheritedEntity) a
FROM (InheritedEntity a JOIN a.related b)
FROM (InheritedEntity a, OtherEntity b) JOIN a.related c

I also have to check how JPQL or HQL solves this issue.

Hey, you have not seen the query yet...

But yes, I agree it would be possible, though annoying, to write any query in plain-SQL.

The "patch" is just a prototype, to illustrate the issue. For me it works. It could be, of course, that it breaks some other feature my application does not use.

As for the inheritance joins, I do not yet understand why they should be handled differently than any other joins. The RDBMS should be agnostic to the reason a join is done, it just knows the basic mechanism, which should be the same.

Same SQLWalker first-shot concept patch, now based on Version 2.0.3

Ben,

is it possible to just generate the code for the inheritance joins at the end of the FROM clause?

Combined with always adding parantheses around the list of regular FROM clause entries, I believe, this could very well solve the problem. And it could turn out to be just a little manageable change.
There would not be any need for explicitly adding parantheses then, from a user-programmer perspective.

I uploaded a new SQLWalker.

This is a first shot at handling inheritance joins just like regular joins (generating an expression after the non-join table references, rather than between them). On MySQL 5.0, the patched code seems to generate valid SQL for all the use cases of my application.

Could you please run the unit tests against it, to see if the changes break anything?

I just gave the patched SQLWalker a try on PostgreSQL 8.4, and it did not work. PostgreSQL would complain about syntax errors, as the parantheses are not expected. I will see later this day if PostgreSQL requires the additional parantheses at all. Maybe on PostgreSQL the queries work fine without the parantheses in the first place. Given that PostgreSQL complains about syntax errors when provided with the additional parantheses, this is to be expected, but I will let you know as soon as I know for sure.

Sorry for the late update. I had to fix a RDBMS-dependent UTF-8 issue in my application before being able to fully test the relevant queries with PostgreSQL

Now that I did, PostgreSQL seems to have the same problem like MySQL Server.

Take this DQL query for example:

SELECT vendorListXMLRow
  FROM \persistentData\model\import\vendorListXML\VendorListXMLRow vendorListXMLRow,
       \persistentData\model\import\parameterListXML\ParameterListXMLRow parameterListXMLRow
  JOIN vendorListXMLRow.vendorListXML vendorListXML
  JOIN vendorListXML.inputComponentDataVersion vendorListXMLInputComponentDataVersion
  JOIN parameterListXMLRow.parameterListXML parameterListXML
  JOIN parameterListXML.inputComponentDataVersion parameterListXMLInputComponentDataVersion
  WHERE :dataVersion MEMBER OF vendorListXMLInputComponentDataVersion.dataVersions
    AND vendorListXMLRow.ID = parameterListXMLRow.WERT
    AND :dataVersion MEMBER OF parameterListXMLInputComponentDataVersion.dataVersions
    AND parameterListXMLRow.ID = :parameterID

On PostgreSQL 8.4.7, it will get compiled down to:

SELECT v0_.ID AS ID0, v0_.DEBITORENSAMMELKONTENLISTEN_DATEINAME AS DEBITORENSAMMELKONTENLISTEN_DATEINAME1,
       v0_.RECHNUNGSKONFIGURATIONS_DATEINAME_INLAND AS RECHNUNGSKONFIGURATIONS_DATEINAME_INLAND2,
       v0_.RECHNUNGSKONFIGURATIONS_DATEINAME_AUSLAND AS RECHNUNGSKONFIGURATIONS_DATEINAME_AUSLAND3,
       v0_.dbID AS dbID4,
       v0_.vendorListXML_dbID AS vendorListXML_dbID5
  FROM VendorListXMLRow v0_, ParameterListXMLRow p1_
 INNER JOIN VendorListXML v2_ ON v0_.vendorListXML_dbID = v2_.dbID
  LEFT JOIN VersionedDataObject v3_ ON v2_.dbID = v3_.dbID
 INNER JOIN InputComponentDataVersion i4_ ON v3_.inputComponentDataVersion_dbID = i4_.dbID
 INNER JOIN ParameterListXML p5_ ON p1_.parameterListXML_dbID = p5_.dbID
  LEFT JOIN VersionedDataObject v6_ ON p5_.dbID = v6_.dbID
 INNER JOIN InputComponentDataVersion i7_ ON v6_.inputComponentDataVersion_dbID = i7_.dbID
 WHERE EXISTS (SELECT 1 FROM DataVersion_inputComponentDataVersions d8_ 
                INNER JOIN DataVersion d9_ ON d8_.inputComponentDataVersion_dbID = d9_.dbID
                WHERE d8_.dataVersion_dbID = i4_.dbID
                  AND d9_.dbID = $1)
   AND v0_.ID = p1_.WERT
   AND EXISTS (SELECT 1 FROM DataVersion_inputComponentDataVersions d8_
                INNER JOIN DataVersion d9_ ON d8_.inputComponentDataVersion_dbID = d9_.dbID
                WHERE d8_.dataVersion_dbID = i7_.dbID
                  AND d9_.dbID = $2)
   AND p1_.ID = $3 LIMIT 

... which will not work, because v0 can not be referenced by the first JOIN expression.

Here is the log:

2011-03-28 12:17:03 CEST ERROR:  invalid reference to FROM-clause entry for table "v0_" at character 440
2011-03-28 12:17:03 CEST HINT:  There is an entry for table "v0_", but it cannot be referenced from this part of the query.
2011-03-28 12:17:03 CEST STATEMENT: [same as above]

Adding parantheses around the non-join FROM clause argument-list entries the way it worked for MySQL 5.0 will not work here. PostgreSQL will complain about a syntax error:

ERROR:  syntax error at or near ", " LINE 6:    FROM (VendorListXMLRow v0_, ParameterListXMLRow p1_)

So the bottom line is that the problem does exist on PostgreSQL as well, but the quick fix that apparently worked for MySQL 5.0 will not work here.

Note that PostgreSQL claims very specific compliance with the relevant packages of the SQL 2008 standard, while the MySQL documentation is, as usual, broadly inaccurate and amateurish about that topic. It does not matter here, if version 5.0 or 5.1 of MySQL is concerned.

http://www.postgresql.org/docs/8.4/static/features.html

http://dev.mysql.com/doc/refman/5.0/en/standards.html

When in doubt, I would suggest to simply aim for SQL 2008 compliance (as later versions of the standard supersede the older ones), so as to not risk optimizing for MySQL peculiarities rather than for general compatibility. This should make it possible to define a single level of compliance, independent of the specific RDBMS.

Removed long PostgreSQL log line...

Are you still with me?

I am just super busy right now. Doing my best..

OK!

Hm, just tried a little bit. What about:

diff --git a/lib/Doctrine/ORM/Query/SqlWalker.php b/lib/Doctrine/ORM/Query/SqlWalker.php
index a14987e..0fb3e80 100644
--- a/lib/Doctrine/ORM/Query/SqlWalker.php
+++ b/lib/Doctrine/ORM/Query/SqlWalker.php
@@ -621,11 +621,15 @@ class SqlWalker implements TreeWalker
             $this->_rootAliases[] = $dqlAlias;
 
             $class = $this->_em->getClassMetadata($rangeDecl->abstractSchemaName);
+            if ($class->isInheritanceTypeJoined()) {
+                $sql .= '(';
+            }
+
             $sql .= $class->getQuotedTableName($this->_platform) . ' '
                   . $this->getSQLTableAlias($class->table['name'], $dqlAlias);
 
             if ($class->isInheritanceTypeJoined()) {
-                $sql .= $this->_generateClassTableInheritanceJoins($class, $dqlAlias);
+                $sql .= $this->_generateClassTableInheritanceJoins($class, $dqlAlias).')';
             }
 
             foreach ($identificationVariableDecl->joinVariableDeclarations as $joinVarDecl) {

I think this query will work for you without any changes to the SQLWalker:

SELECT vendorListXMLRow
FROM
    \persistentData\model\import\vendorListXML\VendorListXMLRow vendorListXMLRow
        JOIN vendorListXMLRow.vendorListXML vendorListXML
        JOIN vendorListXML.inputComponentDataVersion vendorListXMLInputComponentDataVersion,
    \persistentData\model\import\parameterListXML\ParameterListXMLRow parameterListXMLRow
        JOIN parameterListXMLRow.parameterListXML parameterListXML
        JOIN parameterListXML.inputComponentDataVersion parameterListXMLInputComponentDataVersion
WHERE :dataVersion MEMBER OF vendorListXMLInputComponentDataVersion.dataVersions
 AND vendorListXMLRow.ID = parameterListXMLRow.WERT
 AND :dataVersion MEMBER OF parameterListXMLInputComponentDataVersion.dataVersions
 AND parameterListXMLRow.ID = :parameterID

Hey thanks, I'll try doing it that way.

Right now everything works fine with the local patch, at least on MySQL 5.0.

As for PostgreSQL, I will give it a run in a spare minute and let you know.

Any feedback on this issue?

I have not yet managed to change and test the queries of the application, because it is a major change affecting lots of complex queries that span literally several dozen pages each (we had to emulate the unsupported CASE statements with conditional logic and joins, which multiplied the lines of code in many queries).
I have postponed changing the queries until next week, when I will upgrade the application to Doctrine 2.1 after finishing some due tasks. Afterwards I will run the application code with changed queries according to the pattern described in your comment above, and let you know. I will give it a try on PostgreSQL then, too. Last time I checked, my local patch did not work on PostgreSQL. I hope the rewritten queries will.

I think your local patch is not necessary, you have to order the from and join clauses correctly (as required when writing SQL itself). This problem is "handled" by DQL by delegating the error handling to the SQL level as we don't evaluate semantical errors of SQL in DQL.

Sure, with the latest version it could work that way. I'll try, as I said.

The local SQLWalker patch was just a temporary measure. I hope it will become unnecessary after rewriting the queries the way you proposed.

Sorry for the delay. I am now upgrading to Doctrine 2.1 and rewriting the application's queries according to the way you explained. I will let you know about results within the next couple of days, after performing test runs based on the reworked queries.

I rewrote all queries to match the basic form you showed in your comment from April, 3rd.

Using the basic form

SELECT x
FROM drivingTable1 alias1
JOIN alias1.otherTable1 alias2,
drivingTable 2 alias3
JOIN alias3.otherTable2 alias4
...

the queries now work on MySQL Server without any local modifications of Doctrine 2.

I tested it with MySQL Server version 5.0.51a, and Doctrine version 2.1.1.

So far everything seems to be good.

I will try it on PostgreSQL, too, and let you know about that.

I tried a complex sample query on PostgreSQL using the same join syntax, and it worked like a charm.

I'll close the issue as fixed.

Thanks a lot.

Issue closed.

I don't know about the DQL validation part, though. The fact that DQL can or could compile to something that is not valid SQL could be considered symptomatic of a bug by itself. Don't know about the state of that, maybe it's more robust by now.