Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for nil partition key for secondary indexes in memory connector #414

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add support for nil partition key for secondary indexes in memory connector #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8929e62
Add support for nil partition key for secondary indexes in memory
aniketpant Dec 14, 2019
2ea86ba
Remove unused fmt import
aniketpant Dec 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions connectors/memory/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ func partitionKeyBuilder(pk *dosa.PrimaryKey, values map[string]dosa.FieldValue)
var encodedKey []byte
for _, k := range pk.PartitionKeys {
if v, ok := values[k]; ok {
if isNilInterface(v) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just v == nil?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue
}
encodedVal, _ := encoder.Encode(v)
encodedKey = append(encodedKey, encodedVal...)
} else {
Expand Down Expand Up @@ -714,6 +717,28 @@ func getStartingPoint(ei *dosa.EntityInfo, token string) (start string, startPar
return start, startPartKey, nil
}

func isNilInterface(v dosa.FieldValue) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNilInterface could use a unit test

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Will add one.

switch v := v.(type) {
case *dosa.UUID:
return v == nil
case *string:
return v == nil
case *int64:
return v == nil
case *int32:
return v == nil
case *float64:
return v == nil
case *[]byte:
return v == nil
case *time.Time:
return v == nil
case *bool:
return v == nil
}
return false
}

// CheckSchema is just a stub; there is no schema management for the in memory connector
// since creating a new one leaves you with no data!
func (c *Connector) CheckSchema(ctx context.Context, scope, namePrefix string, ed []*dosa.EntityDefinition) (int32, error) {
Expand Down
1 change: 0 additions & 1 deletion names_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
package dosa

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand Down
17 changes: 17 additions & 0 deletions testclient/testclient_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,23 @@ func TestTestClient(t *testing.T) {
err = client.Upsert(context.Background(), nil, &testEnt)
assert.NoError(t, err)

uuidv := dosa.NewUUID()
op := dosa.NewRangeOp(&testentity.TestEntity{}).
Eq("UUIDVP", dosa.UUID(uuidv)).
Limit(1)
results, _, err := client.Range(context.Background(), op)
assert.Empty(t, results)
assert.NoError(t, err)

testEnt.UUIDVP = &uuidv
err = client.Upsert(context.Background(), nil, &testEnt)
assert.NoError(t, err)

results, _, err = client.Range(context.Background(), op)
assert.Len(t, results, 1)
assert.Equal(t, &testEnt, results[0].(*testentity.TestEntity))
assert.NoError(t, err)

readEnt := testentity.TestEntity{
UUIDKey: uuid,
StrKey: "key",
Expand Down
32 changes: 20 additions & 12 deletions testentity/testentity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,26 @@ import (

// TestEntity uses common key types and all types in value fields.
type TestEntity struct {
dosa.Entity `dosa:"name=awesome_test_entity, primaryKey=(UUIDKey, StrKey ASC, Int64Key DESC)"`
UUIDKey dosa.UUID `dosa:"name=an_uuid_key"`
StrKey string
Int64Key int64
UUIDV dosa.UUID
StrV string
Int64V int64 `dosa:"name=an_int64_value"`
Int32V int32
DoubleV float64
BoolV bool
BlobV []byte
TSV time.Time
dosa.Entity `dosa:"name=awesome_test_entity, primaryKey=(UUIDKey, StrKey ASC, Int64Key DESC)"`
EntityByUUIDVP dosa.Index `dosa:"key=(UUIDVP)"`
EntityByStrVP dosa.Index `dosa:"key=(StrVP)"`
EntityByInt64VP dosa.Index `dosa:"key=(Int64VP)"`
EntityByInt32VP dosa.Index `dosa:"key=(Int32VP)"`
EntityByDoubleVP dosa.Index `dosa:"key=(DoubleVP)"`
EntityByBoolVP dosa.Index `dosa:"key=(BoolVP)"`
EntityByTSVP dosa.Index `dosa:"key=(TSVP)"`

UUIDKey dosa.UUID `dosa:"name=an_uuid_key"`
StrKey string
Int64Key int64
UUIDV dosa.UUID
StrV string
Int64V int64 `dosa:"name=an_int64_value"`
Int32V int32
DoubleV float64
BoolV bool
BlobV []byte
TSV time.Time

UUIDVP *dosa.UUID
StrVP *string
Expand Down