[PATCH] Non-MySQL RDBMS cannot mix implicit and explicit JOIN types

Hi all,

Just do a quick test, changing from :

sf_simple_forum_forum, sf_simple_forum_category LEFT JOIN sf_simple_forum_post

to

sf_simple_forum_forum CROSS JOIN sf_simple_forum_category LEFT JOIN sf_simple_forum_post

is working, when using the cli ...

So looks more like a propel problems ?

regards

Caeies.

Hi all,

I have made the following change to propel :

--- lib/vendor/propel/util/BasePeer.php (revision 8506)
+++ lib/vendor/propel/util/BasePeer.php (working copy)
@@ -832,7 +832,7 @@
                $sql =  "SELECT "
                                .($selectModifiers ? implode(" ", $selectModifiers) . " " : "")
                                .implode(", ", $selectClause)
-                               ." FROM ". ( (!empty($joinClause) && count($fromClause) > 1 && (substr(get_class($db), 0, 7) == 'DBMySQL')) ? "(" . implode(", ", $fromClause) . ")" : implode(", ", $fromClause) )
+                               ." FROM ". ( (!empty($joinClause) && count($fromClause) > 1 && (substr(get_class($db), 0, 7) == 'DBMySQL')) ? "(" . implode(", ", $fromClause) . ")" : implode(" CROSS JOIN ", $fromClause) )
                                                                .($joinClause ? ' ' . implode(' ', $joinClause) : '')
                                .($whereClause ? " WHERE ".implode(" AND ", $whereClause) : "")
                                .($groupByClause ? " GROUP BY ".implode(",", $groupByClause) : "")

It's now working, but 1) I fear to have side effects even if it doesn't looks like in my apps; 2) That won't work with some others DB ... and I got no idea on how do to this properly ...

Regards,

Caeies.

I often come across this problem with PostgreSQL... and I haven't understood why before, but I've now looked into it further....

The problem is in combining the default implicit join (using the comma) with an explicit join, such as "LEFT JOIN". In both MySQL5.0.12+ and PgSQL8.0+ (and probably earlier versions), the explicit JOIN types take precedence over the implicit comma joins, as is the SQL2003 standard. This means that:

SELECT * FROM sf_simple_forum_forum, sf_simple_forum_category LEFT JOIN sf_simple_forum_post 
  ON (sf_simple_forum_forum.LATEST_POST_ID=sf_simple_forum_post.ID) 
  WHERE sf_simple_forum_forum.CATEGORY_ID=sf_simple_forum_category.ID

is actually executed as:

SELECT * FROM sf_simple_forum_forum, (sf_simple_forum_category LEFT JOIN sf_simple_forum_post 
  ON (sf_simple_forum_forum.LATEST_POST_ID=sf_simple_forum_post.ID))
  WHERE sf_simple_forum_forum.CATEGORY_ID=sf_simple_forum_category.ID

Therefore, since the 'ON' clause only operates on the tables being joined, the ON clause fails because the first column is not accessible in the second join, which the ON statement is part of.

The proper query should look like:

SELECT * FROM sf_simple_forum_forum JOIN sf_simple_forum_category LEFT JOIN sf_simple_forum_post 
  WHERE sf_simple_forum_forum.LATEST_POST_ID=sf_simple_forum_post.ID 
  AND sf_simple_forum_forum.CATEGORY_ID=sf_simple_forum_category.ID

In standard SQL, CROSS JOIN join conditions are specified with 'WHERE'. But it seems both PgSQL and MySQL support using CROSS JOIN with an ON condition.

The solution for this error in the plugin should modify the query that is causing the problem within the forum. It should be able to be fixed by simply changing the query to explicitly use Criteria::INNER_JOIN instead of the default implicit (comma) join.

Some background on related issues:

1. A change to fix this precedence issue for MySQL was made in [1336], by adding parenthesis.
2. It was discovered that this broke PgSQL queries entirely (see #557), so was again updated in [1457].
3. The issue was raised in Propel here.
4. That latest change was incorporated into Propel 1.2 Here, and now exists pretty much the same in Propel 1.3

Despite those changes, a fix to the precedence issue for non-MySQL RDBMS has not been developed.

I would like to see Caeies' patch tested on MySQL (different versions) to see if that works. If CROSS JOIN works as a perfect drop-in replacement for the implicit join on all RDBMS, then I see no reason not to use it. Then we could get rid of the RDBMS-specific hack that is in there now, as well.

I can confirm CROSS JOIN works in PgSQL (I'm running version 8.1.11)

I can now also confirm CROSS JOIN works in MySQL 3.23, 4.1, and 5.0

I have therefore attached a patch which gets rid of the MySQL-specific hack and incorporates the CROSS JOIN syntax as the default join type when there is an implicit join along with an explicit one.

I'm changing this to a model defect, as it's more general than just an issue with the forum plugin.

Should this be filed with the Propel trac instead? propel.phpdb.org

Yes, I think it should be filed with propel as well...

I guess the question is, how would this change get incorporated into SF...

I'm sure it can get fixed in Propel 1.3, but what about SF1.0.x users that rely on Propel 1.2? I'm not sure this patch would get incorporated into the 1.2 Propel codebase...

I applied a modified version of the CROSS JOIN patch to Propel 1.3 branch. Hopefully this won't have adverse affects. This can be tracked in Propel here: http://propel.phpdb.org/trac/ticket/283

Thanks!

This is fix in Propel 1.3, as this is a bug in propel. Please file and resolve at propel trac: propel.phpdb.org, when it is fixed in 1.2 we will update.