Skip to content

DDC 775#288

Merged
beberlei merged 5 commits intodoctrine:masterfrom
FabioBatSilva:DDC-775
Mar 22, 2012
Merged

DDC 775#288
beberlei merged 5 commits intodoctrine:masterfrom
FabioBatSilva:DDC-775

Conversation

@FabioBatSilva
Copy link
Member

Hello

http://www.doctrine-project.org/jira/browse/DDC-775

This patch adds support for arithmetic expression, case expression and functions on DQL ORDER BY clause.

Please let me know if can I make something better :)

Thanks

@stof
Copy link
Member

stof commented Feb 16, 2012

Functions in the ORDER BY clause are not cross-platform. This has already been rejected for this reason.

As of 2.2, you can achieve the result using the HIDDEN keyword in the SELECT clause to exclude it from the result set:

SELECT u, HIDDEN IDENTITY(u.email) as email FROM Doctrine\Tests\Models\CMS\CmsUser u ORDER BY email

@beberlei
Copy link
Member

Any way to automatically move expression in OrderBy into the select clause and use the alias in Order By?

@stof
Copy link
Member

stof commented Feb 19, 2012

@beberlei we discussed it with @guilhermeblanco a while ago on IRC and we both agreed that this would make the code more complex and that it can already be achieved using HIDDEN in 2.2. Our conclusion was that the best solution would be to improve the doc to explain how to achieve it with the current code (but I'm not sure if he worked on the doc after that)

@FabioBatSilva
Copy link
Member Author

Sorry,

I did not attend the previous discussion, how many databases do not support this ?
On Hibernate SQL functions are allowed if they are supported by the underlying database.

I suggest documenting this behavior in databases that do not support functions, instead of limit this feature in all databases.
maybe a flag in DBAL to tell us whether this is supported, otherwise throws an exception.

@guilhermeblanco
Copy link
Member

@FabioBatSilva Are you sure? JPA v2 points at page 106 this:

orderby_clause ::= ORDER BY orderby_item {, orderby_item}* 
orderby_item ::= state_field_path_expression [ASC | DESC]

If Hibernate supports it, it's not documented I think. Official docs doesn't point anything either.

@guilhermeblanco
Copy link
Member

@FabioBatSilva @beberlei @stof @asm89 Ok, here is the deal.
I took some deeper look at the patch, and we may support some of the features you are proposing.

Here is your original proposal:

OrderByItem ::= (
     SimpleArithmeticExpression | SingleValuedPathExpression | CaseExpression |
     ScalarExpression | AggregateExpression | FunctionDeclaration | ResultVariable
) ["ASC" | "DESC"]

Here is my suggestion:

OrderByItem ::= (
     SimpleArithmeticExpression | SingleValuedPathExpression | ScalarExpression | ResultVariable
) ["ASC" | "DESC"]

@FabioBatSilva
Copy link
Member Author

hello @guilhermeblanco,

i'm fine with your suggestion :)

But Hibernate and NHibernate allow functions and aggregate functions in the user documentation.
In both cases there are specifications for the support of the database.
if I remember right EJB does not support this feature.

NHibernate : http://nhforge.org/doc/nh/en/index.html#queryhql-ordering
Hibernate : http://docs.jboss.org/hibernate/orm/3.5/reference/en/html/queryhql.html#queryhql-ordering

Hibernate Docs : SQL functions and aggregate functions are allowed in the having and order by clauses if they are supported by the underlying database (i.e., not in MySQL).

@FabioBatSilva
Copy link
Member Author

Hi guys.
I change the patch to use the proposal from @guilhermeblanco.

Please take a look

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the indentation is wrong here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done !!

beberlei added a commit that referenced this pull request Mar 22, 2012
@beberlei beberlei merged commit 7d7edbb into doctrine:master Mar 22, 2012
@FabioBatSilva FabioBatSilva deleted the DDC-775 branch January 18, 2013 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants