ronantakizawa/codereview-bench huggingface dataset:
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:

	for i := 2; i < len(args); i++ {
		switch args[i] {
		case "EX", "ex":
			i++
			if i == len(args) {
				return Encode(errors.New("ERR syntax error"), false)
			}

			exDurationSec, err := strconv.ParseInt(args[i], 10, 64)
			if err != nil {
				return Encode(errors.New("ERR value is not an integer or out of range"), false)
			}
			exDurationMs = exDurationSec * 1000
		case "XX", "xx":
			// Get the key from the hash table
			obj := Get(key)

			// if key does not exist, return RESP encoded nil
			if obj == nil {
				return RESP_NIL
			}

		default:
			return Encode(errors.New("ERR syntax error"), false)
		}
	}

	// putting the k and value in a Hash Table
	Put(key, NewObj(value, exDurationMs, oType, oEnc))
	return RESP_OK
}

// evalGET returns the value for the queried key in args
// The key should be the only param in args
// The RESP value of the key is encoded and then returned
// evalGET returns RESP_NIL if key is expired or it does not exist
func evalGET(args []string) []byte {
	if len(args) != 1 {
		return Encode(errors.New("ERR wrong number of arguments for 'get' command"), false)
	}

	var key string = args[0]

	// Get the key from the hash table
	obj := Get(key)

	// if key does not exist, return RESP encoded nil
	if obj == nil {
		return RESP_NIL
	}

Review comment (suggestion):
Not a requirement for this PR, but it may be a good time to start consolidating/reusing the error messages for easier maintainability and ensuring that the error messages are exactly as that of REDIS.

@arpitbbhayani @JyotinderSingh if you guys feel there is a need i can open a PR.

Code after:
	key, value = args[0], args[1]
	oType, oEnc := deduceTypeEncoding(value)

	for i := 2; i < len(args); i++ {
		switch args[i] {
		case "EX", "ex":
			i++
			if i == len(args) {
				return Encode(errors.New("ERR syntax error"), false)
			}

			exDurationSec, err := strconv.ParseInt(args[i], 10, 64)
			if err != nil {
				return Encode(errors.New("ERR value is not an integer or out of range"), false)
			}
			exDurationMs = exDurationSec * 1000

		case "PXAT", "pxat":
			i++
			if i == len(args) {
				return Encode(errors.New("ERR syntax error"), false)
			}

			exDurationUnixMs, err := strconv.ParseInt(args[i], 10, 64)
			if err != nil {
				return Encode(errors.New("ERR value is not an integer or out of range"), false)
			}

			if exDurationUnixMs < 0 {
				return Encode(errors.New("ERR invalid expire time in 'set' command"), false)
			}

			exDurationMs = exDurationUnixMs - time.Now().UnixMilli()
			// If the expiry time is in the past, set exDurationMs to 0
			// This will be used to signal immediate expiration
			if exDurationMs < 0 {
				exDurationMs = 0
			}

		case "XX", "xx":
			// Get the key from the hash table
			obj := Get(key)

			// if key does not exist, return RESP encoded nil
			if obj == nil {
				return RESP_NIL
			}
		default:
			return Encode(errors.New("ERR syntax error"), false)
		}
	}
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:
		lastIdx = len(args) + keySpecs.LastKey
	}
	for i := keySpecs.BeginIndex; i <= lastIdx; i += step {
		keys = append(keys, args[i])
	}

	return Encode(keys, false)
}
func evalRename(args []string) []byte {

	if len(args) != 2 {
		return Encode(errors.New("ERR wrong number of arguments for 'RENAME' command"), false)
	}
	sourceKey := args[0]
	destKey := args[1]

	//if Source and Destination Keys are same return RESP encoded ok
	if sourceKey == destKey {
		return RESP_OK
	}

	sourceObj := Get(sourceKey)

	// if Source key does not exist, return RESP encoded nil
	if sourceObj == nil {
		return Encode("(error) ERR no such key", false)
	}

	destObj := Get(destKey)

	//Delete the destination key if exists
	if destObj != nil {
		Del(destKey)
	}

	// Rename the key by putting the source key's value in the destination key
	Put(destKey, sourceObj)

	//Deleting the source Key
	Del(sourceKey)

	return RESP_OK

}

func executeCommand(cmd *RedisCmd, c *Client) []byte {
	diceCmd, ok := diceCmds[cmd.Cmd]
	if !ok {
		return Encode(fmt.Errorf("ERR unknown command '%s', with args beginning with: %s", cmd.Cmd, strings.Join(cmd.Args, " ")), false)
	}


Review comment (suggestion):
```suggestion
		return Encode("ERR no such key", false)
```

Code after:
			}
		}

		// initialize result with byteArray
		operationResult := NewByteArray(len(result))
		operationResult.data = result
		operationResult.Length = int64(len(result))

		// resize the byte array if necessary
		operationResult.ResizeIfNecessary()

		// create object related to result
		operationResultObject := NewObj(operationResult, -1, OBJ_TYPE_BYTEARRAY, OBJ_ENCODING_BYTEARRAY)

		// store the result in destKey
		Put(destKey, operationResultObject)

		return Encode(len(result), true)
	}
}

// evalCommand evaluates COMMAND <subcommand> command based on subcommand
// COUNT: return total count of commands in Dice.
func evalCommand(args []string) []byte {
	if len(args) == 0 {
		return Encode(errors.New("(error) ERR wrong number of arguments for 'command' command"), false)
	}
	subcommand := strings.ToUpper(args[0])
	switch subcommand {
	case "COUNT":
		return evalCommandCount(nil)
	case "GETKEYS":
		return evalCommandGetKeys(args[1:])
	default:
		return Encode(fmt.Errorf("ERR unknown subcommand '%s'. Try COMMAND HELP", subcommand), false)
	}
}

// evalKeys returns the list of keys that match the pattern
// The pattern should be the only param in args
func evalKeys(args []string) []byte {
	if len(args) != 1 {
		return Encode(errors.New("ERR wrong number of arguments for 'keys' command"), false)
	}

	pattern := args[0]
	keys, err := Keys(pattern)
	if err != nil {
		return Encode(err, false)
	}
Repository: DiceDB/dice
File: core/store.go (Go)

Code before:
		return nil
	}

	return obj
}

func GetDel(k string) *Obj {
	storeMutex.Lock()
	defer storeMutex.Unlock()
	keypoolMutex.Lock()
	defer keypoolMutex.Unlock()

	ptr, ok := keypool[k]
	if !ok {
		return nil
	}

	obj, exists := store[ptr]
	if !exists {
		return nil
	}

	expired := hasExpired(obj)
	executeDelete(ptr, obj, k)
	// Notify watchers
	WatchChannel <- WatchEvent{k, "GETDEL", obj}
	if expired {
		return nil
	}
	return obj
}

func Del(k string) bool {
	storeMutex.Lock()
	defer storeMutex.Unlock()
	keypoolMutex.Lock()
	defer keypoolMutex.Unlock()

	if ok := delOperation(k); ok {
		return true
	}
	return false
}

func DelByPtr(ptr unsafe.Pointer) bool {
	storeMutex.Lock()
	defer storeMutex.Unlock()
	keypoolMutex.Lock()
	defer keypoolMutex.Unlock()

	if obj, ok := store[ptr]; ok {

Review comment (refactor):
If both `Del` and `GetDel` call `WatchChannel <- WatchEvent{k, "GETDEL", obj}`, can we move this into the function and pass the type of operation being performed to `executeDelete` so that it can distinguish between the source of the event and pass it accordingly into the channel.

Code after:
				err = e
				return
			} else if found {
				keys = append(keys, k)
			}
		}
	}, WithStoreRLock(), WithKeypoolRLock())

	return keys, err
}

// Function to add a new watcher to a query.
func AddWatcher(query DSQLQuery, clientFd int) {
	clients, _ := WatchList.LoadOrStore(query, &sync.Map{})
	clients.(*sync.Map).Store(clientFd, struct{}{})
}

// Function to remove a watcher from a query.
func RemoveWatcher(query DSQLQuery, clientFd int) {
	if clients, ok := WatchList.Load(query); ok {
		clients.(*sync.Map).Delete(clientFd)
		// If no more clients for this query, remove the query from WatchList
		if countClients(clients.(*sync.Map)) == 0 {
			WatchList.Delete(query)
		}
	}
}

// Rename function to implement RENAME functionality using existing helpers
func Rename(sourceKey string, destKey string) bool {
	return withLocksReturn(func() bool {
		// If source and destination are the same, do nothing and return true
		if sourceKey == destKey {
			return true
		}

		sourcePtr, sourceOk := keypool[sourceKey]
		if !sourceOk {
			return false
		}

		sourceObj := store[sourcePtr]
		if sourceObj == nil || hasExpired(sourceObj) {
			if sourceObj != nil {
				deleteKey(sourceKey, sourcePtr, sourceObj)
			}
			return false
		}

		// Use putHelper to handle putting the object at the destination key
		putHelper(destKey, sourceObj)
Repository: DiceDB/dice
File: core/store.go (Go)

Code before:
	}
	obj.LastAccessedAt = getCurrentClock()

	ptr := ensureKeyInPool(k)
	store[ptr] = obj

	incrementKeyCount()
	notifyWatchers(k, "SET", obj)
}

func Get(k string) *Obj {
	return getHelper(k, true)
}

func GetDel(k string) *Obj {
	var v *Obj
	withLocks(func() {
		ptr, ok := keypool[k]
		if !ok {
			return
		}

		v = store[ptr]
		if v != nil {
			expired := hasExpired(v)
			deleteKeyWithOperationName(k, ptr, v, "GETDEL")
			if expired {
				v = nil
			}
		}
	}, WithStoreRLock(), WithKeypoolRLock())
	return v
}

// setExpiry sets the expiry time for an object.
// This method is not thread-safe. It should be called within a lock.
func setExpiry(obj *Obj, expDurationMs int64) {
	expires[obj] = uint64(time.Now().UnixMilli()) + uint64(expDurationMs)
}

// Helper function to count clients
func countClients(clients *sync.Map) int {
	count := 0
	clients.Range(func(_, _ interface{}) bool {
		count++
		return true
	})
	return count
}

// This method is not thread-safe. It should be called within a lock.

Review comment (suggestion):
retaining `DEL` is fine as an operation name, the name of the operation should just signify the core operation that caused a change in the keyset - `DEL` is enough for that.

Code after:
	}
	obj.LastAccessedAt = getCurrentClock()

	ptr := ensureKeyInPool(k)
	store[ptr] = obj

	incrementKeyCount()
	notifyWatchers(k, "SET", obj)
}

func Get(k string) *Obj {
	return getHelper(k, true)
}

func GetDel(k string) *Obj {
	var v *Obj
	withLocks(func() {
		ptr, ok := keypool[k]
		if !ok {
			return
		}

		v = store[ptr]
		if v != nil {
			expired := hasExpired(v)
			deleteKey(k, ptr, v)
			if expired {
				v = nil
			}
		}
	}, WithStoreLock(), WithKeypoolLock())
	return v
}

// setExpiry sets the expiry time for an object.
// This method is not thread-safe. It should be called within a lock.
func setExpiry(obj *Obj, expDurationMs int64) {
	expires[obj] = uint64(time.Now().UnixMilli()) + uint64(expDurationMs)
}

// Helper function to count clients
func countClients(clients *sync.Map) int {
	count := 0
	clients.Range(func(_, _ interface{}) bool {
		count++
		return true
	})
	return count
}

// This method is not thread-safe. It should be called within a lock.
Repository: DiceDB/dice
File: core/store.go (Go)

Code before:
}

// incrementKeyCount increments the key count in the KeyspaceStat map.
// This method is not thread-safe. It should be called within a lock.
func incrementKeyCount() {
	if KeyspaceStat[0] == nil {
		KeyspaceStat[0] = make(map[string]int)
	}
	KeyspaceStat[0]["keys"]++
}

// notifyWatchers sends a WatchEvent to the WatchChannel.
// This function is called whenever a key is updated.
func notifyWatchers(k string, operation string, obj *Obj) {
	WatchChannel <- WatchEvent{k, operation, obj}
}

// deleteKey deletes a key from the store, keypool, and expires maps. it also
// decrements the key count in the KeyspaceStat map and notifies watchers about
// the deletion of the key.
// This method is not thread-safe. It should be called within a lock.
func deleteKey(k string, ptr unsafe.Pointer, obj *Obj) bool {
	return deleteKeyWithOperationName(k, ptr, obj, "DEL")
}

func deleteKeyWithOperationName(k string, ptr unsafe.Pointer, obj *Obj, operation string) bool {
	if obj != nil {
		delete(store, ptr)
		delete(expires, obj)
		delete(keypool, k)
		KeyspaceStat[0]["keys"]--
		notifyWatchers(k, operation, obj)
		return true
	}
	return false
}

// delByPtr deletes a key from the store, keypool, and expires maps using a pointer.
// This method is not thread-safe. It should be called within a lock.
func delByPtr(ptr unsafe.Pointer) bool {
	if obj, ok := store[ptr]; ok {
		key := *((*string)(ptr))
		return deleteKey(key, ptr, obj)
	}
	return false
}


Review comment (suggestion):
As discussed in the other comment, we can stick to the original deleteKey implementation with `DEL` as the operation name being sent to the WatchChannel.

Code after:
}

// incrementKeyCount increments the key count in the KeyspaceStat map.
// This method is not thread-safe. It should be called within a lock.
func incrementKeyCount() {
	if KeyspaceStat[0] == nil {
		KeyspaceStat[0] = make(map[string]int)
	}
	KeyspaceStat[0]["keys"]++
}

// notifyWatchers sends a WatchEvent to the WatchChannel.
// This function is called whenever a key is updated.
func notifyWatchers(k string, operation string, obj *Obj) {
	WatchChannel <- WatchEvent{k, operation, obj}
}

// deleteKey deletes a key from the store, keypool, and expires maps. it also
// decrements the key count in the KeyspaceStat map and notifies watchers about
// the deletion of the key.
// This method is not thread-safe. It should be called within a lock.
func deleteKey(k string, ptr unsafe.Pointer, obj *Obj) bool {
	if obj != nil {
		delete(store, ptr)
		delete(expires, obj)
		delete(keypool, k)
		KeyspaceStat[0]["keys"]--
		notifyWatchers(k, "DEL", obj)
		return true
	}
	return false
}

// delByPtr deletes a key from the store, keypool, and expires maps using a pointer.
// This method is not thread-safe. It should be called within a lock.
func delByPtr(ptr unsafe.Pointer) bool {
	if obj, ok := store[ptr]; ok {
		key := *((*string)(ptr))
		return deleteKey(key, ptr, obj)
	}
	return false
}
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:
		switch arg {
		case "REPLACE":
			isReplace = true
		}
	}

	if isReplace == true {
		Del(destinationKey)
	}

	destinationObj := Get(destinationKey)
	if destinationObj != nil {
		return RESP_ZERO
	}

	sourceType, sourceEncoding := ExtractTypeEncoding(sourceObj)

	var value interface{}
	switch sourceType {
	case OBJ_TYPE_STRING:
		sourceValue := sourceObj.Value.(string)
		byteCopy := []byte(sourceValue)
		value = string(byteCopy)

	case OBJ_TYPE_JSON:
		sourceValue := sourceObj.Value.(map[string]interface{})
		jsonStr, err := sonic.MarshalString(sourceValue)
		if err != nil {
			return RESP_ZERO
		}
		err = sonic.UnmarshalString(jsonStr, &value)
		if err != nil {
			return RESP_ZERO
		}

	case OBJ_TYPE_BITSET:
		sourceValue := sourceObj.Value.(*Bloom)
		value = sourceValue.DeepCopy()

	case OBJ_TYPE_BYTEARRAY:
		sourceValue := sourceObj.Value.(*ByteArray)
		value = sourceValue.DeepCopy()

	case OBJ_TYPE_BYTELIST:
		switch sourceEncoding {
		case OBJ_ENCODING_QINT:
			sourceValue := sourceObj.Value.(*QueueInt)
			value = sourceValue.DeepCopy()

		case OBJ_ENCODING_QREF:
			sourceValue := sourceObj.Value.(*QueueRef)

Review comment (suggestion):
The object doesn't need to be a map of strings if it's a JSON. It could just be an array as well. There are a few such possibilities so please ensure we are testing all possible kinds of JSON values (you can refer to the JSON tests for this).

Code after:

	if isReplace {
		Del(destinationKey)
	}

	destinationObj := Get(destinationKey)
	if destinationObj != nil {
		return RespZero
	}

	copyObj := sourceObj.DeepCopy()
	if copyObj == nil {
		return RespZero
	}

	exp, ok := getExpiry(sourceObj)
	var exDurationMs int64 = -1
	if ok {
		exDurationMs = int64(exp - uint64(time.Now().UnixMilli()))
	}

	Put(destinationKey, copyObj)

	if exDurationMs > 0 {
		setExpiry(copyObj, exDurationMs)
	}
	return RespOne
}

// GETEX key [EX seconds | PX milliseconds | EXAT unix-time-seconds |
// PXAT unix-time-milliseconds | PERSIST]
// Get the value of key and optionally set its expiration.
// GETEX is similar to GET, but is a write command with additional options.
// The GETEX command supports a set of options that modify its behavior:
// EX seconds -- Set the specified expire time, in seconds.
// PX milliseconds -- Set the specified expire time, in milliseconds.
// EXAT timestamp-seconds -- Set the specified Unix time at which the key will expire, in seconds.
// PXAT timestamp-milliseconds -- Set the specified Unix time at which the key will expire, in milliseconds.
// PERSIST -- Remove the time to live associated with the key.
// The RESP value of the key is encoded and then returned
// evalGET returns RespNIL if key is expired or it does not exist
func evalGETEX(args []string) []byte {
	if len(args) < 1 {
		return Encode(errors.New("ERR wrong number of arguments for 'getex' command"), false)
	}

	var key string = args[0]

	// Get the key from the hash table
	obj := Get(key)
Repository: DiceDB/dice
File: core/store.go (Go)

Code before:
	}, WithStoreLock(), WithKeypoolLock())
}

func NewObj(value interface{}, expDurationMs int64, oType, oEnc uint8) *Obj {
	obj := &Obj{
		Value:          value,
		TypeEncoding:   oType | oEnc,
		LastAccessedAt: getCurrentClock(),
	}
	if expDurationMs >= 0 {
		setExpiry(obj, expDurationMs)
	}
	return obj
}

type PutOptions struct {
	KeepTTL bool
}

func Put(k string, obj *Obj, opts ...PutOption) {
	options := getDefaultOptions()

	for _, optApplier := range opts {
		optApplier(options)
	}

	withLocks(func() {
		putHelper(k, obj, options)
	}, WithStoreLock(), WithKeypoolLock())
}

func getDefaultOptions() *PutOptions {
	return &PutOptions{
		KeepTTL: false,
	}
}

type PutOption func(*PutOptions)

func WithKeepTTL(value bool) PutOption {
	return func(po *PutOptions) {
		po.KeepTTL = value
	}
}

// PutAll is a bulk insert function that takes a map of
// keys and values and inserts them into the store
func PutAll(data map[string]*Obj) {
	withLocks(func() {
		for k, obj := range data {
			putHelper(k, obj, nil)

Review comment (refactor):
Do you think it will be better if we do this inside the putHelper (and simply pass the opts directly to it)?

That way we can probably get rid of the nil params being passed to putHelper in a few other places. Additionally, we won't need to do an `opts != nil` check inside the helper.

Code after:
	}, WithStoreLock(), WithKeypoolLock())
}

func NewObj(value interface{}, expDurationMs int64, oType, oEnc uint8) *Obj {
	obj := &Obj{
		Value:          value,
		TypeEncoding:   oType | oEnc,
		LastAccessedAt: getCurrentClock(),
	}
	if expDurationMs >= 0 {
		setExpiry(obj, expDurationMs)
	}
	return obj
}

type PutOptions struct {
	KeepTTL bool
}

func Put(k string, obj *Obj, opts ...PutOption) {
	withLocks(func() {
		putHelper(k, obj, opts...)
	}, WithStoreLock(), WithKeypoolLock())
}

func getDefaultOptions() *PutOptions {
	return &PutOptions{
		KeepTTL: false,
	}
}

type PutOption func(*PutOptions)

func WithKeepTTL(value bool) PutOption {
	return func(po *PutOptions) {
		po.KeepTTL = value
	}
}

// PutAll is a bulk insert function that takes a map of
// keys and values and inserts them into the store
func PutAll(data map[string]*Obj) {
	withLocks(func() {
		for k, obj := range data {
			putHelper(k, obj)
		}
	}, WithStoreLock(), WithKeypoolLock())
}

// GetNoTouch is a function to retrieve a value from the store without updating
// the last accessed time of the object.
Repository: DiceDB/dice
File: core/eval_test.go (Go)

Code before:
				key := "EXISTING_KEY"
				value := "mock_value"
				obj := &Obj{
					Value:          value,
					LastAccessedAt: uint32(time.Now().Unix()),
				}
				store[unsafe.Pointer(obj)] = obj
				keypool[key] = unsafe.Pointer(obj)
				expires[obj] = uint64(time.Now().Add(-2 * time.Minute).Unix())
			},
			input:  []string{"EXISTING_KEY"},
			output: RespNIL,
		},
	}

	runEvalTests(t, tests, evalGET)
}

func testEvalEXPIRETIME(t *testing.T) {
	tests := map[string]evalTestCase{
		"key does not exist": {
			setup:  func() {},
			input:  []string{"NONEXISTENT_KEY"},
			output: RespMinusTwo,
		},
		"key exists without expary": {
			setup: func() {
				key := "EXISTING_KEY"
				value := "mock_value"
				obj := &Obj{
					Value:          value,
					LastAccessedAt: uint32(time.Now().Unix()),
				}
				store[unsafe.Pointer(obj)] = obj
				keypool[key] = unsafe.Pointer(obj)
			},
			input:  []string{"EXISTING_KEY"},
			output: RespMinusOne,
		},
	}

	runEvalTests(t, tests, evalEXPIRETIME)
}

func testEvalJSONGET(t *testing.T) {
	tests := map[string]evalTestCase{
		"nil value": {
			setup:  func() {},
			input:  nil,
			output: []byte("-ERR wrong number of arguments for 'JSON.GET' command\r\n"),
		},

Review comment (suggestion):
```suggestion
		"key exists without expiry": {
```

Code after:
			setup: func() {
				key := "EXISTING_KEY"
				value := "mock_value"
				obj := &Obj{
					Value:          value,
					LastAccessedAt: uint32(time.Now().Unix()),
				}
				store[unsafe.Pointer(obj)] = obj
				keypool[key] = unsafe.Pointer(obj)
				expires[obj] = uint64(time.Now().Add(-2 * time.Minute).Unix())
			},
			input:  []string{"EXISTING_KEY"},
			output: RespNIL,
		},
	}

	runEvalTests(t, tests, evalGET)
}

func testEvalEXPIRETIME(t *testing.T) {
	tests := map[string]evalTestCase{
		"wrong number of args": {
			input:  []string{"KEY1", "KEY2"},
			output: []byte("-ERR wrong number of arguments for 'expire' command\r\n"),
		},
		"key does not exist": {
			input:  []string{"NONEXISTENT_KEY"},
			output: RespMinusTwo,
		},
		"key exists without expiry": {
			setup: func() {
				key := "EXISTING_KEY"
				value := "mock_value"
				obj := &Obj{
					Value:          value,
					LastAccessedAt: uint32(time.Now().Unix()),
				}
				store[unsafe.Pointer(obj)] = obj
				keypool[key] = unsafe.Pointer(obj)
			},
			input:  []string{"EXISTING_KEY"},
			output: RespMinusOne,
		},
		"key exists with expiry": {
			setup: func() {
				key := "EXISTING_KEY"
				value := "mock_value"
				obj := &Obj{
					Value:          value,
					LastAccessedAt: uint32(time.Now().Unix()),
				}