[MLOP-645] Implement query method, cassandra

[AlvaroMarquesAndrade]

Why? 📖

It was necessary to create the query for Cassandra migrations.

What? 🔧

Created the query method for Cassandra migrations.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

How everything was tested? 📏

Unit tests.

Checklist

• My code follows the style guidelines of this project (docstrings, type hinting and linter compliance);
• I have performed a self-review of my own code;
• I have made corresponding changes to the documentation;
• I have added tests that prove my fix is effective or that my feature works;
• New and existing unit tests pass locally with my changes;
• Add labels to distinguish the type of pull request. Available labels are bug, enhancement, feature, and review.

[roelschr]

This method creates a query assuming there was a difference between database and feature set schemas. So it's not its responsibility to check if there was a diff... I think the run method should do that :) THis method should only ensure schema_diff is not None, therefore, it should be a required argument.

[AlvaroMarquesAndrade]

I've added this suggestion, thanks!

[roelschr]

For cassandra we have at least 4 types of alterations allowed:

ALTER TABLE [keyspace_name.] table_name 
[ALTER column_name TYPE cql_type]
[ADD (column_definition_list)]
[DROP column_list | COMPACT STORAGE ]
[RENAME column_name TO column_name]
[WITH table_properties];

why only ADD is being covered here?

[AlvaroMarquesAndrade]

I've added an explanation within the CassandraMigration docstring. I'll probably replicate this same logic to MetastoreMigration.

[roelschr]

This files was not supposed to be in this PR, right?

[AlvaroMarquesAndrade]

That's due to mypy, it'll break if I don't change this file.

[roelschr]

This file was not supposed to be in this file, right?

[AlvaroMarquesAndrade]

Same thing here.

[roelschr]

no_diff should raise an error, because create_query shouldn't be called without a proper difference between database and feature set.

[AlvaroMarquesAndrade]

Since schema_diff is now a required argument, I've removed this test. I can add, however, a test to check the arguments of create_query method. Do you think that this is necessary?

[roelschr]

I guess you don't need to use a warning for that, also. INFO level message will be alright :)

[AlvaroMarquesAndrade]

Nice, thanks for the suggestion

[AlvaroMarquesAndrade]

Since schema_diff is now a required argument, I've removed this check.

[roelschr]

I guess this is not true, since you seem to be using the ALTER command here

[AlvaroMarquesAndrade]

Thanks, I've updated the docstrings from CassandraMigration class and its methods.

[roelschr]

I cannot think of a case where we have create_items AND other kinds of diffs in the same feature set. I understand the Diff.Kind.CREATE as the necessity of a create table, right? Given that, the diff method was made to work only when both schemas exist, replicating the ALTER TABLE options. I would advocate that maybe we could change this logic a bit: the migrate calls for a create query with both schemas. The create query breaks into two sides: if the db_schema is None, it calls for a _create_table_query(feature_set_schema) method. Else, it calls for a _get_diff, then _alter_table_query(diff) for diff in schema_diff. WDYT?

This way, the _get_diff keeps working only when there are two schemas to compare. You won't need to iterate over the whole feature set schema to create another list of diffs classified as Diff.Kind.CREATE and you avoid iterating over schema_diff unnecessarily.

[AlvaroMarquesAndrade]

Thanks for the suggestion, I've applied these changes. I've also proposed a new parameter for the create_query method. Please take a look and we can discuss if there's a better way of dealing with the case when we're writing data to an entity table.

[roelschr]

It's kinda redundant to make a list of Diffs if every single feature column of the feature set schema will be there and the value is just the properties of each feature, also present in the schema. See my comment above.

[roelschr]

Nice, I forgot that adding columns requires its type too!

[roelschr]

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

[acsilveira]

LGTM.