From f49604bcf8904c8e6382ff6238c6bb0f23620b5c Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Tue, 3 Dec 2024 11:39:31 +0530 Subject: [PATCH 1/9] ELiminating redundant checks --- internal/eval/bytearray.go | 2 +- internal/eval/eval_test.go | 4 ++-- internal/eval/store_eval.go | 33 +++++++++----------------- internal/eval/type_string.go | 46 ++++++++++++++++-------------------- internal/object/deep_copy.go | 2 +- internal/object/object.go | 4 ---- 6 files changed, 36 insertions(+), 55 deletions(-) diff --git a/internal/eval/bytearray.go b/internal/eval/bytearray.go index 45900aa21..dd0d1813d 100644 --- a/internal/eval/bytearray.go +++ b/internal/eval/bytearray.go @@ -36,7 +36,7 @@ func NewByteArrayFromObj(obj *object.Obj) (*ByteArray, error) { } func getValueAsByteSlice(obj *object.Obj) ([]byte, error) { - oType := object.ExtractType(obj) + oType := obj.Type switch oType { case object.ObjTypeInt: return []byte(strconv.FormatInt(obj.Value.(int64), 10)), nil diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go index 61ae149fe..1ecab654c 100644 --- a/internal/eval/eval_test.go +++ b/internal/eval/eval_test.go @@ -6719,7 +6719,7 @@ func testEvalAPPEND(t *testing.T, store *dstore.Store) { migratedOutput: EvalResponse{Result: 3, Error: nil}, validator: func(output []byte) { obj := store.Get("key") - oType := object.ExtractType(obj) + oType := obj.Type if oType != object.ObjTypeInt { t.Errorf("unexpected encoding") } @@ -6774,7 +6774,7 @@ func testEvalAPPEND(t *testing.T, store *dstore.Store) { migratedOutput: EvalResponse{Result: 2, Error: nil}, validator: func(output []byte) { obj := store.Get("key") - oType := object.ExtractType(obj) + oType := obj.Type if oType != object.ObjTypeString { t.Errorf("unexpected encoding") } diff --git a/internal/eval/store_eval.go b/internal/eval/store_eval.go index 37b694f5e..3566a76ff 100644 --- a/internal/eval/store_eval.go +++ b/internal/eval/store_eval.go @@ -338,7 +338,7 @@ func evalGET(args []string, store *dstore.Store) *EvalResponse { } // Decode and return the value based on its encoding - switch oType := object.ExtractType(obj); oType { + switch oType := obj.Type; oType { case object.ObjTypeInt: // Value is stored as an int64, so use type assertion if IsInt64(obj.Value) { @@ -642,7 +642,7 @@ func evalGETRANGE(args []string, store *dstore.Store) *EvalResponse { } var str string - switch oType := object.ExtractType(obj); oType { + switch oType := obj.Type; oType { case object.ObjTypeString: if val, ok := obj.Value.(string); ok { str = val @@ -1095,18 +1095,7 @@ func evalAPPEND(args []string, store *dstore.Store) *EvalResponse { // Key does not exist, create a new key if obj == nil { - // Deduce type and encoding based on the value if no leading zeros - oType := deduceType(value) - - // Transform the value based on the type and encoding - storedValue, err := storeValueWithType(value, oType) - if err != nil { - return &EvalResponse{ - Result: nil, - Error: err, - } - } - + storedValue, oType := getRawStringOrInt(value) store.Put(key, store.NewObj(storedValue, exDurationMs, oType)) return &EvalResponse{ Result: len(value), @@ -1121,7 +1110,7 @@ func evalAPPEND(args []string, store *dstore.Store) *EvalResponse { Error: diceerrors.ErrWrongTypeOperation, } } - oType := object.ExtractType(obj) + oType := obj.Type // Transform the value based on the current encoding currentValue, err := convertValueToString(obj, oType) @@ -4441,7 +4430,7 @@ func evalGETDEL(args []string, store *dstore.Store) *EvalResponse { objVal := store.GetDel(key) // Decode and return the value based on its encoding - switch oType := object.ExtractType(objVal); oType { + switch oType := objVal.Type; oType { case object.ObjTypeInt: // Value is stored as an int64, so use type assertion if IsInt64(objVal.Value) { @@ -5082,7 +5071,7 @@ func evalSETBIT(args []string, store *dstore.Store) *EvalResponse { object.AssertType(obj.Type, object.ObjTypeString) == nil || object.AssertType(obj.Type, object.ObjTypeInt) == nil { var byteArray *ByteArray - oType := object.ExtractType(obj) + oType := obj.Type switch oType { case object.ObjTypeByteArray: @@ -5180,7 +5169,7 @@ func evalGETBIT(args []string, store *dstore.Store) *EvalResponse { } requiredByteArraySize := offset>>3 + 1 - switch oType := object.ExtractType(obj); oType { + switch oType := obj.Type; oType { case object.ObjTypeSet: return &EvalResponse{ Result: nil, @@ -5429,7 +5418,7 @@ func bitfieldEvalGeneric(args []string, store *dstore.Store, isReadOnly bool) *E var value *ByteArray var err error - switch oType := object.ExtractType(obj); oType { + switch oType := obj.Type; oType { case object.ObjTypeByteArray: value = obj.Value.(*ByteArray) case object.ObjTypeString, object.ObjTypeInt: @@ -6383,7 +6372,7 @@ func evalTYPE(args []string, store *dstore.Store) *EvalResponse { } var typeStr string - switch oType := object.ExtractType(obj); oType { + switch oType := obj.Type; oType { case object.ObjTypeString, object.ObjTypeInt, object.ObjTypeByteArray: typeStr = "string" case object.ObjTypeDequeue: @@ -6430,7 +6419,7 @@ func evalTYPE(args []string, store *dstore.Store) *EvalResponse { // var value []byte -// switch oType, _ := object.ExtractType(obj); oType { +// switch oType, _ := obj.Type; oType { // case object.ObjTypeByteArray: // byteArray := obj.Value.(*ByteArray) // byteArrayObject := *byteArray @@ -6491,7 +6480,7 @@ func evalTYPE(args []string, store *dstore.Store) *EvalResponse { // values[i] = make([]byte, 0) // } else { // // handle the case when it is byte array -// switch oType, _ := object.ExtractType(obj); oType { +// switch oType, _ := obj.Type; oType { // case object.ObjTypeByteArray: // byteArray := obj.Value.(*ByteArray) // byteArrayObject := *byteArray diff --git a/internal/eval/type_string.go b/internal/eval/type_string.go index dc4c716be..f320f5510 100644 --- a/internal/eval/type_string.go +++ b/internal/eval/type_string.go @@ -7,8 +7,22 @@ import ( "github.com/dicedb/dice/internal/object" ) -// Similar to -// tryObjectEncoding function in Redis +type String struct { + Value string + Type uint8 +} + +func NewString(value string) *String { + return &String{ + Value: value, + Type: object.ObjTypeString, + } +} + +func (s *String) Serialize() []byte { + return []byte{} +} + func deduceType(v string) (o uint8) { // Check if the value has leading zero if len(v) > 1 && v[0] == '0' { @@ -21,30 +35,12 @@ func deduceType(v string) (o uint8) { return object.ObjTypeString } -// Function to handle converting the value based on the encoding type -func storeValueWithType(value string, oType uint8) (interface{}, error) { - var returnValue interface{} - - // treat as string if value has leading zero - if len(value) > 1 && value[0] == '0' { - // If so, treat as string - return value, nil +func getRawStringOrInt(value string) (interface{}, uint8) { + intValue, err := strconv.ParseInt(value, 10, 64) + if err != nil { // value is not an integer, hence a string + return value, object.ObjTypeString } - - switch oType { - case object.ObjTypeInt: - intValue, err := strconv.ParseInt(value, 10, 64) - if err != nil { - return nil, diceerrors.ErrWrongTypeOperation - } - returnValue = intValue - case object.ObjTypeString: - returnValue = value - default: - return nil, diceerrors.ErrWrongTypeOperation - } - - return returnValue, nil + return intValue, object.ObjTypeInt // value is an integer } // Function to convert the value to a string for concatenation or manipulation diff --git a/internal/object/deep_copy.go b/internal/object/deep_copy.go index 18a10b73d..0e211be9f 100644 --- a/internal/object/deep_copy.go +++ b/internal/object/deep_copy.go @@ -19,7 +19,7 @@ func (obj *Obj) DeepCopy() *Obj { newObj.Value = copier.DeepCopy() } else { // Handle types that are not DeepCopyable - sourceType := ExtractType(obj) + sourceType := obj.Type switch sourceType { case ObjTypeString: sourceValue := obj.Value.(string) diff --git a/internal/object/object.go b/internal/object/object.go index 63b6b0c30..630230980 100644 --- a/internal/object/object.go +++ b/internal/object/object.go @@ -82,7 +82,3 @@ var ObjTypeSortedSet uint8 = 8 var ObjTypeCountMinSketch uint8 = 9 var ObjTypeBF uint8 = 10 var ObjTypeDequeue uint8 = 11 - -func ExtractType(obj *Obj) (e1 uint8) { - return obj.Type -} From 45fb9864749ea2216c25f7ad7b30d191fd83be64 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Tue, 3 Dec 2024 11:45:56 +0530 Subject: [PATCH 2/9] Creating a type for ObjectType instead of vanilla uint8 --- internal/eval/bytearray.go | 2 +- internal/eval/eval.go | 2 +- internal/eval/store_eval.go | 6 +++--- internal/eval/type_string.go | 20 ++++++-------------- internal/object/object.go | 30 +++++++++++++++++++----------- internal/object/typeencoding.go | 4 ++-- internal/store/store.go | 2 +- 7 files changed, 33 insertions(+), 33 deletions(-) diff --git a/internal/eval/bytearray.go b/internal/eval/bytearray.go index dd0d1813d..4bf4f1f0a 100644 --- a/internal/eval/bytearray.go +++ b/internal/eval/bytearray.go @@ -81,7 +81,7 @@ func getByteArrayValueAsByteSlice(obj *object.Obj) ([]byte, error) { } // ByteSliceToObj converts a byte slice to an Obj of the specified type and encoding -func ByteSliceToObj(store *dstore.Store, oldObj *object.Obj, b []byte, objType uint8) (*object.Obj, error) { +func ByteSliceToObj(store *dstore.Store, oldObj *object.Obj, b []byte, objType object.ObjectType) (*object.Obj, error) { switch objType { case object.ObjTypeInt: return ByteSliceToIntObj(store, oldObj, b) diff --git a/internal/eval/eval.go b/internal/eval/eval.go index 7c1173b1f..95f708c96 100644 --- a/internal/eval/eval.go +++ b/internal/eval/eval.go @@ -165,7 +165,7 @@ func evalMSET(args []string, store *dstore.Store) []byte { insertMap := make(map[string]*object.Obj, len(args)/2) for i := 0; i < len(args); i += 2 { key, value := args[i], args[i+1] - oType := deduceType(value) + _, oType := getRawStringOrInt(value) var storedValue interface{} switch oType { case object.ObjTypeInt: diff --git a/internal/eval/store_eval.go b/internal/eval/store_eval.go index 3566a76ff..4fe6f04cd 100644 --- a/internal/eval/store_eval.go +++ b/internal/eval/store_eval.go @@ -201,7 +201,7 @@ func evalSET(args []string, store *dstore.Store) *EvalResponse { var oldVal *interface{} key, value = args[0], args[1] - oType := deduceType(value) + _, oType := getRawStringOrInt(value) for i := 2; i < len(args); i++ { arg := strings.ToUpper(args[i]) @@ -2477,7 +2477,7 @@ func incrByFloatCmd(args []string, incr float64, store *dstore.Store) *EvalRespo if obj == nil { strValue := formatFloat(incr, false) - oType := deduceType(strValue) + _, oType := getRawStringOrInt(strValue) obj = store.NewObj(strValue, -1, oType) store.Put(key, obj) return &EvalResponse{ @@ -2511,7 +2511,7 @@ func incrByFloatCmd(args []string, incr float64, store *dstore.Store) *EvalRespo } strValue := formatFloat(value, true) - oType := deduceType(strValue) + _, oType := getRawStringOrInt(strValue) // Remove the trailing decimal for integer values // to maintain consistency with redis diff --git a/internal/eval/type_string.go b/internal/eval/type_string.go index f320f5510..3553a00f5 100644 --- a/internal/eval/type_string.go +++ b/internal/eval/type_string.go @@ -9,7 +9,7 @@ import ( type String struct { Value string - Type uint8 + Type object.ObjectType } func NewString(value string) *String { @@ -23,28 +23,20 @@ func (s *String) Serialize() []byte { return []byte{} } -func deduceType(v string) (o uint8) { - // Check if the value has leading zero +func getRawStringOrInt(v string) (interface{}, object.ObjectType) { if len(v) > 1 && v[0] == '0' { // If so, treat as string - return object.ObjTypeString + return v, object.ObjTypeString } - if _, err := strconv.ParseInt(v, 10, 64); err == nil { - return object.ObjTypeInt - } - return object.ObjTypeString -} - -func getRawStringOrInt(value string) (interface{}, uint8) { - intValue, err := strconv.ParseInt(value, 10, 64) + intValue, err := strconv.ParseInt(v, 10, 64) if err != nil { // value is not an integer, hence a string - return value, object.ObjTypeString + return v, object.ObjTypeString } return intValue, object.ObjTypeInt // value is an integer } // Function to convert the value to a string for concatenation or manipulation -func convertValueToString(obj *object.Obj, oType uint8) (string, error) { +func convertValueToString(obj *object.Obj, oType object.ObjectType) (string, error) { var currentValueStr string switch oType { diff --git a/internal/object/object.go b/internal/object/object.go index 630230980..24c8527a5 100644 --- a/internal/object/object.go +++ b/internal/object/object.go @@ -25,7 +25,7 @@ package object // objects (e.g., strings, numbers, complex data structures like lists or maps). type Obj struct { // Type holds the type of the object (e.g., string, int, complex structure) - Type uint8 + Type ObjectType // LastAccessedAt stores the last access timestamp of the object. // It helps track when the object was last accessed and may be used for cache eviction or freshness tracking. @@ -72,13 +72,21 @@ type InternalObj struct { ExDuration int64 } -var ObjTypeString uint8 = 0 -var ObjTypeJSON uint8 = 3 -var ObjTypeByteArray uint8 = 4 -var ObjTypeInt uint8 = 5 -var ObjTypeSet uint8 = 6 -var ObjTypeHashMap uint8 = 7 -var ObjTypeSortedSet uint8 = 8 -var ObjTypeCountMinSketch uint8 = 9 -var ObjTypeBF uint8 = 10 -var ObjTypeDequeue uint8 = 11 +// ObjectType represents the type of a DiceDB object +type ObjectType uint8 + +// Define object types as constants +const ( + ObjTypeString ObjectType = iota + _ // skip 1 and 2 to maintain compatibility + _ + ObjTypeJSON + ObjTypeByteArray + ObjTypeInt + ObjTypeSet + ObjTypeHashMap + ObjTypeSortedSet + ObjTypeCountMinSketch + ObjTypeBF + ObjTypeDequeue +) diff --git a/internal/object/typeencoding.go b/internal/object/typeencoding.go index b8f677066..8934fe710 100644 --- a/internal/object/typeencoding.go +++ b/internal/object/typeencoding.go @@ -6,14 +6,14 @@ import ( diceerrors "github.com/dicedb/dice/internal/errors" ) -func AssertTypeWithError(te, t uint8) error { +func AssertTypeWithError(te, t ObjectType) error { if te != t { return errors.New("WRONGTYPE Operation against a key holding the wrong kind of value") } return nil } -func AssertType(_type, expectedType uint8) []byte { +func AssertType(_type, expectedType ObjectType) []byte { if err := AssertTypeWithError(_type, expectedType); err != nil { return diceerrors.NewErrWithMessage(diceerrors.WrongKeyTypeErr) } diff --git a/internal/store/store.go b/internal/store/store.go index 5c1f46297..6bb18aecb 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -84,7 +84,7 @@ func ResetStore(store *Store) *Store { return store } -func (store *Store) NewObj(value interface{}, expDurationMs int64, oType uint8) *object.Obj { +func (store *Store) NewObj(value interface{}, expDurationMs int64, oType object.ObjectType) *object.Obj { obj := &object.Obj{ Value: value, Type: oType, From 6d4a7568a5f43284f9e703f92ee1a595abe789da Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Tue, 3 Dec 2024 11:48:29 +0530 Subject: [PATCH 3/9] Removing the additional check --- internal/eval/eval.go | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/internal/eval/eval.go b/internal/eval/eval.go index 95f708c96..0b28e079d 100644 --- a/internal/eval/eval.go +++ b/internal/eval/eval.go @@ -165,16 +165,7 @@ func evalMSET(args []string, store *dstore.Store) []byte { insertMap := make(map[string]*object.Obj, len(args)/2) for i := 0; i < len(args); i += 2 { key, value := args[i], args[i+1] - _, oType := getRawStringOrInt(value) - var storedValue interface{} - switch oType { - case object.ObjTypeInt: - storedValue, _ = strconv.ParseInt(value, 10, 64) - case object.ObjTypeString: - storedValue = value - default: - return clientio.Encode(fmt.Errorf("ERR unsupported type: %d", oType), false) - } + storedValue, oType := getRawStringOrInt(value) insertMap[key] = store.NewObj(storedValue, exDurationMs, oType) } From 1355e7c3346fbe4b9e2d38492d8b657a11c8e516 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Tue, 3 Dec 2024 15:27:12 +0530 Subject: [PATCH 4/9] Bloomfilter naming conventions --- internal/eval/bloom_test.go | 6 +-- internal/eval/store_eval.go | 35 +++++++------- internal/eval/{bloom.go => type_bloom.go} | 56 +++++++++++------------ internal/eval/type_string.go | 6 +-- 4 files changed, 51 insertions(+), 52 deletions(-) rename internal/eval/{bloom.go => type_bloom.go} (87%) diff --git a/internal/eval/bloom_test.go b/internal/eval/bloom_test.go index 3d06c3daa..5ef929250 100644 --- a/internal/eval/bloom_test.go +++ b/internal/eval/bloom_test.go @@ -100,19 +100,19 @@ func TestGetOrCreateBloomFilter(t *testing.T) { opts := defaultBloomOpts() // Should create a new filter under the key `key`. - bloom, err := getOrCreateBloomFilter(key, store, opts) + bloom, err := GetOrCreateBloomFilter(key, store, opts) if bloom == nil || err != nil { t.Errorf("nil bloom or non-nil error returned while creating new filter - key: %s, opts: %+v, err: %v", key, opts, err) } // Should get the filter (which was created above) - bloom, err = getOrCreateBloomFilter(key, store, opts) + bloom, err = GetOrCreateBloomFilter(key, store, opts) if bloom == nil || err != nil { t.Errorf("nil bloom or non-nil error returned while fetching existing filter - key: %s, opts: %+v, err: %v", key, opts, err) } // Should get the filter with nil opts - bloom, err = getOrCreateBloomFilter(key, store, nil) + bloom, err = GetOrCreateBloomFilter(key, store, nil) if bloom == nil || err != nil { t.Errorf("nil bloom or non-nil error returned while fetching existing filter - key: %s, opts: %+v, err: %v", key, opts, err) } diff --git a/internal/eval/store_eval.go b/internal/eval/store_eval.go index 4fe6f04cd..343a386e4 100644 --- a/internal/eval/store_eval.go +++ b/internal/eval/store_eval.go @@ -2867,11 +2867,17 @@ func evalBFRESERVE(args []string, store *dstore.Store) *EvalResponse { return makeEvalError(err) } - _, err = CreateBloomFilter(args[0], store, opts) - if err != nil { + key := args[0] + + _, err = GetBloomFilter(key, store) + if err != nil && err != diceerrors.ErrKeyNotFound { // bloom filter does not exist return makeEvalError(err) + } else if err != nil && err == diceerrors.ErrKeyNotFound { // key does not exists + CreateOrReplaceBloomFilter(key, opts, store) + return makeEvalResult(clientio.OK) + } else { // bloom filter already exists + return makeEvalResult(clientio.OK) } - return makeEvalResult(clientio.OK) } // evalBFADD evaluates the BF.ADD command responsible for adding an element to a bloom filter. If the filter does not @@ -2881,12 +2887,12 @@ func evalBFADD(args []string, store *dstore.Store) *EvalResponse { return makeEvalError(diceerrors.ErrWrongArgumentCount("BF.ADD")) } - bloom, err := getOrCreateBloomFilter(args[0], store, nil) + bf, err := GetOrCreateBloomFilter(args[0], store, nil) if err != nil { return makeEvalError(err) } - result, err := bloom.add(args[1]) + result, err := bf.add(args[1]) if err != nil { return makeEvalError(err) } @@ -2901,14 +2907,14 @@ func evalBFEXISTS(args []string, store *dstore.Store) *EvalResponse { return makeEvalError(diceerrors.ErrWrongArgumentCount("BF.EXISTS")) } - bloom, err := GetBloomFilter(args[0], store) - if err != nil { + bf, err := GetBloomFilter(args[0], store) + if err != nil && err != diceerrors.ErrKeyNotFound { return makeEvalError(err) - } - if bloom == nil { + } else if err != nil && err == diceerrors.ErrKeyNotFound { return makeEvalResult(clientio.IntegerZero) } - result, err := bloom.exists(args[1]) + + result, err := bf.exists(args[1]) if err != nil { return makeEvalError(err) } @@ -2922,25 +2928,20 @@ func evalBFINFO(args []string, store *dstore.Store) *EvalResponse { return makeEvalError(diceerrors.ErrWrongArgumentCount("BF.INFO")) } - bloom, err := GetBloomFilter(args[0], store) - + bf, err := GetBloomFilter(args[0], store) if err != nil { return makeEvalError(err) } - if bloom == nil { - return makeEvalError(diceerrors.ErrGeneral("not found")) - } opt := "" if len(args) == 2 { opt = args[1] } - result, err := bloom.info(opt) + result, err := bf.info(opt) if err != nil { return makeEvalError(err) } - return makeEvalResult(result) } diff --git a/internal/eval/bloom.go b/internal/eval/type_bloom.go similarity index 87% rename from internal/eval/bloom.go rename to internal/eval/type_bloom.go index 35b893705..3d7a6072c 100644 --- a/internal/eval/bloom.go +++ b/internal/eval/type_bloom.go @@ -92,7 +92,7 @@ func newBloomOpts(args []string) (*BloomOpts, error) { // newBloomFilter creates and returns a new filter. It is responsible for initializing the // underlying bit array. -func newBloomFilter(opts *BloomOpts) *Bloom { +func NewBloom(opts *BloomOpts) *Bloom { // Calculate bits per element // bpe = -log(errorRate)/ln(2)^2 num := -1 * math.Log(opts.errorRate) @@ -278,25 +278,41 @@ func (opts *BloomOpts) updateIndexes(value string) error { return nil } -// getOrCreateBloomFilter attempts to fetch an existing bloom filter from -// the kv store. If it does not exist, it tries to create one with -// given `opts` and returns it. -func getOrCreateBloomFilter(key string, store *dstore.Store, opts *BloomOpts) (*Bloom, error) { +// CreateOrReplaceBloomFilter creates a new bloom filter with given `opts` +// and stores it in the kv store. If the bloom filter already exists, it +// replaces the existing one. If `opts` is nil, it uses the default options. +func CreateOrReplaceBloomFilter(key string, opts *BloomOpts, store *dstore.Store) *Bloom { + if opts == nil { + opts = defaultBloomOpts() + } + bf := NewBloom(opts) + obj := store.NewObj(bf, -1, object.ObjTypeBF) + store.Put(key, obj) + return bf +} + +// GetOrCreateBloomFilter fetches an existing bloom filter from +// the kv store and returns the datastructure instance of it. +// If it does not exist, it tries to create one with given `opts` and returns it. +// Note: It also stores it in the kv store. +func GetOrCreateBloomFilter(key string, store *dstore.Store, opts *BloomOpts) (*Bloom, error) { bf, err := GetBloomFilter(key, store) - if err != nil { + if err != nil && err != diceerrors.ErrKeyNotFound { return nil, err + } else if err != nil && err == diceerrors.ErrKeyNotFound { + bf = CreateOrReplaceBloomFilter(key, opts, store) } - if bf == nil { - bf, err = CreateBloomFilter(key, store, opts) - } - return bf, err + return bf, nil } -// get the bloom filter +// GetBloomFilter fetches an existing bloom filter from +// the kv store and returns the datastructure instance of it. +// The function also returns diceerrors.ErrKeyNotFound if the key does not exist. +// It also returns diceerrors.ErrWrongTypeOperation if the object is not a bloom filter. func GetBloomFilter(key string, store *dstore.Store) (*Bloom, error) { obj := store.Get(key) if obj == nil { - return nil, nil + return nil, diceerrors.ErrKeyNotFound } if err := object.AssertType(obj.Type, object.ObjTypeBF); err != nil { return nil, diceerrors.ErrWrongTypeOperation @@ -304,19 +320,3 @@ func GetBloomFilter(key string, store *dstore.Store) (*Bloom, error) { return obj.Value.(*Bloom), nil } - -func CreateBloomFilter(key string, store *dstore.Store, opts *BloomOpts) (*Bloom, error) { - bf, err := GetBloomFilter(key, store) - if bf != nil { - return nil, diceerrors.ErrGeneral("item exists") - } - if err != nil { - return nil, err - } - if opts == nil { - opts = defaultBloomOpts() - } - obj := store.NewObj(newBloomFilter(opts), -1, object.ObjTypeBF) - store.Put(key, obj) - return obj.Value.(*Bloom), nil -} diff --git a/internal/eval/type_string.go b/internal/eval/type_string.go index 3553a00f5..8ab3f9938 100644 --- a/internal/eval/type_string.go +++ b/internal/eval/type_string.go @@ -8,14 +8,12 @@ import ( ) type String struct { - Value string - Type object.ObjectType + value string } func NewString(value string) *String { return &String{ - Value: value, - Type: object.ObjTypeString, + value: value, } } From f505f7dd7cd34649b11f23942ac066676ff33a74 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Tue, 3 Dec 2024 15:38:55 +0530 Subject: [PATCH 5/9] Unit test fixes. Strangely mem utilization reduced. --- internal/eval/bloom_test.go | 2 +- internal/eval/eval_test.go | 6 +++--- internal/eval/type_string_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/eval/bloom_test.go b/internal/eval/bloom_test.go index 5ef929250..000499bd6 100644 --- a/internal/eval/bloom_test.go +++ b/internal/eval/bloom_test.go @@ -122,7 +122,7 @@ func TestUpdateIndexes(t *testing.T) { // Create a value, default opts and initialize all params of the filter value := "hello" opts := defaultBloomOpts() - bloom := newBloomFilter(opts) + bloom := NewBloom(opts) err := opts.updateIndexes(value) if err != nil { diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go index 1ecab654c..5ba8b3f63 100644 --- a/internal/eval/eval_test.go +++ b/internal/eval/eval_test.go @@ -2995,7 +2995,7 @@ func testEvalPFADD(t *testing.T, store *dstore.Store) { name: "PFADD Incorrect type provided", setup: func() { key, value := "EXISTING_KEY", "VALUE" - oType := deduceType(value) + _, oType := getRawStringOrInt(value) var exDurationMs int64 = -1 keepttl := false @@ -4830,7 +4830,7 @@ func testEvalDebug(t *testing.T, store *dstore.Store) { store.Put(key, obj) }, input: []string{"MEMORY", "EXISTING_KEY"}, - migratedOutput: EvalResponse{Result: 89, Error: nil}, + migratedOutput: EvalResponse{Result: 72, Error: nil}, }, "root path": { @@ -4843,7 +4843,7 @@ func testEvalDebug(t *testing.T, store *dstore.Store) { store.Put(key, obj) }, input: []string{"MEMORY", "EXISTING_KEY", "$"}, - migratedOutput: EvalResponse{Result: 89, Error: nil}, + migratedOutput: EvalResponse{Result: 72, Error: nil}, }, "invalid path": { diff --git a/internal/eval/type_string_test.go b/internal/eval/type_string_test.go index 0fc2bf33a..57230ff89 100644 --- a/internal/eval/type_string_test.go +++ b/internal/eval/type_string_test.go @@ -13,7 +13,7 @@ func TestDeduceType(t *testing.T) { tests := []struct { name string input string - wantType uint8 + wantType object.ObjectType wantEnc uint8 }{ { @@ -45,7 +45,7 @@ func TestDeduceType(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotType := deduceType(tt.input) + _, gotType := getRawStringOrInt(tt.input) if gotType != tt.wantType { t.Errorf("deduceType(%q) = (%v), want (%v)", tt.input, gotType, tt.wantType) } From 531a32a0cdb391b044059ccb1a73c96f19e67395 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Wed, 4 Dec 2024 16:50:30 +0530 Subject: [PATCH 6/9] Some errors as constants and bloom -> bloomfilter --- integration_tests/commands/http/bloom_test.go | 7 ++++--- integration_tests/commands/http/jsondebug_test.go | 2 +- integration_tests/commands/resp/bloom_test.go | 7 ++++--- integration_tests/commands/websocket/bloom_test.go | 7 ++++--- integration_tests/commands/websocket/jsondebug_test.go | 2 +- internal/errors/migrated_errors.go | 1 + internal/eval/bloom_test.go | 2 +- internal/eval/eval_test.go | 2 +- internal/eval/store_eval.go | 6 ++++-- internal/eval/{type_bloom.go => type_bloomfilter.go} | 4 ++-- 10 files changed, 23 insertions(+), 17 deletions(-) rename internal/eval/{type_bloom.go => type_bloomfilter.go} (99%) diff --git a/integration_tests/commands/http/bloom_test.go b/integration_tests/commands/http/bloom_test.go index d6c47e41b..40c1de24a 100644 --- a/integration_tests/commands/http/bloom_test.go +++ b/integration_tests/commands/http/bloom_test.go @@ -3,6 +3,7 @@ package http import ( "testing" + diceerrors "github.com/dicedb/dice/internal/errors" "github.com/stretchr/testify/assert" ) @@ -82,7 +83,7 @@ func TestBloomFilter(t *testing.T) { Body: map[string]interface{}{"key": "bf", "values": []interface{}{0.01, 2000}}, }, }, - expected: []interface{}{"OK", "ERR item exists"}, + expected: []interface{}{"OK", diceerrors.ErrKeyExists}, }, } @@ -199,7 +200,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { Body: map[string]interface{}{"key": "bf"}, }, }, - expected: []interface{}{"ERR not found"}, + expected: []interface{}{diceerrors.ErrKeyNotFound}, }, { name: "BF.RESERVE with a very high error rate", @@ -281,7 +282,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { Body: map[string]interface{}{"key": "bf", "values": []interface{}{0.01, 2000}}, }, }, - expected: []interface{}{"OK", "ERR item exists"}, + expected: []interface{}{"OK", diceerrors.ErrKeyExists}, }, { name: "BF.INFO after multiple additions", diff --git a/integration_tests/commands/http/jsondebug_test.go b/integration_tests/commands/http/jsondebug_test.go index 26db95c84..37ab3a061 100644 --- a/integration_tests/commands/http/jsondebug_test.go +++ b/integration_tests/commands/http/jsondebug_test.go @@ -16,7 +16,7 @@ func TestJSONDEBUG(t *testing.T) { {Command: "JSON.SET", Body: map[string]interface{}{"key": "k1", "path": "$", "json": map[string]interface{}{"a": 1}}}, {Command: "JSON.DEBUG", Body: map[string]interface{}{"values": []interface{}{"MEMORY", "k1"}}}, }, - expected: []interface{}{"OK", float64(89)}, + expected: []interface{}{"OK", float64(72)}, }, { name: "jsondebug with a valid path", diff --git a/integration_tests/commands/resp/bloom_test.go b/integration_tests/commands/resp/bloom_test.go index 53ba8b506..e01c0b091 100644 --- a/integration_tests/commands/resp/bloom_test.go +++ b/integration_tests/commands/resp/bloom_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + diceerrors "github.com/dicedb/dice/internal/errors" "github.com/stretchr/testify/assert" ) @@ -42,7 +43,7 @@ func TestBFReserveAddInfoExists(t *testing.T) { { name: "BF.RESERVE on existent filter returns error", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 1000"}, - expect: []interface{}{"OK", "ERR item exists"}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists}, delays: []time.Duration{0, 0}, cleanUp: []string{"DEL bf"}, }, @@ -135,7 +136,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.INFO on a non-existent filter", cmds: []string{"BF.INFO bf"}, - expect: []interface{}{"ERR not found"}, + expect: []interface{}{diceerrors.ErrKeyNotFound}, delays: []time.Duration{0}, cleanUp: []string{"del bf"}, }, @@ -170,7 +171,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.RESERVE with duplicate filter name", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 2000"}, - expect: []interface{}{"OK", "ERR item exists"}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists}, delays: []time.Duration{0, 0}, cleanUp: []string{"del bf"}, }, diff --git a/integration_tests/commands/websocket/bloom_test.go b/integration_tests/commands/websocket/bloom_test.go index 7198505df..ba250fa95 100644 --- a/integration_tests/commands/websocket/bloom_test.go +++ b/integration_tests/commands/websocket/bloom_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + diceerrors "github.com/dicedb/dice/internal/errors" "github.com/stretchr/testify/assert" ) @@ -37,7 +38,7 @@ func TestBFReserveAddInfoExists(t *testing.T) { { name: "BF.RESERVE on existent filter returns error", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 1000"}, - expect: []interface{}{"OK", "ERR item exists"}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists}, cleanUp: []string{"DEL bf"}, }, } @@ -126,7 +127,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.INFO on a non-existent filter", cmds: []string{"BF.INFO bf"}, - expect: []interface{}{"ERR not found"}, + expect: []interface{}{diceerrors.ErrKeyNotFound}, delays: []time.Duration{0}, cleanUp: []string{"del bf"}, }, @@ -161,7 +162,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.RESERVE with duplicate filter name", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 2000"}, - expect: []interface{}{"OK", "ERR item exists"}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists}, delays: []time.Duration{0, 0}, cleanUp: []string{"del bf"}, }, diff --git a/integration_tests/commands/websocket/jsondebug_test.go b/integration_tests/commands/websocket/jsondebug_test.go index 8f2cba904..77a805f69 100644 --- a/integration_tests/commands/websocket/jsondebug_test.go +++ b/integration_tests/commands/websocket/jsondebug_test.go @@ -30,7 +30,7 @@ func TestJSONDEBUG(t *testing.T) { `JSON.SET k1 $ {"a":1}`, "JSON.DEBUG MEMORY k1", }, - expected: []interface{}{"OK", float64(89)}, + expected: []interface{}{"OK", float64(72)}, }, { name: "jsondebug with a valid path", diff --git a/internal/errors/migrated_errors.go b/internal/errors/migrated_errors.go index 6c8fe4e4c..ea81b0c4e 100644 --- a/internal/errors/migrated_errors.go +++ b/internal/errors/migrated_errors.go @@ -33,6 +33,7 @@ var ( ErrInvalidIPAddress = errors.New("invalid IP address") ErrInvalidFingerprint = errors.New("invalid fingerprint") ErrKeyDoesNotExist = errors.New("ERR could not perform this operation on a key that doesn't exist") + ErrKeyExists = errors.New("ERR key exists") // Error generation functions for specific error messages with dynamic parameters. ErrWrongArgumentCount = func(command string) error { diff --git a/internal/eval/bloom_test.go b/internal/eval/bloom_test.go index 000499bd6..d88d97f2f 100644 --- a/internal/eval/bloom_test.go +++ b/internal/eval/bloom_test.go @@ -122,7 +122,7 @@ func TestUpdateIndexes(t *testing.T) { // Create a value, default opts and initialize all params of the filter value := "hello" opts := defaultBloomOpts() - bloom := NewBloom(opts) + bloom := NewBloomFilter(opts) err := opts.updateIndexes(value) if err != nil { diff --git a/internal/eval/eval_test.go b/internal/eval/eval_test.go index 5ba8b3f63..22089ddf6 100644 --- a/internal/eval/eval_test.go +++ b/internal/eval/eval_test.go @@ -9028,7 +9028,7 @@ func testEvalBFINFO(t *testing.T, store *dstore.Store) { { name: "BF.INFO on non-existent filter", input: []string{"nonExistentFilter"}, - migratedOutput: EvalResponse{Result: nil, Error: errors.New("ERR not found")}, + migratedOutput: EvalResponse{Result: nil, Error: diceerrors.ErrKeyNotFound}, }, } diff --git a/internal/eval/store_eval.go b/internal/eval/store_eval.go index 343a386e4..c58a63008 100644 --- a/internal/eval/store_eval.go +++ b/internal/eval/store_eval.go @@ -2869,13 +2869,15 @@ func evalBFRESERVE(args []string, store *dstore.Store) *EvalResponse { key := args[0] - _, err = GetBloomFilter(key, store) + bf, err := GetBloomFilter(key, store) if err != nil && err != diceerrors.ErrKeyNotFound { // bloom filter does not exist return makeEvalError(err) } else if err != nil && err == diceerrors.ErrKeyNotFound { // key does not exists CreateOrReplaceBloomFilter(key, opts, store) return makeEvalResult(clientio.OK) - } else { // bloom filter already exists + } else if bf != nil { // bloom filter already exists + return makeEvalError(diceerrors.ErrKeyExists) + } else { return makeEvalResult(clientio.OK) } } diff --git a/internal/eval/type_bloom.go b/internal/eval/type_bloomfilter.go similarity index 99% rename from internal/eval/type_bloom.go rename to internal/eval/type_bloomfilter.go index 3d7a6072c..816524cad 100644 --- a/internal/eval/type_bloom.go +++ b/internal/eval/type_bloomfilter.go @@ -92,7 +92,7 @@ func newBloomOpts(args []string) (*BloomOpts, error) { // newBloomFilter creates and returns a new filter. It is responsible for initializing the // underlying bit array. -func NewBloom(opts *BloomOpts) *Bloom { +func NewBloomFilter(opts *BloomOpts) *Bloom { // Calculate bits per element // bpe = -log(errorRate)/ln(2)^2 num := -1 * math.Log(opts.errorRate) @@ -285,7 +285,7 @@ func CreateOrReplaceBloomFilter(key string, opts *BloomOpts, store *dstore.Store if opts == nil { opts = defaultBloomOpts() } - bf := NewBloom(opts) + bf := NewBloomFilter(opts) obj := store.NewObj(bf, -1, object.ObjTypeBF) store.Put(key, obj) return bf From 32e86d00eb557d2b21e360d6167d3da8f2ca605c Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Wed, 4 Dec 2024 17:01:56 +0530 Subject: [PATCH 7/9] Integration tests fixes --- integration_tests/commands/http/bloom_test.go | 6 +++--- integration_tests/commands/resp/bloom_test.go | 6 +++--- integration_tests/commands/resp/jsondebug_test.go | 2 +- integration_tests/commands/websocket/bloom_test.go | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/integration_tests/commands/http/bloom_test.go b/integration_tests/commands/http/bloom_test.go index 40c1de24a..8d28958b9 100644 --- a/integration_tests/commands/http/bloom_test.go +++ b/integration_tests/commands/http/bloom_test.go @@ -83,7 +83,7 @@ func TestBloomFilter(t *testing.T) { Body: map[string]interface{}{"key": "bf", "values": []interface{}{0.01, 2000}}, }, }, - expected: []interface{}{"OK", diceerrors.ErrKeyExists}, + expected: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, }, } @@ -200,7 +200,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { Body: map[string]interface{}{"key": "bf"}, }, }, - expected: []interface{}{diceerrors.ErrKeyNotFound}, + expected: []interface{}{diceerrors.ErrKeyNotFound.Error()}, }, { name: "BF.RESERVE with a very high error rate", @@ -282,7 +282,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { Body: map[string]interface{}{"key": "bf", "values": []interface{}{0.01, 2000}}, }, }, - expected: []interface{}{"OK", diceerrors.ErrKeyExists}, + expected: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, }, { name: "BF.INFO after multiple additions", diff --git a/integration_tests/commands/resp/bloom_test.go b/integration_tests/commands/resp/bloom_test.go index e01c0b091..5af5b3044 100644 --- a/integration_tests/commands/resp/bloom_test.go +++ b/integration_tests/commands/resp/bloom_test.go @@ -43,7 +43,7 @@ func TestBFReserveAddInfoExists(t *testing.T) { { name: "BF.RESERVE on existent filter returns error", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 1000"}, - expect: []interface{}{"OK", diceerrors.ErrKeyExists}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, delays: []time.Duration{0, 0}, cleanUp: []string{"DEL bf"}, }, @@ -136,7 +136,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.INFO on a non-existent filter", cmds: []string{"BF.INFO bf"}, - expect: []interface{}{diceerrors.ErrKeyNotFound}, + expect: []interface{}{diceerrors.ErrKeyNotFound.Error()}, delays: []time.Duration{0}, cleanUp: []string{"del bf"}, }, @@ -171,7 +171,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.RESERVE with duplicate filter name", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 2000"}, - expect: []interface{}{"OK", diceerrors.ErrKeyExists}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, delays: []time.Duration{0, 0}, cleanUp: []string{"del bf"}, }, diff --git a/integration_tests/commands/resp/jsondebug_test.go b/integration_tests/commands/resp/jsondebug_test.go index af1bda920..8ad10a9ae 100644 --- a/integration_tests/commands/resp/jsondebug_test.go +++ b/integration_tests/commands/resp/jsondebug_test.go @@ -23,7 +23,7 @@ func TestJSONDEBUG(t *testing.T) { `JSON.SET k1 $ {"a":1}`, "JSON.DEBUG MEMORY k1", }, - expected: []interface{}{"OK", int64(89)}, + expected: []interface{}{"OK", int64(72)}, }, { name: "jsondebug with a valid path", diff --git a/integration_tests/commands/websocket/bloom_test.go b/integration_tests/commands/websocket/bloom_test.go index ba250fa95..d8c9222ca 100644 --- a/integration_tests/commands/websocket/bloom_test.go +++ b/integration_tests/commands/websocket/bloom_test.go @@ -38,7 +38,7 @@ func TestBFReserveAddInfoExists(t *testing.T) { { name: "BF.RESERVE on existent filter returns error", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 1000"}, - expect: []interface{}{"OK", diceerrors.ErrKeyExists}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, cleanUp: []string{"DEL bf"}, }, } @@ -127,7 +127,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.INFO on a non-existent filter", cmds: []string{"BF.INFO bf"}, - expect: []interface{}{diceerrors.ErrKeyNotFound}, + expect: []interface{}{diceerrors.ErrKeyNotFound.Error()}, delays: []time.Duration{0}, cleanUp: []string{"del bf"}, }, @@ -162,7 +162,7 @@ func TestBFEdgeCasesAndErrors(t *testing.T) { { name: "BF.RESERVE with duplicate filter name", cmds: []string{"BF.RESERVE bf 0.01 1000", "BF.RESERVE bf 0.01 2000"}, - expect: []interface{}{"OK", diceerrors.ErrKeyExists}, + expect: []interface{}{"OK", diceerrors.ErrKeyExists.Error()}, delays: []time.Duration{0, 0}, cleanUp: []string{"del bf"}, }, From fdbb9794ca03532ace83b62e47a9e51c6bd22576 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Wed, 4 Dec 2024 17:05:54 +0530 Subject: [PATCH 8/9] Object Type changed and casted during RDB serialization --- internal/eval/dump_restore.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/eval/dump_restore.go b/internal/eval/dump_restore.go index 5c1b39953..7049cdd2b 100644 --- a/internal/eval/dump_restore.go +++ b/internal/eval/dump_restore.go @@ -24,10 +24,12 @@ func rdbDeserialize(data []byte) (*object.Obj, error) { if err != nil { return nil, err } - objType, err := buf.ReadByte() + _oType, err := buf.ReadByte() if err != nil { return nil, err } + + objType := object.ObjectType(_oType) switch objType { case object.ObjTypeString: value, err = readString(buf) @@ -112,7 +114,7 @@ func readSet(buf *bytes.Reader) (interface{}, error) { func rdbSerialize(obj *object.Obj) ([]byte, error) { var buf bytes.Buffer buf.WriteByte(0x09) - buf.WriteByte(obj.Type) + buf.WriteByte(byte(obj.Type)) switch obj.Type { case object.ObjTypeString: str, ok := obj.Value.(string) From 0d736f63bbcd0f61115ba7f96218eb70c1b25638 Mon Sep 17 00:00:00 2001 From: Arpit Bhayani Date: Wed, 4 Dec 2024 17:09:10 +0530 Subject: [PATCH 9/9] Lint fixes --- internal/eval/store_eval.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/eval/store_eval.go b/internal/eval/store_eval.go index f59e8bb83..c80ad9d63 100644 --- a/internal/eval/store_eval.go +++ b/internal/eval/store_eval.go @@ -2877,9 +2877,8 @@ func evalBFRESERVE(args []string, store *dstore.Store) *EvalResponse { return makeEvalResult(clientio.OK) } else if bf != nil { // bloom filter already exists return makeEvalError(diceerrors.ErrKeyExists) - } else { - return makeEvalResult(clientio.OK) } + return makeEvalResult(clientio.OK) } // evalBFADD evaluates the BF.ADD command responsible for adding an element to a bloom filter. If the filter does not