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

sp_QuickieStore: Added ability to check for regressions. #486

Merged

Conversation

ReeceGoding
Copy link
Contributor

@ReeceGoding ReeceGoding commented Oct 13, 2024

At long last, closes #448 .

Is there still a ban on updating the documentation? I remember not being told to do that a while ago because Erik had big plans for it. Particularly with this change, the functionality of sp_QuickieStore will be far ahead of the docs. This is too big to go undocumented. It also merits examples in the examples file.

@erikdarlingdata
Copy link
Owner

@ReeceGoding I will review this now that I'm back at home. It generally looks good, I think aside from some formatting oddities, but don't worry about those.

My primary concern is that normal functionality works (well, let's just say "as it worked before") without the new functionality interfering. It looks like there are a couple spots where they interweave a little bit. How much testing have you done with normal operating stuff alongside the new regression stuff?

Also, sort of cool, QuickieStore is now > 10k lines of code, ha ha ha.

@erikdarlingdata erikdarlingdata added enhancement New feature or request sp_QuickieStore For the loving of Query Store labels Oct 15, 2024
@ReeceGoding
Copy link
Contributor Author

ReeceGoding commented Oct 15, 2024

@erikdarlingdata I've done a fair bit of testing with the new regression stuff. It took quite a bit of experimentation to get it to work. I don't have a list of the test cases that I've tried.

As for testing that the old stuff hasn't been broken, I've played around with having both the old and new versions open in different tabs and confirming that they give the same results. As far as I can tell, they do. sp_QuickieStore now has 48 parameters, so you'll have to forgive me if I haven't tried every permutation!

As for the parts where the old functionality overlaps with the new, I've done my best to put that all behind an IF @regression_mode = 1 check. I couldn't always do that, but most of it is protected that way.

EDIT: As is tradition, I found a bug caused by my changes. Thanks for making sure that I checked my work again.

… mode. Addressed ORDER BY formatting oddities.
@erikdarlingdata
Copy link
Owner

erikdarlingdata commented Oct 16, 2024

@ReeceGoding I don't expect you to test all 48 parameters, just perhaps some common ones/ones in the examples file if possible. Anyway, it looks good to me in general (thanks for the second round of changes). I'll let this hang out for about a week to see if you find anything else in there you want to tweak before merging. I'm also holding off on some other changes until this goes in.

To answer your other question, I don't have any sort of embargo on updating the docs or anything. If you're going to add the new parameters in, just a) make sure the help section is updated with them and b) run with help = 1 to get table output. When you have that, use this website: https://ozh.github.io/ascii-tables/ to make the table, but remember to set the output style to GitHub Markdown first.

Thanks!

@ReeceGoding
Copy link
Contributor Author

@erikdarlingdata Thanks Erik. I'll run through the examples file when I get a chance and update both it and the documentation. If it's not this weekend, then it'll be at some point next week. I'm currently low on free time.

@ReeceGoding
Copy link
Contributor Author

@erikdarlingdata I've tested against everything currently in the examples file, except for the 2022-exclusive stuff. Aside from stuff that we expect to change like @debug = 1, all of the outputs from the code given are identical now to what they were previously on the box that I tested on. However, note that not all of the parameters are in the example file.

I still need to add the new parameters to the readme and examples file.

@erikdarlingdata
Copy link
Owner

@ReeceGoding does that mean you're comfortable with merging this one in?

@ReeceGoding
Copy link
Contributor Author

@erikdarlingdata I think I best update the documentation and examples with these new parameters first. I'll probably do that tomorrow.

@ReeceGoding
Copy link
Contributor Author

@erikdarlingdata All done! Feel free to merge.

@erikdarlingdata
Copy link
Owner

@ReeceGoding 🫡

@erikdarlingdata erikdarlingdata merged commit 312defb into erikdarlingdata:dev Oct 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sp_QuickieStore For the loving of Query Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants