Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query Builder Class - orderBy more possibilities #6716

Open
amigoscs opened this issue Oct 19, 2022 · 10 comments
Open

Query Builder Class - orderBy more possibilities #6716

amigoscs opened this issue Oct 19, 2022 · 10 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@amigoscs
Copy link

I want to execute a query using Query Builder Class:

SELECT * FROM paintings ORDER BY year IS NOT NULL, year ASC;

or

SELECT * FROM paintings ORDER BY year IS NULL, year ASC;

but Query Builder Class does not support this:

$builder->orderBy('year IS NULL, year ASC');

Please add this feature in future releases.

@kenjis kenjis added the enhancement PRs that improve existing functionalities label Oct 19, 2022
@michalsn
Copy link
Member

michalsn commented Oct 20, 2022

Cases like this are already supported:

$builder->orderBy('year IS NULL', '', false)->orderBy('year', 'asc');

@amigoscs
Copy link
Author

Cases like this are already supported:

$builder->orderBy('year IS NULL', '', false)->orderBy('year', 'asc');

Yes, but if $escape = true then an invalid request will be generated:

ORDER BY 'year IS' 'NULL', 'year' ASC

Behavior is ambiguous...

@kenjis kenjis removed the enhancement PRs that improve existing functionalities label Oct 20, 2022
@kenjis
Copy link
Member

kenjis commented Oct 20, 2022

@michalsn Thank you. Exactly!

        $builder = new BaseBuilder('paintings', $this->db);
        $builder->orderBy('year IS NULL', '', false)->orderBy('year', 'asc');
SELECT * FROM "paintings" ORDER BY year IS NULL, "year" ASC

@kenjis
Copy link
Member

kenjis commented Oct 20, 2022

@amigoscs
Copy link
Author

@kenjis

  1. it works
    $builder->orderBy('year IS NULL', '', false)->orderBy('year', 'asc');

  2. this does not work
    $builder->orderBy('year IS NULL')->orderBy('year', 'asc');

  3. this does not work
    $builder->orderBy('year', 'IS NULL')->orderBy('year', 'asc');

  4. this does not work
    $builder->orderBy('year', 'IS NULL', false)->orderBy('year', 'asc');

'IS NULL' is expected to have the same behavior as 'ASC' or 'DESC', but is produced differently.

If fix this line in the system/Database/BaseBuilder.php file:

$qbOrderBy[] = ($direction === '' && preg_match('/\s+(ASC|DESC)$/i', rtrim($field), $match, PREG_OFFSET_CAPTURE))

and

$direction = in_array($direction, ['ASC', 'DESC'], true) ? ' ' . $direction : '';

on those

$qbOrderBy[] = ($direction === '' && preg_match('/\s+(IS NULL|IS NOT NULL|ASC|DESC)$/i', rtrim($field), $match, PREG_OFFSET_CAPTURE))

and

$direction = in_array($direction, ['IS NULL', 'IS NOT NULL', 'ASC', 'DESC'], true) ? ' ' . $direction : '';

then everything starts working.

@michalsn
Copy link
Member

@amigoscs

I don't think allowing the IS NULL and IS NOT NULL as valid parameters would be a good decision. Every DB engine supports the case you're dealing with differently.

If you change your DB engine to SQLSRV, your query will fail.

@amigoscs
Copy link
Author

@michalsn

Well, maybe then add the modified orderBy() method to system\Database\MySQLi\Builder.php

@kenjis
Copy link
Member

kenjis commented Oct 20, 2022

This syntax is not supported by QueryBuilder, because all the supported DBMS do not support it.
IS NULL is not a direction for ORDER BY.
I think 2, 3 and 4 should not work.

@michalsn
Copy link
Member

@amigoscs

In general, we don't accept features that would have support in only one DB engine. And allowing different valid $direction values for each engine doesn't make sense either.

What would make sense is building some kind of abstraction like we did for the RANDOM value, but personally, I don't like this idea at all. Although if you sent a PR with a version that would support all DB engines, we would certainly consider such changes.

For cases like yours, we have a third parameter in orderBy(), and this is a proper way to handle this for now.

@kenjis kenjis added the database Issues or pull requests that affect the database layer label Oct 20, 2022
@sclubricants
Copy link
Member

'IS NULL' is expected to have the same behavior as 'ASC' or 'DESC'

This is wrong. The way it works and is documented is:

ORDER BY [EXPRESSION] [DIRECTION]

I would do it like:

ORDER BY IF(year IS NULL, 0, 1) ASC

This should be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

4 participants