-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve Spree::Order::NumberGenerator speed #4722
Improve Spree::Order::NumberGenerator speed #4722
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4722 +/- ##
=======================================
Coverage 86.12% 86.12%
=======================================
Files 577 577
Lines 14640 14643 +3
=======================================
+ Hits 12608 12612 +4
+ Misses 2032 2031 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @RyanofWoods! ❤️
When generating a number, if it already exists, the length of numbers will be increased if over half of the possible options are already taken. This query is very expensive when having millions of records. Upon multiple number collisions, this SQL becomes a bottleneck. Co-authored-by: Sung <sung@meundies.com>
758583e
to
4cce19a
Compare
No problem @waiting-for-dev , thanks for MeUndies for discovering this ❤️ I just pushed again as I added some specs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for porting this over @RyanofWoods 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what if I never restart my application?
I don't understand, what's your point? @jarednorman 🤔 |
Summary
When generating a number, if it already exists, the length of numbers will be increased if over half of the possible options are already taken. Calculating the count of orders is very expensive when having millions of records. Upon multiple number collisions, this SQL becomes a bottleneck.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed (
cross them outif they are not):- [ ] I have attached screenshots to demo visual changes.- [ ] I have opened a PR to update the guides.- [ ] I have updated the README to account for my changes.