-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix #1442: add codecs for UUID type(s) #1496
Conversation
@@ -48,6 +48,10 @@ public abstract class JSONCodecRegistries { | |||
JSONCodecs.DURATION_FROM_STRING, | |||
JSONCodecs.TIME_FROM_STRING, | |||
JSONCodecs.TIMESTAMP_FROM_STRING, | |||
// UUID codecs | |||
JSONCodecs.UUID_FROM_STRING, | |||
JSONCodecs.TIMEUUID_FROM_STRING, |
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.
Although we won't allow creation of "timeuuid" type, we do allow read/write, for which codec needed
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.
Sorry, I may miss the context for this one.
Why do we not allow "timeuuid" type?
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.
I am not 100% sure; I think it may be considered confusing thing -- you can store time-based UUIDs in "uuid" column anyway. And it is "Cassandraism" so hiding it may improve user experience.
Still: we can add support easily in future, if we so decide, but right now spreadsheet includes type as unsupported.
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.
LGTM, one comment.
What this PR does:
Adds codec impls, tests (unit, integration) for cql "uuid" and "timeuuid" types.
Which issue(s) this PR fixes:
Fixes #1442
Checklist