Support for multiple statements in Cassandra cql migration files #76

aggrhm · 2018-07-17T15:25:59Z

Currently you cannot include multiple statements in the CQL migration files due to Cassandra only allowing 1 statement per query. This makes creating multiple tables or indices in a single migration difficult. This change splits the CQL migration script by a semi-colon and newline pattern, and iterates them 1 by 1, allowing multiple statements per migration.

…d on semi-colon and newline.

coveralls · 2018-07-17T15:34:51Z

Pull Request Test Coverage Report for Build 134

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 49.522%

Totals
Change from base Build 125:	0.0%
Covered Lines:	828
Relevant Lines:	1672

💛 - Coveralls

dhui

Have you read this?

migrate has avoid parsing the migration files b/c it's more complicate than it seems.

Upfront, I see a few issues:

You could break up a valid query that contains a semi-colon. e.g. embedded in a string
Partial failures. How do you handle a query that fails in the middle of a single migration? Is the migration wrapped in a transaction?
Backwards compatibility. This shouldn't break existing migrations and with the first issue I mentioned, it's likely to. Add an option to the db driver to use batches instead of a single query.

Also, I'm not familiar enough w/ Cassandra to know if multiple statements are disallowed in a single query.

dhui · 2018-07-17T23:55:17Z

database/cassandra/cassandra.go

+	for _, q := range(queries) {
+		tq := strings.TrimSpace(q)
+		if (tq == "") { continue }
+		if err := c.session.Query(tq).Exec(); err != nil {


You might be better off using Batch since atomicity is guaranteed. But I'm still not sure about parsing the migration data.

aggrhm · 2018-07-18T02:48:29Z

I didn't see that link, that's probably worth adding to the CONTRIBUTING.md file. Responses to your other issues are below:

Yes I agree this will break a migration with a string with a semi-colon and newline in it, but this is a pretty uncommon migration. In any event, maybe we could add a flag in the connection url to allow these parsed multi-statement migrations (or some sort of header comment so it only applies to particular migration files?).
Yes, this could result in partial changes to the database, but it will just leave the schema in a dirty state, where the user needs to clean it themselves and force the version number. You cannot wrap CREATE TABLE statements in a transaction.
A param or header comment for allowing parsed multi-statements will allow old migrations to continue to work as normal. BATCH statements only support INSERT, UPDATE, and DELETE (https://docs.datastax.com/en/cql/3.3/cql/cql_reference/cqlBatch.html).

I agree with your basic concern, but the issue arises from the gocql library (and somewhat Cassandra itself). Apparently, Cassandra can only support a single statement per "session.Query" call. This makes this library very tedious for Cassandra, in that I need to make a separate migration file per individual schema change.

How do you feel about adding a param to the connection url? Or adding a header comment that denotes this migration should be parsed into multiple statements?

(BTW, this issue was brought up before it was forked here (mattes/migrate#338) and here (apache/cassandra-gocql-driver#1063)

dhui · 2018-07-18T05:40:08Z

Thanks for the context!

I think an option like x-multi-statement (disabled by default) for the Cassandra driver, appropriately documented, would work. e.g. warn about simple split on ; not working w/ embedded semi-colons and atomicity.

Although having headers in the migration file offers more control and granularity, it introduces additional parsing of the migration files which I'd like to avoid to maintain simplicity.

dhui

Add tests

dhui · 2018-07-18T08:41:30Z

database/cassandra/cassandra.go

-		return database.Error{OrigErr: err, Err: "migration failed", Query: migr}
+
+	// split query by semi-colon
+	queries := strings.Split(query, ";\n")


Don't split on \n so we don't need to deal with different line endings

aggrhm · 2018-07-18T18:08:18Z

Sounds good. I have some other work to get done but I'll try to make these changes over the next couple of days.

aggrhm · 2018-07-20T13:31:15Z

Ok I just pushed support for the x-multi-statement flag and updated the README. Also split on ";" instead of ";\n". Ran this on my migrations for my project and it works great.

dhui

Thanks for addressing the feedback! There are only a few more minor issues remaining.

dhui · 2018-07-21T08:13:30Z

database/cassandra/README.md

@@ -3,6 +3,7 @@
 * Drop command will not work on Cassandra 2.X because it rely on
 system_schema table which comes with 3.X
 * Other commands should work properly but are **not tested**
+* The Cassandra driver (gocql) does not natively support executing multipe statements in a single query. To allow for multiple statements in a single migration, you can use the `x-multi-statement` param. Note that this simply splits the migration into separately-executed statements by a semi-colon ';'. The queries are also not executed in any sort of transaction/batch, meaning you are responsible for fixing partial migrations.


Use `;` instead of ';' and provide an explicit warning about semi-colons in strings causing issues.

dhui · 2018-07-21T08:16:49Z

database/cassandra/cassandra.go

+				return database.Error{OrigErr: err, Err: "migration failed", Query: migr}
+			}
+		}
+	} else {


return in the if block above and remove the else to save a level of indentation
https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

aggrhm · 2018-07-24T19:32:26Z

Additional requested changes fixed.

dhui · 2018-07-24T23:02:40Z

Thanks for your PR!

Support for multiple statements in Cassandra cql migration files base…

c4b81d1

…d on semi-colon and newline.

dhui requested changes Jul 18, 2018

View reviewed changes

Added Cassandra URL flag to enable multi statement migrations

327822c

dhui requested changes Jul 21, 2018

View reviewed changes

Add detail to README for Cassandra flag, improve if block

4244ebe

dhui merged commit fad64ed into golang-migrate:master Jul 24, 2018

strangeman mentioned this pull request Aug 3, 2018

Add support for the multiline clickhouse queries #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple statements in Cassandra cql migration files #76

Support for multiple statements in Cassandra cql migration files #76

aggrhm commented Jul 17, 2018

coveralls commented Jul 17, 2018 •

edited

Loading

dhui left a comment

dhui Jul 17, 2018

aggrhm commented Jul 18, 2018

dhui commented Jul 18, 2018

dhui left a comment

dhui Jul 18, 2018

aggrhm commented Jul 18, 2018

aggrhm commented Jul 20, 2018

dhui left a comment

dhui Jul 21, 2018

dhui Jul 21, 2018

aggrhm commented Jul 24, 2018

dhui commented Jul 24, 2018

Support for multiple statements in Cassandra cql migration files #76

Support for multiple statements in Cassandra cql migration files #76

Conversation

aggrhm commented Jul 17, 2018

coveralls commented Jul 17, 2018 • edited Loading

Pull Request Test Coverage Report for Build 134

💛 - Coveralls

dhui left a comment

Choose a reason for hiding this comment

dhui Jul 17, 2018

Choose a reason for hiding this comment

aggrhm commented Jul 18, 2018

dhui commented Jul 18, 2018

dhui left a comment

Choose a reason for hiding this comment

dhui Jul 18, 2018

Choose a reason for hiding this comment

aggrhm commented Jul 18, 2018

aggrhm commented Jul 20, 2018

dhui left a comment

Choose a reason for hiding this comment

dhui Jul 21, 2018

Choose a reason for hiding this comment

dhui Jul 21, 2018

Choose a reason for hiding this comment

aggrhm commented Jul 24, 2018

dhui commented Jul 24, 2018

coveralls commented Jul 17, 2018 •

edited

Loading