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

Added GROUP BY support, tests to dolt query_diff #768

Merged
merged 5 commits into from Jun 27, 2020

Conversation

andy-wm-arthur
Copy link
Contributor

No description provided.

Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

My one main comment is that you shouldn't need to duplicate test setup from the enginetest package. Just use it directly (unless there is some very compelling reason not to that I'm missing).

query string
}

var engineTestSelectQueries = []engineTestQuery{
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to just get these directly from the enginetest package. They're exported. Just use the queries and ignore the expected values

Copy link
Member

Choose a reason for hiding this comment

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

And if you're concerned that not every test query is supported (those involving typestable e.g.), just use doltHarness.SkipQueryTest to filter them.

{query: `SELECT SUM(i) FROM mytable`},
}

// runs a subset of SELECT queries from enginetest/queries.go as a sanity check
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to run all of them? If you're just checking that they run and produce no diff, then that should be fine, right?

Another interesting test would be to create the table, commit it, then insert the rows and verify that the "to" diff contains exactly the rows expected by the normal query in the engine test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a great idea

@@ -262,3 +362,506 @@ func testQueryDiffer(t *testing.T, test queryDifferTest) {
assert.Nil(t, to)
assert.Equal(t, io.EOF, err)
}

// subset of test data from go-mysql-server/enginetest/testdata.go:CreateSubsetTestData()
var engineTestSetup = []testCommand{
Copy link
Member

Choose a reason for hiding this comment

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

Probably should just create these with a doltHarness. Not sure what value you get by duplicating them here.

Copy link
Member

Choose a reason for hiding this comment

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

That method even lets you create a subset of the tables you are interested in (but creating all of them should work)

@andy-wm-arthur andy-wm-arthur force-pushed the andy/query-diff-4 branch 2 times, most recently from 032df45 to a381e98 Compare June 26, 2020 22:24
@andy-wm-arthur andy-wm-arthur merged commit 647744d into master Jun 27, 2020
@andy-wm-arthur andy-wm-arthur deleted the andy/query-diff-4 branch June 27, 2020 00:31
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.

None yet

2 participants