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

NEW DataQuery::exists now generates EXISTS SQL statements #8915

Conversation

ScopeyNZ
Copy link
Contributor

@ScopeyNZ ScopeyNZ commented Apr 10, 2019

This PR adds an exists method to DataQuery that will generate an EXISTS statement from the existing query. This basically looks like this:

SELECT EXISTS(SELECT * FROM MyDataObject WHERE SomeProperty)

This is the preferred way to do these sorts of queries as the engine can just do checks like "does the MyDataObject.SomeProperty index have any entries for true" - this can be a lot faster than counting results.

@ScopeyNZ
Copy link
Contributor Author

I just realised I forgot to ensure tests are passing 🤦‍♂️ . I'll hope travis has good news.

@ScopeyNZ
Copy link
Contributor Author

Ok the remaining failing tests are fixed by @sminnee's recent updates to consistent boolean return types and relies on a fix in 2.x of the postgres module.

Should I update travis config or update my code to be compatible with older versions of postgres?

@robbieaverill
Copy link
Contributor

Should I update travis config or update my code to be compatible with older versions of postgres?

It sounds like that should be done upstream, but you could patch it here too if you want to

src/ORM/DataQuery.php Outdated Show resolved Hide resolved
@kinglozzer
Copy link
Member

Looks like a great addition! If it’s possible to work around the BC issue I’d be happy to take this in

src/ORM/DataQuery.php Outdated Show resolved Hide resolved
src/ORM/DataQuery.php Outdated Show resolved Hide resolved
@ScopeyNZ ScopeyNZ force-pushed the pulls/4.4/helping-performance-exist branch from 3be475e to 81e39aa Compare April 14, 2019 23:17
@ScopeyNZ
Copy link
Contributor Author

Ok I've updated this PR to only contain the EXISTS query changes. I'll raise additional PRs for the threshold fix and the idea about the "prefer conditions" API.

@ScopeyNZ
Copy link
Contributor Author

ScopeyNZ commented Apr 15, 2019

Hmmmn.. Tests...

image

I have no idea why it's failing. The 5.6 build is pretty much the same (runs on postgres) and it's passing...

@ScopeyNZ
Copy link
Contributor Author

I figured out the failing tests: #8934

@ScopeyNZ ScopeyNZ changed the base branch from 4 to 4.4 April 18, 2019 01:35
src/ORM/DataQuery.php Outdated Show resolved Hide resolved
@ScopeyNZ
Copy link
Contributor Author

I added a very simple test. I tried doing some craziness with aggregates and HAVING but the API actually makes it really hard to do as you can't really select the field you're aggregating in your HAVING clause and that's not so easy to with the DataList API.

@emteknetnz
Copy link
Member

This pull request hasn't had any activity for a while. Are you going to be doing further work on it, or would you prefer to close it now?

@robbieaverill
Copy link
Contributor

@emteknetnz this isn't waiting for the author

@sminnee
Copy link
Member

sminnee commented Sep 1, 2020

I resolved the conflict but I'd probably give such an old branch a rebase before merging.

… SQL "exists" query

The exists query in SQL allows the query optimiser (engine specific) to execute these queries much faster - often only needing the presence of an index to return "yes it exists".
@sminnee sminnee force-pushed the pulls/4.4/helping-performance-exist branch from 69f2594 to 3575070 Compare September 1, 2020 04:21
@sminnee
Copy link
Member

sminnee commented Sep 1, 2020

...rebased against 4 and updated the merge target.

@sminnee sminnee changed the base branch from 4.4 to 4 September 1, 2020 04:23
Copy link
Member

@sminnee sminnee left a comment

Choose a reason for hiding this comment

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

If the tests pass I'm happy with this.

src/ORM/DataQuery.php Outdated Show resolved Hide resolved
Co-authored-by: Robbie Averill <robbie@averill.co.nz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants