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

Adding Missing Test cases and test case refactoring #144

Merged
merged 17 commits into from
Apr 12, 2022

Conversation

timveil
Copy link
Collaborator

@timveil timveil commented Mar 30, 2022

The test package was missing a number of benchmark tests. This PR attempts to resolve all of that by adding any missing benchmark, loader or worker tests. To simplify the creation of these tests, some refactoring was done to the base "abstract" test classes. 2 tests, chbenchmark and twitter, have been temporarily @Ignored as the changes required to make them work have already been addressed in PR #134 which.

Of course when adding the missing test cases, some issues were identified in either the benchmarks themselves (usually having to do with behavior at very low SF, or with the HSQLDB dialect files). These issues were resolved as part of this PR.

Furthermore, I changed the way HSQLDB is initialized so that tests can be paused and the database can be accessed/queried. This is extremely useful for troubleshooting.

@timveil timveil added the in-progress This PR is in progress. label Mar 30, 2022
@timveil timveil requested review from apavlo and lmwnshn and removed request for apavlo and lmwnshn March 30, 2022 23:26
@timveil
Copy link
Collaborator Author

timveil commented Mar 30, 2022

i need to do one more pass over these changes at a very low scale factor .001 as now the full suite of unit tests at SF .01 takes nearly 20 minutes (most being TPCH). i'll ping ya'll for a review once i've finished any changes.

@lmwnshn
Copy link
Collaborator

lmwnshn commented Mar 31, 2022

Please ping me on Slack (otherwise I might miss it). Thanks!

@timveil timveil added ready-for-review This PR is ready to be reviewed. and removed in-progress This PR is in progress. labels Mar 31, 2022
@timveil timveil requested review from apavlo and lmwnshn March 31, 2022 14:03
@timveil timveil marked this pull request as ready for review March 31, 2022 14:06
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

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

I disagree with your change to NoOp.

@@ -38,7 +38,7 @@

// The query only contains a semi-colon
// That is enough for the DBMS to have to parse it and do something
public final SQLStmt noopStmt = new SQLStmt(";");
public final SQLStmt noopStmt = new SQLStmt("select 1");
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same as a NOOP since it requires evaluation. Is there a reason for this change?

Maybe we should rename it to the "DoNothing" benchmark and then have separate SelectOne and SemiColon procedures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NoOp was one of the missing test cases; unfortunately HSQLDB doesn't accept ; as a valid sql statement. my thinking was this was a reasonable fix but now that i'm looking at it i should have just done this with a hsqldb dialect file and not changed the java class. is that a better approach?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this has been resolved by reverting the change to NoOp.java and instead adding a dialect-hsqldb.xml file with a hsqldb friendly query


import java.util.List;

@Ignore("the testcase is under development")
Copy link
Member

Choose a reason for hiding this comment

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

Is this still true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as soon as i can submit my config-refactor pr, this is greatly simplified. I marked this as ignore because at the moment it only loads the schema for one of the benchmarks instead of both. fixing this was a lot of the work that i'd already done in config-refactor. my thinking is to ignore this test until the subsequent pr is merged and then it can be reenabled.

timveil added 3 commits March 31, 2022 10:56
…e but possible port conflicts; also waiting during stop incase there is a problem brining hsqldb down
@timveil timveil requested a review from apavlo April 1, 2022 14:50
Copy link
Member

@apavlo apavlo left a comment

Choose a reason for hiding this comment

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

LGTM. Please resolve conflicts

@timveil timveil merged commit 611b167 into cmu-db:main Apr 12, 2022
@timveil timveil deleted the test-refactor branch April 12, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review This PR is ready to be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants