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

Feat: Support for "Update" type queries in Templated Benchmarks #386

Merged
merged 16 commits into from
Dec 6, 2023

Conversation

ETHenzlere
Copy link
Contributor

@ETHenzlere ETHenzlere commented Oct 31, 2023

Feature adds a check for UPDATE style queries and handles the execution in templated benchmarks

Copy link
Contributor

@anjagruenheid anjagruenheid left a comment

Choose a reason for hiding this comment

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

LGTM, make it public and ask Brian for a review.

@ETHenzlere ETHenzlere changed the title Feat: Support for "Update" queries in Templated Benchmarks Feat: Support for "Update" type queries in Templated Benchmarks Nov 8, 2023
@ETHenzlere ETHenzlere marked this pull request as ready for review November 8, 2023 10:58
@ETHenzlere
Copy link
Contributor Author

@bpkroth could you give me a review for this PR?

// False for UPDATE, INSERT, DELETE queries
boolean hasResultSet = stmt.execute();

while (true) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is kind of a weird double loop.

There should never be any more results for INSERT/UPDATE/DELETE, correct?
So, you really don't need to outer loop with a break. Just the if hasResultSet and then loop inside that?

Copy link
Collaborator

@bpkroth bpkroth Dec 5, 2023

Choose a reason for hiding this comment

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

Maybe just add an assert inside the INSERT/UPDATE/DELETE branch that it doesn't have more results and remove the outer loop.

Copy link
Contributor Author

@ETHenzlere ETHenzlere Dec 5, 2023

Choose a reason for hiding this comment

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

The way I understand it, we can theoretically have a statement with multiple queries (select * from x; select * from y) which would return multiple result sets. And they could theoretically also get mixed with "update" type queries.
The question is whether we want to only support this for "select" type queries, all possible combinations or we do not want to support this at all

I have currently adapted such that only a combination of "select" type queries would be supported

Copy link
Collaborator

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

Fixed some small whitespace issues. LGTM now. Thanks for the contribution!

@bpkroth bpkroth enabled auto-merge (squash) December 6, 2023 16:39
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.

3 participants