-
Notifications
You must be signed in to change notification settings - Fork 205
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
Basic UDT implementation #406
base: master
Are you sure you want to change the base?
Conversation
This is awesome. I haven't had a chance to look at the code yet but I'm stoked to have this functionality. |
I should be able to review this week. |
2a521d7
to
959616f
Compare
# -*- encoding : utf-8 -*- | ||
require File.expand_path('../spec_helper', __FILE__) | ||
|
||
describe Cequel::Record do |
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 wasn't too sure what to put here since I didn't really define a class to represent a UDT since Cassandra::UDT
handles it pretty well
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 is basically fine, but can you add a description suffix like describe Cequel::Record, "UDT support" do
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 is looking good. i do have some minor concerns. my biggest is that i think we need a better way to define types. my second concern is that is don't love using Hash
as the class for all UDTs.
regarding defining types, what if we added them to keyspace schema? that would allow the schema sync to create them. this might allow us to address my second concern too. consider an approach like
class Person do
include Cequel::UDT
field :name, :text
field :age, :int
end
this could then be wired into the existing type casting system, https://github.com/cequel/cequel/blob/master/lib/cequel/type.rb, so that we use the same code for UDTs as other types.
module_eval <<-RUBY, __FILE__, __LINE__+1 | ||
def #{name} | ||
value = read_attribute(#{name.inspect}) | ||
(value.is_a? Cassandra::UDT) ? value.to_h.symbolize_keys : value |
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.
maybe value.to_h.symbolize_keys!
to reduce the GC load?
# @api private | ||
# | ||
def cast(value) | ||
Cassandra::UDT.new(value.to_h) |
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.
do we really need to convert it to a hash? what if we just passed value
to #new
?
class UdtColumn < DataColumn | ||
# | ||
# @param value the value to cast | ||
# @return the value cast as a hash |
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 seems this code returns a Cassandra::UDT
not a hash.
# -*- encoding : utf-8 -*- | ||
require File.expand_path('../spec_helper', __FILE__) | ||
|
||
describe Cequel::Record do |
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 is basically fine, but can you add a description suffix like describe Cequel::Record, "UDT support" do
|
||
describe Cequel::Record do | ||
model :Post do | ||
connection.execute("CREATE TYPE cequel_test.person (name text, last_name text, age int)") |
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.
could we have better syntax for type creation?
Alright, I'm going to try and get this done during the first two weeks of November |
Can this feature please get implemented? Its a really important feature of Cassandra thats unavailable in Cequel.
|
@devnut, you are welcome to take this PR over and complete it if you'd like |
Summary
I created a basic implementation for handling UDTs. It feels a bit off, so feel welcome to suggest changes that might be better. I wouldn't really say I understand Cequel's structure 100% since I haven't read through the entire codebase but I'm definitely ready to learn!
These changes work with Cassandra 3.6 upwards as seen in the Cassandra changelog.
I also bumped the
cassandra-driver
version but that commit can be removed.On to the meat of this PR. Let's say we have this UDT:
You can create an object with a UDT column and set it with a hash, like so:
To update it, we have to overwrite the entire hash, as I couldn't get atomic modification working yet.
Design Choices
I decided to go for the syntax
udt :name, :type
rather than something likecolumn :name, :udt, type: :type
to stay true to the way collection columns are defined, and to dissuade users from wanting to use UDTs as key columns, which I don't really think is a good idea. I'm aware UDTs are not collection columns but you get the idea. It's not a scalar column.Issues
#263
Pending for other PRs
rake cequel:migrate
worksfrozen
types so we can have UDTs for earlier versions