Fix syntax of comment ordering subquery

Could you point us to where this is actually documented?

@jmpoure: you can really not call that query an "MySQLism", especially if you can't point us to where it is documented that you can't have an ORDER in a FROM-based subquery.

All comments regarding standard or non-standard aside, the fact that the query fails should be of primary concern. I can verify that the query fails in PostgreSQL. I tried the suggested alternative query in the parent issue and while that -does- work in PostgreSQL, it does not work in MySQL 5:

mysql> SELECT thread FROM comments WHERE cid IN (SELECT cid FROM comments WHERE nid = 307 AND status = 0 ORDER BY timestamp DESC LIMIT 1) ORDER BY thread DESC LIMIT 1;
ERROR 1235 (42000): This version of MySQL doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'

How about this?

 $result = db_query('SELECT thread FROM {comments} c INNER JOIN (SELECT cid FROM {comments} WHERE nid = %d AND status = 0 ORDER BY timestamp DESC LIMIT %d) c2 ON c.cid = c2.cid ORDER BY thread DESC LIMIT 1', $node->nid, $new_replies);

That at least didn't throw an error both in MySQL and PostgreSQL.

According to PostgreSQL's documentation,

ORDER BY Clause

The optional ORDER BY clause has this general form:

ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...]

The ORDER BY clause causes the result rows to be sorted according to the specified expression(s). If two rows are equal according to the leftmost expression, they are compared according to the next expression and so on. If they are equal according to all specified expressions, they are returned in an implementation-dependent order.

... snip ...

Note that ordering options apply only to the expression they follow; for example ORDER BY x, y DESC does not mean the same thing as ORDER BY x DESC, y DESC.

So it appears that multiple ORDER BY statements are legitimate.

Jay,

You should work off the CVS code. The current CVS for this statement is:

 $result = db_query_range('(SELECT thread
      FROM {comment}
      WHERE nid = :nid
        AND status = 0
      ORDER BY timestamp DESC)
      ORDER BY SUBSTRING(thread, 1, (LENGTH(thread) - 1))', array(':nid' => $node->nid), 0, $new_replies)
      ->fetchField(); 

Actually, can you test if this CVS code gives the same issues under PostgreSQL?

In fact, it seems like this is just a syntax issue:

(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY thread DESC LIMIT 1

... is an implicit FROM subquery. The syntax is apparently valid on MySQL, but not on PostgreSQL, so we should write:

SELECT thread FROM (SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY thread DESC LIMIT 1

No, I don't agree to multiple ORDER BY statements in postgres.
http://www.postgresql.org/docs/8.3/interactive/sql-select.html
talks about The ORDER BY clause (not clauses) and the syntax shows only
one (optional) ORDER BY clause per SELECT statement.:

The optional ORDER BY clause has this general form:
ORDER BY expression [ ASC | DESC | USING operator ] [ NULLS { FIRST | LAST } ] [, ...]

That means: you can have one or more specified expression(s)
(indicated by [, ...]), but only one ORDER BY clause

Instead of:

(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1 ;

it should simply say:
SELECT thread FROM comments WHERE nid = 63743 AND status = 0
ORDER BY timestamp DESC , thread DESC LIMIT 1 ;

@Norbert: see my comment #13, it's nothing more then a syntax issue: MySQL read the query as an implicit sub-query on FROM, which is what we want. Making the subquery explicit should solve the issue.

Instead of:
(SELECT thread FROM comments WHERE nid = 63743 AND status = 0 ORDER BY timestamp DESC LIMIT 136) ORDER BY thread DESC LIMIT 1 ;

it should simply say:
SELECT thread FROM comments WHERE nid = 63743 AND status = 0
ORDER BY timestamp DESC , thread DESC LIMIT 1 ;

No, those are not the same queries (see the difference in the LIMIT clauses).

Bumping to D7 to quick fix that there (and add a test for that...), before backporting.

#237509: PostgreSQL 8.3 error: multiple ORDER BY clauses in comment.module was a duplicate.

Folks!
(i'm author of bugreport and patch #237509 which is duplicate to this bug)

As I can see

1. PosgreSQL does not supports multiple ORDER BY
2. MySQL does not supports LIMIT within subquery
3. comment.module really needs two ORDER BYs (first select N latest comments, than select highest thread id in them)

The only way is to use
if ( $GLOBALS['db_type']=='pgsql')
{
pgsql query
}
else
{ mysql query.

So, patch is trivial, just combine my patch from http://drupal.org/node/462270 to these ugly if-s

@Alex Tutubalin:

MySQL does support LIMIT inside sub-queries (of course... the current code, that is an implicit subquery works). The only thing left to do is to roll a patch according to my suggestion in #13.

@Damien

This is different queries.
First one selects Top N threads by timestamp, than maximum thread ID in these N top-timestamp;
Second one will effectively selects latest (by timestamp).

Again, my solution is
- Left MySQL query as is (assuming that two order-by-s is doing right thing)
- Make special PgSQL query (subselect with Order-by/Limit)
Select between two queries at PHP level by GLOBALS[pg_type]

This should work, althought not beautiful solution.

@Alex_Tutubalin: Please read carefully. This query is what we want and it should work the same on both PostgreSQL and MySQL:

SELECT thread FROM (SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY thread DESC LIMIT 1

Have you check it on MySQL ?

Also, on PgSQL 8.3 you should use slightly different syntax
SELECT column FROM (SELECT ....) AS column ORDER BY... LIMIT...

SELECT thread FROM (SELECT thread FROM {comments} WHERE nid = %d AND status = 0 ORDER BY timestamp DESC LIMIT %d) AS last_threads ORDER BY thread DESC LIMIT 1

works perfectly on MySQL.

So, my patch from http://drupal.org/node/462270 should work.

tag for http://geek.joshwaihi.com/content/drupal-code-sprints-postgresql

tagging properly

attaching patch by Alex_Tutubalin, which i am testing at the Postres Code Sprint

So far i can't replicate this error - i've tried in D6 and D7
Can someone please add the steps to make this error occur? Then i can verify it is fixed by the patch.
(otherwise the patch looks very sane and safe for D6 - needs re-rolling for D7).

#27 was for drupal6

Now patch for D7

But suppose reply for comments are broken in d7

misbehaving bot

Bot passed all tests

Makes total sense, and the bot is happy.

There have been a lot of back and forth in this issue, so here is some clarification for the core committers.

The issue here is that we have a query that has an invalid syntax:

(SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) ORDER BY thread DESC LIMIT 1

It is an implicit FROM-based subquery (we first get the threads of the latest comments, then we order them by thread).

MySQL happily deals with this query, even if it is invalid, but PostgreSQL ignore the parenthesis and return a cryptic "Multiple ORDER BY clauses are not allowed).

The patch simply transforms this query into:

SELECT thread FROM (SELECT thread FROM {comments} WHERE nid = %d  AND status = 0 ORDER BY timestamp DESC LIMIT %d) AS thread ORDER BY thread DESC LIMIT 1

I post new issue related to comment ordering #529374: Reply on comment is broken
Maybe this one should be fixed first

Let's re-test

Looks like bot was broken so rtbc

Committed to HEAD. Thanks!

We need to backport this.

Reroll #27

Extra space in the SQL.

@jmpoure: Please read http://drupal.org/node/317.

So here reroll without extra space

And status change... sorry

Need another review to rtbc this!

looks good

Who tested this on an actual Drupal 6 setup on MySQL? I'm seeing people tested the standalone query which was formed into the patch and people also tested the patch on PostgreSQL, but nobody tested the patch on MySQL, right?

I don't get the same result between the new and old queries. New query also didn't pass the SQL-99 validator (http://developer.mimer.com/validator/parser99/index.tml)

@Dave I found no difference betwen old and new query on mysql 5.0.51a (debian). Please provide more info about your env

I've applied the patch to a D6 on both Postgres and mysql. The Postgres instance worked after the patch and not before. Mysql instance works either way.

The two queries return exactly the same results against mysql on a 50000 node generated test set with up to 150 comments per node and a random new_replies between 3 and 10.

The validator at mimer doesn't seem to like the ORDER BY or LIMIT in the subquery. I can't find anything in the spec forbidding them though.

Thanks, committed to Drupal 6 as well.