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

Updated parser to support aggegrate functions in null comparisons #864

Closed
wants to merge 1 commit into from
Closed

Conversation

curry684
Copy link

Parser didn't support syntax like "SELECT i FROM entity i JOIN i.second s GROUP BY i HAVING max(s.something) IS NULL" which should be perfectly valid. Changed isAggregateFunction to support parameterless call which makes it consistent with isFunction.

Parser didn't support syntax like "SELECT i FROM entity i JOIN i.second s GROUP BY i HAVING max(s.something) IS NULL" which should be perfectly valid. Changed isAggregateFunction to support parameterless call which makes it consistent with isFunction.
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

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

We use Jira to track the state of pull requests and the versions they got
included in.

{
if(!isset($tokenType))
Copy link
Member

Choose a reason for hiding this comment

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

use null === instead of !isset, and please add the curly braces

@stof
Copy link
Member

stof commented Nov 29, 2013

Please add some tests covering this case

@beberlei
Copy link
Member

beberlei commented Jan 2, 2014

@guilhermeblanco Is this valid?

@curry684 please update the DQL documentation to acommendate the new syntax, if guilherme accepts this change.

@beberlei
Copy link
Member

beberlei commented Jan 2, 2014

@curry684 This needs tests as well, please add them.

@curry684
Copy link
Author

curry684 commented Jan 3, 2014

I needed to fix the bug badly, so I fixed it in our local code and have been running production with it ever since. Whether the fix is entirely valid - no clue, and I honestly don't have time to dive into the project deep enough to go writing docs or tests, I just considered it good courtesy to submit the fix instead of just report a bug when I have fixed it anyway. It really needs someone to review it for coding guidelines anyway.

@beberlei
Copy link
Member

beberlei commented Jan 3, 2014

@curry684 This is not a bug, its a new feature. If you take a look at the documentation DQL EBNF, then you see this is not supported by DQL.

@guilhermeblanco
Copy link
Member

@beberlei support request is valid, but patch doesn't seem entirely correct.
Allowing aggregate functions in null comparisons requires further investigation on all drivers.
Need to check

@guilhermeblanco
Copy link
Member

As of 841bdd5 this is now implemented. Null comparisons with aggregates is part of SQL-92, so all drivers are supported.

@curry684
Copy link
Author

Thank you :)

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.

5 participants