Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Cassandra mock connection is converting all column names to String, this fix preserves the original type #150

Merged
merged 0 commits into from Nov 27, 2013

Conversation

Projects
None yet
3 participants

No description provided.

@ryanking ryanking and 1 other commented on an outdated diff Jul 11, 2012

lib/cassandra/mock.rb
@@ -449,7 +449,7 @@ def merge_and_sort(old_stuff, new_stuff)
new_stuff = new_stuff.inject({}){|h,k| h[k] = nil; h }
end
- new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0].to_s] = k[1]; h }
+ new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0]] = k[1]; h }
@ryanking

ryanking Jul 11, 2012

Contributor

The prior behavior should match how this library works with a real cassandra cluster. Does it not?

@imkirnos

imkirnos Jul 11, 2012

i'm getting a failure when i use the mock cassandra, which i believe i
being caused by the following:

in get(), extract_and_validate_params_for_real() gets called, which maps
the column name being looked up to the correct type as defined in the
schema, so to_s() is invoked on the right class when doing the hash lookup
by column name.

in insert(), extract_and_validate_params_for_real() does not get called, so
to_s() is invoked on whatever the type is of the column name that the
caller passes in. if it's a (ruby) Integer, say, when the column type in
the schema is LongType, it should be converted to the LongType.to_s
representation before being inserted. instead, it's currently inserted
into the hash as a plain Integer.to_s(), causing get() to break (since it
does attempt to convert the column name to LongType, and a value like
"123456" isn't a valid string representation of a Cassandra::Long)

so this mismatch between get() and insert() (and delete() behaves like
insert() as well i think) is causing errors. i don't know how cassandra
treats types in production, so perhaps my patch resolves things in the
wrong way and i should make insert() and delete() behave like get() in
looking up the type. now that i think about it, that sounds like the right
thing to do.

does my explanation above make sense?

On Wed, Jul 11, 2012 at 11:26 AM, Ryan King <
reply@reply.github.com

wrote:

@@ -449,7 +449,7 @@ def merge_and_sort(old_stuff, new_stuff)
new_stuff = new_stuff.inject({}){|h,k| h[k] = nil; h }
end

  •  new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0].to_s] = k[1];
    
    h }
  •  new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0]] = k[1]; h }
    

The prior behavior should match how this library works with a real
cassandra cluster. Does it not?


Reply to this email directly or view it on GitHub:
https://github.com/twitter/cassandra/pull/150/files#r1143019

-ilya

@imkirnos

imkirnos Jul 11, 2012

here's what i think is a better fix:

https://github.com/cardspring/cassandra/commit/90baff94d2ed032a966d79ebf259b9a68dd3650a

and here's a simple test to reproduce the bug i'm seeing:

require 'cassandra/1.0'
require 'cassandra/mock'
schema_string = %q<
  {
    "api_test": {
        "Long":

{"comparator_type":"org.apache.cassandra.db.marshal.LongType"},
"String":
{"comparator_type":"org.apache.cassandra.db.marshal.StringType"},
"SuperLong": {
"column_type":"Super",
"comparator_type":"org.apache.cassandra.db.marshal.LongType"
}
}
}
>
@Schema = JSON.parse(schema_string)
connection = Cassandra::Mock.new('api_test', @Schema)

connection.insert("Long", "key", 17 => "blah")
connection.get("Long", "key", 17) # blows up

connection.insert("SuperLong", "key", 17 => {'col1' => "blah", 'col2'

=> "blah2"})
connection.get("SuperLong", "key", 17) # blows up

connection.insert("String", "key", "col1" => "blah", "col2" => "blah")
connection.get("String", "key") # ok

On Wed, Jul 11, 2012 at 12:39 PM, Ilya Kirnos imkirnos@gmail.com wrote:

On Wed, Jul 11, 2012 at 12:20 PM, Ilya Kirnos imkirnos@gmail.com wrote:

i'm getting a failure when i use the mock cassandra, which i believe i
being caused by the following:

in get(), extract_and_validate_params_for_real() gets called, which maps
the column name being looked up to the correct type as defined in the
schema, so to_s() is invoked on the right class when doing the hash lookup
by column name.

in insert(), extract_and_validate_params_for_real() does not get called,
so to_s() is invoked on whatever the type is of the column name that the
caller passes in. if it's a (ruby) Integer, say, when the column type in
the schema is LongType, it should be converted to the LongType.to_s
representation before being inserted. instead, it's currently inserted
into the hash as a plain Integer.to_s(), causing get() to break (since it
does attempt to convert the column name to LongType, and a value like
"123456" isn't a valid string representation of a Cassandra::Long)

so this mismatch between get() and insert() (and delete() behaves like
insert() as well i think) is causing errors. i don't

i take that back about remove(). it does map the type, so it appears as
though it's only insert() that does not.

know how cassandra treats types in production, so perhaps my patch
resolves things in the wrong way and i should make insert() and delete()
behave like get() in looking up the type. now that i think about it, that
sounds like the right thing to do.

does my explanation above make sense?

On Wed, Jul 11, 2012 at 11:26 AM, Ryan King <
reply@reply.github.com

wrote:

@@ -449,7 +449,7 @@ def merge_and_sort(old_stuff, new_stuff)
new_stuff = new_stuff.inject({}){|h,k| h[k] = nil; h }
end

  •  new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0].to_s] =
    
    k[1]; h }
  •  new_stuff = new_stuff.to_a.inject({}){|h,k| h[k[0]] = k[1]; h }
    

The prior behavior should match how this library works with a real
cassandra cluster. Does it not?


Reply to this email directly or view it on GitHub:
https://github.com/twitter/cassandra/pull/150/files#r1143019

-ilya

-ilya

-ilya

Contributor

natemueller commented Jul 22, 2012

I think the 90baff9 approach makes sense. If you make a new branch, cherry-pick that in and add a test case I'll merge it in.

@ghost ghost assigned natemueller Jul 22, 2012

@natemueller natemueller merged commit 1355f37 into cassandra-rb:master Nov 27, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment