Added support for ORDER BY NULLS (FIRST|LAST) #100

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants

helmer commented Aug 2, 2011

I was recently having troubles with ordering of values that may contain NULLs. For that, I added such support to Lexer and Parser. Yet, these (NULLS, FIRST and LAST) are Postgre/Oracle specific identifiers.

What are your thoughts on this?

helmer commented Aug 2, 2011

Sorry about the whitespace trim, makes the diff ugly, please overlook

Owner

beberlei commented Aug 2, 2011

DQL is vendor neutral, so this feature won't be merged.

You sould add a custom SQL Walker instead, see here http://www.doctrine-project.org/docs/orm/2.0/en/cookbook/dql-custom-walkers.html

this way you don't have to touch the DQL Lexer/Parser at all.

@beberlei beberlei closed this Aug 2, 2011

Owner

beberlei commented Aug 2, 2011

The walker could work like the MySQL NoCache walker shown at the bottom of the article. Via $query->setHint("myplugin.orderby.nulls", "first") and use that query hint to modify the query appropiately.

Contributor

lenar commented Aug 2, 2011

Actually, for such a simple thing custom AST walker is pretty much overkill. Really.

In this case (and in many other cases) one additional everywhere-supported function would be sufficient: IF(expr, when-true, when-false). This would give the flexibility without custom-reimplement-everywhere-code-hell. As I see this is not currently implemented: http://www.doctrine-project.org/docs/orm/2.0/en/reference/dql-doctrine-query-language.html#dql-functions.

Owner

beberlei commented Aug 2, 2011

Your patch changes at least 20 real lines of code. I could write you a custom AST walker that does ORDER BY NULLS in 10 LOC. I dont see the overhead you claim it has? You dump code on us to maintain and didn't even add tests. Where is the benefit for Doctrine?

Additionally DQL gets vendor driven (which it is the exact difference it was designed for), it will never contain vendor features.

IF() is a completly different issue. That is supported by all vendors and could be added in a future version.

Contributor

lenar commented Aug 2, 2011

@beberlei: 10 LOC in 10, 100 or even 1000 projects is lot more than 20 in doctrine. That's first. Second, it's not really my code nor PR. Third, I didn't pry for reopening this PR. All I said is that this issue could've been no-op and solved by IF() if that was (already) supported without creating this 20 LOC PR. The remark was for future consideration.

Owner

beberlei commented Aug 2, 2011

I didn't say an SQL Walker is not something we could merge into Doctrine or distribute with Doctrine Extensions for example.

And yes, i mistakenly took you for the PR author, sorry for that :-)

helmer commented Aug 2, 2011

I mostly opened this PR for discussion, towards a feature that'd be nice to have (if not elementary), AND as it was already mentioned, it is indeed vendor specific.

Put that aside, comments like "you dump us stuff without tests" do not exactly make me eager to contribute furthermore. I did my best, opened it for thoughts, and it gets shot down immediately and aggressively.

I am not too familiar with Doctrine specifics like SQL walkers, which I'll look into tomorrow morning and hopefully propose a solution that makes everyone happy. Or, as @lenar suggested, perhaps a DQL function would suffice.

Pfft. Peace.

droletm commented Jun 20, 2012

here is the custom walker that I've build to do the trick:

<?php
namespace DoctrineExtensions\CustomWalker;

use Doctrine\ORM\Query\SqlWalker;

/**
 * SortableNullsWalker
 */
class SortableNullsWalker extends SqlWalker
{
    const NULLS_FIRST = 'NULLS FIRST';
    const NULLS_LAST = 'NULLS LAST';

    public function walkOrderByClause($orderByClause)
    {
        $sql = parent::walkOrderByClause($orderByClause);

        if ($nullFields = $this->getQuery()->getHint('SortableNullsWalker.fields'))
        {
            if (is_array($nullFields))
            {
                $platform = $this->getConnection()->getDatabasePlatform()->getName();
                switch ($platform)
                {
                    case 'mysql':
                        // for mysql the nulls last is represented with - before the field name
                        foreach ($nullFields as $field => $sorting)
                        {
                            /**
                             * NULLs are considered lower than any non-NULL value,
                             * except if a – (minus) character is added before
                             * the column name and ASC is changed to DESC, or DESC to ASC;
                             * this minus-before-column-name feature seems undocumented.
                             */
                            if ('NULLS LAST' === $sorting)
                            {
                                $sql = preg_replace('/\s+([a-z])(\.' . $field . ') (ASC|DESC)?\s*/i', " -$1 $2 $3 ", $sql);
                            }
                        }
                        break;
                    case 'oracle':
                    case 'postgresql':
                        foreach ($nullFields as $field => $sorting)
                        {
                            $sql = preg_replace('/(\.' . $field . ') (ASC|DESC)?\s*/i', "$1 $2 " . $sorting, $sql);
                        }
                        break;
                    default:
                        // I don't know for other supported platforms.
                        break;
                }
            }
        }

        return $sql;
    }
}

and I use it this way:

$query->setHint(Doctrine\ORM\Query::HINT_CUSTOM_OUTPUT_WALKER, 'DoctrineExtensions\CustomWalker\SortableNullsWalker')
                    ->setHint('SortableNullsWalker.fields',
                        array(
                            'last_updated_date' => DoctrineExtensions\CustomWalker\SortableNullsWalker::NULLS_LAST,
                        )
                    );

hope this help

Thanks @droletm for sharing your code. It didn't work for me in MySQL. If someone is interesed, I modified it to work in MySQL:

http://stackoverflow.com/questions/12652034/how-can-i-order-by-null-in-dql/14121479#14121479

guilhermeblanco added a commit that referenced this pull request Jan 23, 2013

@cram1010 thanks for your code. I tried it and it seems to work with a single ORDER BY field. There is however an issue once you have multiple field, and the configured field is not the first one.
I am not good enough in Regex to fix it.
To reproduce, once I debug into your code, my ORDER BY clause in the $sql variable is:

ORDER BY c0_.isSticky DESC, c0_.datePublished DESC

I set the nulls last hint only for the field datePublished and after the callback it becomes:

ORDER BY -c0_.isSticky DESC, c0_.datePublished ASC

instead of the expected

ORDER BY c0_.isSticky DESC, -c0_.datePublished ASC

Would be cool if you find this...

droletm commented Mar 11, 2013

@cram1010
you can try something like that:
$sql = preg_replace_callback('/ORDER BY (.+)'.'(-?'.$field.') (ASC|DESC)/i', function($matches) {
if ($matches[3] === 'ASC') {
$order = 'DESC';
} elseif ($matches[3] === 'DESC') {
$order = 'ASC';
}
return ('ORDER BY '.$matches[1].$matches[2].' '.$order);
}, $sql);

-? = optionnal - in front of the field name
and I've remove the - in the return string cause it will be part of the captured $matches[1] if it was in the query for that field.

I didn't test it. Have a try.
Hope this help.

@brainwasher

Whith this fix it behaves as expected:

$sql = preg_replace_callback('/ORDER BY (.+)'.'(-?'.$field.') (ASC|DESC)/i', function($matches) {
if ($matches[3] === 'ASC') {
$order = 'DESC';
} elseif ($matches[3] === 'DESC') {
$order = 'ASC';
}
$aux=explode(',',$matches[1]);
$aux[count($aux)-1]=" -" .trim($aux[count($aux)-1]);
$matches[1]=implode(",",$aux);
return ('ORDER BY '.$matches[1].$matches[2].' '.$order);
}, $sql);

Contributor

mvrhov commented Feb 19, 2014

@beberlei IMO this should be reconsidered it seems that this is supported by 2 most used open source DBs and even one closed one.

@beberlei I agree with @mvrhov . Since most databases support it, it could enter the abstraction layer. It might be added as an option to the sort functions. If there are Doctrine supported databases that do not support NULL sorting natively, there can still be a small generic sorter class which does it. At least it would be within the framework and implementations with that requirement would not need to implement storage dependend functionalties in their code.

Owner

beberlei commented Feb 19, 2014

The difference is that DQL currently works on ALL platforms. There is reason something like this is not supported, we want the language to work everywhere.

@beberlei Theoretically having a fallback spliting the query in two (one with nulls and other with not nulls) and joining them using a UNION ALL which is SQL stardard and should work on any SQL database, as described here
http://blog.sqlauthority.com/2012/10/30/sql-server-union-all-and-order-by-how-to-order-table-separately-while-using-union-all/
Another question is it's worth the complexity to make it part of DQL core and not as custom walker.

@dsamblas I could not test it now, because I am not anymore working on that project and to be honest do not have the time at the moment to simulate that case. However, if we take the article you are quoting as basis, it means for me that we could simply write a function which creates the respective DQL statement to achieve the behaviour, isn't it? That means DQL would not need to be touched. I guess the performance would still be bad compared to a native implementation, but it least it is generic. It could still be combined with a walker to be faster if allowed by the underlying database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment