Add support for nil partition key for secondary indexes in memory connector#414
Add support for nil partition key for secondary indexes in memory connector#414aniketpant wants to merge 2 commits intouber-go:masterfrom
Conversation
93e4312 to
425d52a
Compare
connector Why: Cassandra supports secondary indexes where the partition key can be nil. This results in the index not being generated until the value is set. The test client which uses a memory connector should support the same behavior. - This change addresses the need by: Add a `nil` check in `partitionKeyBuilder` which is capable of handling different value types. In these cases, we simply skip over the value instead of encoding a `nil` value which results in a panic inside the gob encoder. -
425d52a to
8929e62
Compare
| var encodedKey []byte | ||
| for _, k := range pk.PartitionKeys { | ||
| if v, ok := values[k]; ok { | ||
| if isNilInterface(v) { |
There was a problem hiding this comment.
I tired doing that at first but seems like it's one of those interface issues where an interface holding a nil value will be non-nil. An alternative to isNilInterface would be using reflection.
func isNilValue(i interface{}) bool {
return i == nil || reflect.ValueOf(i).IsNil()
}
Example: https://play.golang.org/p/j9SqfEgGe4B
Ref: https://tour.golang.org/methods/12
Note that an interface value that holds a nil concrete value is itself non-nil.
There was a problem hiding this comment.
@phliar Do you know if there's an alternative here? See my comment on the issue I'm experiencing and why we need to cast to the interface.
There was a problem hiding this comment.
Your proposed PR is a more efficient way of doing it than adding a reflect call. A golang interface contains both a Type and a Value, and you only want to see if the Value is nil.
At this point in the code, the Type will never be nil, and so checking if the interface is nil will always be false.
There's a longer discussion of this here: https://groups.google.com/forum/#!topic/golang-nuts/wnH302gBa4I/discussion
There was a problem hiding this comment.
Yeah, this totally makes sense now.
I wanted to avoid using reflect here primarily because we are always going to deal with known types here.
| return start, startPartKey, nil | ||
| } | ||
|
|
||
| func isNilInterface(v dosa.FieldValue) bool { |
There was a problem hiding this comment.
isNilInterface could use a unit test
|
|
Why:
Cassandra supports secondary indexes where the partition key can be
nil. This results in the index not being generated until the value is
set. The test client which uses a memory connector should support the
same behavior.
This change addresses the need by:
Add a
nilcheck inpartitionKeyBuilderwhich is capable of handlingdifferent value types. In these cases, we simply skip over the value
instead of encoding a
nilvalue which results in a panic inside thegob encoder.