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

Code before:
	}

	// Add the client to this key's watch list
	WatchList[query][c.Fd] = struct{}{}

	return RESP_OK
}

// 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(fmt.Errorf("(error) ERR wrong number of arguments for 'command' command"), false)
	}
	subcommand := strings.ToUpper(args[0])
	switch subcommand {
	case "COUNT":
		return evalCOMMANDCOUNT()
	default:
		return Encode(fmt.Errorf("ERR unknown subcommand '%s'. Try COMMAND HELP", subcommand), false)
	}
}

// evalCOMMANDCOUNT returns an number of commands supported by DiceDB
func evalCOMMANDCOUNT() []byte {
	return Encode(len(diceCommands), false)
}

func executeCommand(cmd *RedisCmd, c *Client) []byte {
	switch cmd.Cmd {
	case PING:
		return evalPING(cmd.Args)
	case SET:
		return evalSET(cmd.Args)
	case GET:
		return evalGET(cmd.Args)
	case TTL:
		return evalTTL(cmd.Args)
	case DEL:
		return evalDEL(cmd.Args)
	case EXPIRE:
		return evalEXPIRE(cmd.Args)
	case HELLO:
		return evalHELLO(cmd.Args)
	case BGREWRITEAOF:
		return evalBGREWRITEAOF(cmd.Args)
	case INCR:
		return evalINCR(cmd.Args)
	case INFO:
		return evalINFO(cmd.Args)
	case CLIENT:

Review comment (suggestion):
instead of calculating the length of the commands for every request, we can simply store it in a constant and return that. We can add a runtime check to verify that the constant's value does not get outdated.
```go
const (
    diceCommandsCount = len(diceCommands)
)

func init() {
    if len(diceCommands) != diceCommandsCount {
        panic("diceCommands slice has been modified")
    }
}
```

Code after:

	return diceCmd.Eval(cmd.Args)
}

func executeCommandToBuffer(cmd *RedisCmd, buf *bytes.Buffer, c *Client) {
	buf.Write(executeCommand(cmd, c))
}

func EvalAndRespond(cmds RedisCmds, c *Client) {
	var response []byte
	buf := bytes.NewBuffer(response)

	for _, cmd := range cmds {
		// if txn is not in progress, then we can simply
		// execute the command and add the response to the buffer
		if !c.isTxn {
			executeCommandToBuffer(cmd, buf, c)
			continue
		}

		// if the txn is in progress, we enqueue the command
		// and add the QUEUED response to the buffer
		if !txnCommands[cmd.Cmd] {
			// if the command is queuable the enqueu
			c.TxnQueue(cmd)
			buf.Write(RESP_QUEUED)
		} else {
			// if txn is active and the command is non-queuable
			// ex: EXEC, DISCARD
			// we execute the command and gather the response in buffer
			executeCommandToBuffer(cmd, buf, c)
		}
	}

	if _, err := c.Write(buf.Bytes()); err != nil {
		log.Panic(err)
	}
}
Repository: DiceDB/dice
File: core/commands.go (Go)

Code before:
type DiceCmdMeta struct {
	Name string
	Info string
	Docs string
	Eval func([]string) []byte
}

var (
	diceCmds = map[string]DiceCmdMeta{}

	pingCmdMeta = DiceCmdMeta{
		Name: "PING",
		Info: "ping command info here",
		Docs: "ping command docs here",
		Eval: evalPING,
	}
	setCmdMeta = DiceCmdMeta{
		Name: "SET",
		Info: "set command info here",
		Docs: "set command docs here",
		Eval: evalSET,
	}
	getCmdMeta = DiceCmdMeta{
		Name: "GET",
		Info: "get command info here",
		Docs: "get command docs here",
		Eval: evalGET,
	}
	ttlCmdMeta = DiceCmdMeta{
		Name: "TTL",
		Info: "ttl command info here",
		Docs: "ttl command docs here",
		Eval: evalTTL,
	}
	delCmdMeta = DiceCmdMeta{
		Name: "DEL",
		Info: "del command info here",
		Docs: "del command docs here",
		Eval: evalDEL,
	}
	expireCmdMeta = DiceCmdMeta{
		Name: "EXPIRE",
		Info: "expire command info here",
		Docs: "expire command docs here",
		Eval: evalEXPIRE,
	}
	helloCmdMeta = DiceCmdMeta{
		Name: "HELLO",
		Info: "hello command info here",
		Docs: "hello command docs here",
		Eval: evalHELLO,

Review comment (suggestion):
Assuming Docs is a link to documentation - 
We don't have a docs portal yet. Should we skip this atm? Any placeholders that we won't address in short term is smell for me.
Its easy enough to add it later in one go for all once we do something for docs.

Code after:
type DiceCmdMeta struct {
	Name string
	Info string
	Eval func([]string) []byte
}

var (
	diceCmds = map[string]DiceCmdMeta{}

	pingCmdMeta = DiceCmdMeta{
		Name: "PING",
		Info: `PING returns with an encoded "PONG" If any message is added with the ping command,the message will be returned.`,
		Eval: evalPING,
	}
	setCmdMeta = DiceCmdMeta{
		Name: "SET",
		Info: `SET puts a new <key, value> pair in db as in the args
		args must contain key and value.
		args can also contain multiple options -
		EX or ex which will set the expiry time(in secs) for the key
		Returns encoded error response if at least a <key, value> pair is not part of args
		Returns encoded error response if expiry tme value in not integer
		Returns encoded OK RESP once new entry is added
		If the key already exists then the value will be overwritten and expiry will be discarded`,
		Eval: evalSET,
	}
	getCmdMeta = DiceCmdMeta{
		Name: "GET",
		Info: `GET 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
		GET returns RESP_NIL if key is expired or it does not exist`,
		Eval: evalGET,
	}
	ttlCmdMeta = DiceCmdMeta{
		Name: "TTL",
		Info: `TTL returns Time-to-Live in secs for the queried key in args
		 The key should be the only param in args else returns with an error
		 Returns	
		 RESP encoded time (in secs) remaining for the key to expire
		 RESP encoded -2 stating key doesn't exist or key is expired
		 RESP encoded -1 in case no expiration is set on the key`,
		Eval: evalTTL,
	}
	delCmdMeta = DiceCmdMeta{
		Name: "DEL",
		Info: `DEL deletes all the specified keys in args list
		returns the count of total deleted keys after encoding`,
		Eval: evalDEL,
	}
	expireCmdMeta = DiceCmdMeta{
Repository: DiceDB/dice
File: core/store.go (Go)

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

	if obj, ok := store[ptr]; ok {
		delete(store, ptr)
		delete(expires, obj)
		delete(keypool, *((*string)(ptr)))
		KeyspaceStat[0]["keys"]--
		return true
	}
	return false
}

// List all keys in the store by given pattern
func Keys(p string) []string {
	storeMutex.RLock()
	defer storeMutex.RUnlock()
	keypoolMutex.RLock()
	defer keypoolMutex.RUnlock()

	keys := make([]string, 0)
	for k := range keypool {
		if found, err := path.Match(p, k); err == nil && found {
			keys = append(keys, k)
		}
	}

	return keys
}

// 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 clientCount := countClients(clients.(*sync.Map)); clientCount == 0 {
			WatchList.Delete(query)
		}
	}
}

// Helper function to count clients

Review comment (suggestion):
I'm unsure if ignoring the error message here is a good idea. We should return the error to the caller if something bad happens

Code after:
}

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

	if obj, ok := store[ptr]; ok {
		delete(store, ptr)
		delete(expires, obj)
		delete(keypool, *((*string)(ptr)))
		KeyspaceStat[0]["keys"]--
		return true
	}
	return false
}

// List all keys in the store by given pattern
func Keys(p string) ([]string, error) {
	storeMutex.RLock()
	defer storeMutex.RUnlock()
	keypoolMutex.RLock()
	defer keypoolMutex.RUnlock()

	keyCap := 0
	if p == "*" {
		keyCap = len(keypool)
	}

	keys := make([]string, 0, keyCap)
	for k := range keypool {
		if found, err := path.Match(p, k); err != nil {
			return nil, err
		} else if found {
			keys = append(keys, k)
		}
	}

	return keys, nil
}

// 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 {
Repository: DiceDB/dice
File: tests/keys_test.go (Go)

Code before:
package tests

import (
	"testing"

	"gotest.tools/v3/assert"
)

func TestKeys(t *testing.T) {
	conn := getLocalConnection()
	for _, tcase := range []DTestCase{
		{
			InCmds: []string{"SET k v", "KEYS k"},
			Out:    []interface{}{"OK", []interface{}{"k"}},
		},
		{
			InCmds: []string{"SET good v", "SET great v", "KEYS g*"},
			Out:    []interface{}{"OK", "OK", []interface{}{"good", "great"}},
		},
		{
			InCmds: []string{"SET good v", "SET great v", "KEYS g?od", "KEYS g?eat"},
			Out:    []interface{}{"OK", "OK", []interface{}{"good"}, []interface{}{"great"}},
		},
		{
			InCmds: []string{"SET hallo v", "SET hbllo v", "SET hello v", "KEYS h[^e]llo"},
			Out:    []interface{}{"OK", "OK", "OK", []interface{}{"hallo", "hbllo"}},
		},
	} {
		for i := 0; i < len(tcase.InCmds); i++ {
			cmd := tcase.InCmds[i]
			out := tcase.Out[i]
			assert.DeepEqual(t, out, fireCommand(conn, cmd))
		}
	}
}


Review comment (suggestion):
Please add a test covering the following pattern:
```
h[a-b]llo matches hallo and hbllo
```

Code after:
	actualSlice, ok := actual.([]interface{})
	if !ok {
		return false
	}

	if len(expectedSlice) != len(actualSlice) {
		return false
	}

	for _, e := range expectedSlice {
		found := false
		for _, a := range actualSlice {
			if e == a {
				found = true
				break
			}
		}
		if !found {
			return false
		}
	}

	return true
}

func TestKeys(t *testing.T) {
	conn := getLocalConnection()
	testCases := []struct {
		name     string
		commands []string
		expected []interface{}
	}{
		{
			name:     "k matches with k",
			commands: []string{"SET k v", "KEYS k"},
			expected: []interface{}{"OK", []interface{}{"k"}},
		},
		{
			name:     "g* matches good and great",
			commands: []string{"SET good v", "SET great v", "KEYS g*"},
			expected: []interface{}{"OK", "OK", []interface{}{"good", "great"}},
		},
		{
			name:     "g?od matches good",
			commands: []string{"SET good v", "SET great v", "KEYS g?od"},
			expected: []interface{}{"OK", "OK", []interface{}{"good"}},
		},
		{
			name:     "g?eat matches great",
			commands: []string{"SET good v", "SET great v", "KEYS g?eat"},
			expected: []interface{}{"OK", "OK", []interface{}{"great"}},
Repository: DiceDB/dice
File: core/eval_test.go (Go)

Code before:
	"time"
	"unsafe"

	"github.com/bytedance/sonic"
	"gotest.tools/v3/assert"
)

type testCase struct {
	setup     func()
	input     []string
	output    []byte
	validator func(output []byte)
}

func setupTest() {
	store = make(map[unsafe.Pointer]*Obj)
	keypool = make(map[string]unsafe.Pointer)
	expires = make(map[*Obj]uint64)
	KeyspaceStat[0] = make(map[string]int)
}

func teardownTest() {
	store = nil
	keypool = nil
	expires = nil
}

func TestEval(t *testing.T) {
	testCases := map[string]func(*testing.T){
		"MSET":    testEvalMSET,
		"PING":    testEvalPING,
		"HELLO":   testEvalHELLO,
		"SET":     testEvalSET,
		"GET":     testEvalGET,
		"JSONGET": testEvalJSONGET,
		"JSONSET": testEvalJSONSET,
		"TTL":     testEvalTTL,
		"DEL":     testEvalDel,
		"PERSIST": TestEvalPersist,
	}

	for name, testFunc := range testCases {
		t.Run(name, testFunc)
	}
}

func testEvalPING(t *testing.T) {
	tests := map[string]testCase{
		"nil value":            {input: nil, output: []byte("+PONG\r\n")},
		"empty args":           {input: []string{}, output: []byte("+PONG\r\n")},
		"one value":            {input: []string{"HEY"}, output: []byte("$3\r\nHEY\r\n")},

Review comment (suggestion):
You can call `RestetStore()` here instead.
I also noticed that `ResetStore()` doesn't reset the KeyspaceStat map, so please add the line below into the ResetStore function as well as a part of this PR.

Code after:
	"time"
	"unsafe"

	"github.com/bytedance/sonic"
	"gotest.tools/v3/assert"
)

type evalTestCase struct {
	setup     func()
	input     []string
	output    []byte
	validator func(output []byte)
}

func resetStore() {
	store = nil
	keypool = nil
	expires = nil
	KeyspaceStat[0] = nil
}

func setupTest() {
	resetStore()
	store = make(map[unsafe.Pointer]*Obj)
	keypool = make(map[string]unsafe.Pointer)
	expires = make(map[*Obj]uint64)
	KeyspaceStat[0] = make(map[string]int)
}

func TestEval(t *testing.T) {
	testCases := map[string]func(*testing.T){
		"MSET":    testEvalMSET,
		"PING":    testEvalPING,
		"HELLO":   testEvalHELLO,
		"SET":     testEvalSET,
		"GET":     testEvalGET,
		"JSONGET": testEvalJSONGET,
		"JSONSET": testEvalJSONSET,
		"TTL":     testEvalTTL,
		"DEL":     testEvalDel,
		"PERSIST": TestEvalPersist,
	}

	for name, testFunc := range testCases {
		t.Run(name, testFunc)
	}
}

func testEvalPING(t *testing.T) {
	tests := map[string]evalTestCase{
		"nil value":            {input: nil, output: []byte("+PONG\r\n")},
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:
		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 "EXAT", "exat":
			i++
			if i == len(args) {
				return Encode(errors.New("ERR syntax error"), false)
			}

			exAt, 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)
			}

			currentTime := time.Now().Unix()
			if exAt <= currentTime {
				return Encode(errors.New("ERR invalid EXAT time in the past"), false)
			}

			exDurationMs = (exAt - currentTime) * 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

Review comment (bug):
This behaviour doesn't reflect the same what's done in redis. 
Redis does not reject the `EXAT` value if it is in the past. Instead, it sets the key and immediately expires it if the timestamp has already passed.
@JyotinderSingh @arpitbbhayani please correct me if I'm wrong here.

Code after:

	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)
		}
	}

	// putting the k and value in a Hash Table
	Put(key, NewObj(value, exDurationMs, oType, oEnc))
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:
// evalJSONGET retrieves a JSON value stored at the specified key
// args must contain at least the key;  (path unused in this implementation)
// Returns RESP_NIL if key is expired or it does not exist
// Returns encoded error response if incorrect number of arguments
// The RESP value of the key is encoded and then returned
func evalJSONGET(args []string) []byte {
	// Check if there are enough arguments
	if len(args) < 1 {
		return Encode(errors.New("ERR wrong number of arguments for 'JSON.GET' command"), false)
	}

	key := args[0]
	// Default path is root if not specified
	path := "$"
	if len(args) > 1 {
		path = args[1]
	}

	// Retrieve the object from the database
	obj := Get(key)
	if obj == nil || hasExpired(obj) {
		return RESP_NIL
	}

	// Check if the object is of JSON type
	objType, _ := ExtractTypeEncoding(obj)
	if objType != OBJ_TYPE_JSON {
		return Encode(errors.New("WRONGTYPE Operation against a key holding the wrong kind of value"), false)
	}

	jsonData := obj.Value

	// If path is root, return the entire JSON
	if path == "$" {
		resultBytes, err := json.Marshal(jsonData)
		if err != nil {
			return Encode(errors.New("ERR could not serialize result"), false)
		}
		return Encode(string(resultBytes), false)
	}

	// Parse the JSONPath expression
	expr, err := jp.ParseString(path)
	if err != nil {
		return Encode(errors.New("ERR invalid JSONPath"), false)
	}

	// Execute the JSONPath query
	results := expr.Get(jsonData)
	if len(results) == 0 {
		return RESP_NIL

Review comment (suggestion):
Use `assertType` and `assertEncoding` methods for type and encoding verification.

Code after:
	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
	}

	// return the RESP encoded value
	return Encode(obj.Value, false)
}

// evalJSONGET retrieves a JSON value stored at the specified key
// args must contain at least the key;  (path unused in this implementation)
// Returns RESP_NIL if key is expired or it does not exist
// Returns encoded error response if incorrect number of arguments
// The RESP value of the key is encoded and then returned
func evalJSONGET(args []string) []byte {
	if len(args) < 1 {
		return Encode(errors.New("ERR wrong number of arguments for 'JSON.GET' command"), false)
	}

	key := args[0]
	// Default path is root if not specified
	path := defaultRootPath
	if len(args) > 1 {
		path = args[1]
	}

	// Retrieve the object from the database
	obj := Get(key)
	if obj == nil {
		return RESP_NIL
	}

	// Check if the object is of JSON type
Repository: DiceDB/dice
File: core/eval.go (Go)

Code before:
	}

	key := args[0]
	path := args[1]
	jsonStr := args[2]

	// Parse the JSON string
	var jsonValue interface{}
	if err := json.Unmarshal([]byte(jsonStr), &jsonValue); err != nil {
		return Encode(fmt.Errorf("ERR invalid JSON: %v", err), false)
	}

	// Retrieve existing object or create new one
	obj := Get(key)
	var rootData interface{}

	if obj == nil {
		// If the key doesn't exist, create a new object
		if path != "$" {
			rootData = make(map[string]interface{})
		} else {
			rootData = jsonValue
		}
	} else {
		// If the key exists, check if it's a JSON object
		objType, _ := ExtractTypeEncoding(obj)
		if objType != OBJ_TYPE_JSON {
			return Encode(errors.New("WRONGTYPE Operation against a key holding the wrong kind of value"), false)
		}
		rootData = obj.Value
	}

	// If path is not root, use JSONPath to set the value
	if path != "$" {
		expr, err := jp.ParseString(path)
		if err != nil {
			return Encode(errors.New("ERR invalid JSONPath"), false)
		}

		err = expr.Set(rootData, jsonValue)
		if err != nil {
			return Encode(errors.New("ERR failed to set value"), false)
		}
	} else {
		// If path is root, replace the entire JSON
		rootData = jsonValue
	}

	// Create a new object with the updated JSON data
	newObj := NewObj(rootData, -1, OBJ_TYPE_JSON, OBJ_ENCODING_JSON)
	Put(key, newObj)

Review comment (suggestion):
Use `assertType` and `assertEncoding` methods for type and encoding verification.

Code after:
		return Encode(errors.New("ERR invalid JSONPath"), false)
	}

	// Execute the JSONPath query
	results := expr.Get(jsonData)
	if len(results) == 0 {
		return RESP_NIL
	}

	// Serialize the result
	var resultBytes []byte
	if len(results) == 1 {
		resultBytes, err = json.Marshal(results[0])
	} else {
		resultBytes, err = json.Marshal(results)
	}
	if err != nil {
		return Encode(errors.New("ERR could not serialize result"), false)
	}
	return Encode(string(resultBytes), false)
}

// evalJSONSET stores a JSON value at the specified key
// args must contain at least the key, path (unused in this implementation), and JSON string
// Returns encoded error response if incorrect number of arguments
// Returns encoded error if the JSON string is invalid
// Returns RESP_OK if the JSON value is successfully stored
func evalJSONSET(args []string) []byte {
	// Check if there are enough arguments
	if len(args) < 3 {
		return Encode(errors.New("ERR wrong number of arguments for 'JSON.SET' command"), false)
	}

	key := args[0]
	path := args[1]
	jsonStr := args[2]

	// Parse the JSON string
	var jsonValue interface{}
	if err := json.Unmarshal([]byte(jsonStr), &jsonValue); err != nil {
		return Encode(fmt.Errorf("ERR invalid JSON: %v", err), false)
	}

	// Retrieve existing object or create new one
	obj := Get(key)
	var rootData interface{}

	if obj == nil {
		// If the key doesn't exist, create a new object
		if path != defaultRootPath {
			rootData = make(map[string]interface{})