-
Notifications
You must be signed in to change notification settings - Fork 299
[datatype] fix equals method for udt datatypes across schema upgrades#531
[datatype] fix equals method for udt datatypes across schema upgrades #531jhgg wants to merge 1 commit into
Conversation
jhgg
commented
May 27, 2022
Thinking about this more, I think that although this is semantically correct for IsValidDataType, it may not be right for equals - so maybe we might introduce a separate method that just does the check of mutual fields and have is valid data type invoke that instead.
@mpenick
mpenick
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me. It does seem that after creating a type that a type can only be altered in the following ways:
- Rename field:
ALTER TYPE address RENAME zip TO zipcode AND street_name TO street - Add a field:
ALTER TYPE address ADD country text
It'd be nice to have some tests that verify this is a correct assumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have some tests that verify this is a correct assumption.
I'm not entirely sure how we would test that? Are you suggesting we try to test that the database rejects invalid DDL?
There is a third way, you can change the type of a field ALTER field_name TYPE new_cql_datatype - however, I think if you're doing this, you probably need to redeploy your application anyways since the types are changing.
jhgg
commented
May 27, 2022
Thanks for the PR.
What do you think about my second comment? Should I instead refactor this into a "is valid data type for" method, that way equals still remains an equality check?
Additionally, if that's the case, we can drop the name check on the field, this will allow us to handle renaming of a field, and still considering them equal.
Uh oh!
There was an error while loading. Please reload this page.
We encountered a bug with this driver, where an altered UDT would cause
cass_statement_bind_user_type_by_name_nto fail on a prepared statement that was prepared prior to the db altered.I think that this should be a sensible fix, it for sure fixes our issue... however, I have no idea if this is actually sound!
Unfortunately I do not have a C++ reproducer for this bug, since we use the rust wrapper to this driver, that reproducer is here: https://gist.github.com/jhgg/d009d1fb994a0fd550bd01bb803baee5