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

FIX Calculate threshold condition with SQL rather than PHP #8921

Conversation

ScopeyNZ
Copy link
Contributor

@ScopeyNZ ScopeyNZ commented Apr 15, 2019

This is a performance fix. Modern SQL engines can avoid counting a whole result set (potentially thousands of records) when you are only interested if the count exceeds a threshold.

Split from #8915. This is now targeting 4.3 as a patch. Happy to retarget any other branch if requested 🙂

I am setting this query to not be DISTINCT which I think is an okay assumption. Feel free to close this PR if I'm wrong.

@ScopeyNZ ScopeyNZ force-pushed the pulls/4.3/threshold-count-in-sql branch from 81d3cc7 to 3b1c774 Compare April 15, 2019 04:48
    This is a performance fix. Modern SQL engines can avoid counting a whole result set (potentially thousands of records) when you are only interested if the count exceeds a threshold.
@ScopeyNZ ScopeyNZ force-pushed the pulls/4.3/threshold-count-in-sql branch from 3b1c774 to a48beac Compare April 15, 2019 04:48
static::$foreignListCache[$hasOneClass] = [
'count' => $list->count(),
'overThreshold' => $overThreshold,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible albeit incredibly unlikely that this is a breaking change if someone is relying on the existing count cache in a subclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we'd consider that part of our API to be honest

@ScopeyNZ ScopeyNZ changed the base branch from 4 to 4.3 April 16, 2019 00:08
Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

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

The changes look good to me in theory. The fact that the tests pass is a good sign as well.

static::$foreignListCache[$hasOneClass] = [
'count' => $list->count(),
'overThreshold' => $overThreshold,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we'd consider that part of our API to be honest

@robbieaverill robbieaverill merged commit 523456a into silverstripe:4.3 Apr 20, 2019
@robbieaverill robbieaverill deleted the pulls/4.3/threshold-count-in-sql branch April 20, 2019 13:25
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.

2 participants