Go: Implementing Hash commands #2226

umit · 2024-09-04T19:28:55Z

Implement Valkey Hash Commands

This PR implements the following Valkey hash commands:

HGet: Get value of a hash field
HGetAll: Get all fields and values in a hash
HMGet: Get values of multiple hash fields
HSet: Set value of a hash field
HSetNX: Set value of a hash field if not exists
HDel: Delete hash field(s)
HLen: Get number of fields in a hash
HVals: Get all values in a hash
HExists: Check if a hash field exists
HKeys: Get all field names in a hash
HStrLen: Get string length of a hash field's value

eifrah-aws

Thanks for providing this PR!, in general it looks good

My major concerns are:

Some of the methods implemented are "swallowing" errors (for example: HStrLen does not return NULL by Redis, and yet we assume that null is valid and we convert it into 0 while we should return an error to the caller
The Rust code assumes valid strings are returned from redis/valkey, while this is not the case. Do not use String. Assume that a "string" is &[u8] or Vec<u8>

go/api/response_handlers.go

go/api/base_client.go

go/api/response_handlers.go

go/api/base_client.go

go/src/lib.rs

go/api/commands.go

Yury-Fridlyand · 2024-09-05T18:16:41Z

go/api/commands.go

+	// See [valkey.io] for details.
+	//
+	// For example:
+	//


To have docs aligned and common style

Suggested change

//

@janhavigupta007 I've removed.

go/api/commands.go

go/integTest/shared_commands_test.go

go/src/lib.rs

go/utils/transform_utils_test.go

Yury-Fridlyand · 2024-09-05T18:27:47Z

go/src/lib.rs

+                let (vec_ptr, len) = convert_vec_to_pointer(result);
+                let (vec_elements_len_ptr, _) = convert_vec_to_pointer(vec_elements_len);


Please confirm that we're not leaking memory there, especially on an error

+1. Also, I'm curious to know why convert_vec_to_pointer uses mem::forget.

go/api/commands.go

jonathanl-bq · 2024-09-06T21:45:42Z

go/src/lib.rs

+    if !array_value.is_null() {
+        let len = array_value_len as usize;
+        let vec = unsafe { Vec::from_raw_parts(array_value, len, len) };
+        let elem_len = unsafe { Vec::from_raw_parts(array_elements_len, len, len) };


array_elements_len is a pointer, so it could be null here.

go/src/lib.rs

go/api/response_handlers.go

eifrah-aws · 2024-09-10T06:11:31Z

go/api/response_handlers.go

+	numElements := int(response.array_value_len)
+	if len(lengths) != numElements {
+		// Handle inconsistent array lengths (just in case)
+		return nil


Return an error, don't hide it with nil

go/api/response_handlers.go

go/api/base_client.go

ravisi21 · 2024-09-16T07:06:37Z

go/api/response_handlers.go

@@ -31,6 +32,12 @@ func handleStringOrNullResponse(response *C.struct_CommandResponse) string {
 	return handleStringResponse(response)
 }

+// handleBooleanResponse converts a C struct_CommandResponse's bool_value to a Go bool.
+func handleBooleanResponse(response *C.struct_CommandResponse) bool {


Can we have the methods consistent please?

E.g.:

Either

getXXXResponse returns XXX OR

getXXXResponse returns XXX, err

@ravisi21 It's done.

ravisi21 · 2024-09-16T07:22:22Z

go/src/lib.rs

+                let result = map
+                    .into_iter()
+                    .flat_map(|(key, value)| {
+                        let key = <Vec<u8>>::from_redis_value(&key)


This seems like reducing a redis::Value to always a string.
That may serve a specific purpose now of Map<String, String>, but I dont think that would be a right implementation.

We should try to convert the redis::Value to CommandResponse so that the generic nature is maintained.

We are working towards that as you can check in the PR: #2289

eifrah-aws

Hey, I did not forget about this PR. I am waiting for this PR: #2289 to be merged as it fixes basic data exchanging between Rust <-> Go

Yury-Fridlyand

LGTM if all other comments are addressed.

janhavigupta007

Thank you so much for adding hash commands implementation. I am waiting on reviewing this PR to get these PRs #2303 and #2289 merged first. Please let me know if there are any questions/concerns.

janhavigupta007

The pending PRs #2289 amd #2303 have been merged. Please rebase your changes so that we can have a review for this PR.

Signed-off-by: umit <[email protected]>

janhavigupta007 · 2024-09-20T10:42:06Z

go/api/commands.go

+	// See [valkey.io] for details.
+	//
+	// For example:
+	//


janhavigupta007 · 2024-09-20T10:44:42Z

go/api/commands.go

+	//
+	// For example:
+	//  payload, err := client.HGet("my_hash", "field1")
+	//  // payload equals "value"


Can we also show what my_hash contains in order to be clear on how "value" is being returned.

@janhavigupta007 I've added my_hash values in the comment.

janhavigupta007 · 2024-09-20T10:48:51Z

go/api/commands.go

+	//  For every field that does not exist in the hash, a null value is returned.
+	//  If key does not exist, it is treated as an empty hash, and it returns an array of null values.


If the value is not present, an empty string would be returned as string can't be null in GO.

@janhavigupta007 I've updated the message according to your comment.

janhavigupta007 · 2024-09-20T10:50:36Z

go/api/commands.go

+	// For example:
+	//  num1, err := client.HLen("myHash")
+	//  // num1 equals 3
+	//


Suggested change

//

@janhavigupta007 It's done.

janhavigupta007 · 2024-09-20T10:51:00Z

go/api/commands.go

+	// For example:
+	//  exists, err := client.HExists("my_hash", "field1")
+	//  // exists equals true
+	//


Suggested change

//

@janhavigupta007 It's done.

janhavigupta007 · 2024-09-20T10:53:41Z

go/api/base_client.go

 }

 func (client *baseClient) MSetNX(keyValueMap map[string]string) (bool, error) {
 	result, err := client.executeCommand(C.MSetNX, utils.MapToString(keyValueMap))
 	if err != nil {
 		return false, err
 	}
-	return handleBooleanResponse(result), nil
+


In some functions, extra spaces are added and somewhere it's not. Let's keep it consistent.

Suggested change

@janhavigupta007 I've updated with extra spaces. The separate return statement on a new line is generally considered better practice in Go.

Does this something that can be handled by gofmt?

janhavigupta007 · 2024-09-20T10:58:39Z

go/api/response_handlers.go

+
+	result := make(map[string]string, response.array_value_len)
+	values := unsafe.Slice(response.array_value, response.array_value_len)
+
+	for _, v := range values {
 		key := convertCharArrayToString(v.map_key.string_value, v.map_key.string_value_len)
 		value := convertCharArrayToString(v.map_value.string_value, v.map_value.string_value_len)
-		m[key] = value
+		result[key] = value
 	}


Any reason we are modifying this? Also, please retain the ordering of the functions so that it can only reflect the changes made and it will be easier for review.

@janhavigupta007 The function was updated during the rebase operation. I revert it and added null response check.

janhavigupta007 · 2024-09-20T11:07:19Z

go/api/base_client.go

+	res, err := handleStringToStringMapResponse(result)
+	if err != nil {
+		return nil, err
+	}
+
+	return res, nil


We can directly return handleStringToStringMapResponse(result) here.

@janhavigupta007 It's done.

janhavigupta007 · 2024-09-20T11:08:15Z

go/api/base_client.go

+func (client *baseClient) HMGet(key string, fields []string) ([]string, error) {
+	result, err := client.executeCommand(C.HMGet, append([]string{key}, fields...))
+	if err != nil {
+		return []string{}, err


Please return nil, err from here.

@janhavigupta007 It's fixed.

Signed-off-by: umit <[email protected]>

eifrah-aws · 2024-09-23T06:49:20Z

go/api/base_client.go

@@ -146,63 +147,71 @@ func (client *baseClient) Set(key string, value string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return handleStringResponse(result), nil
+
+	return handleStringResponse(result)


Thank you for fixing this 👍

eifrah-aws · 2024-09-23T06:51:28Z

go/api/base_client.go

 }

 func (client *baseClient) MSetNX(keyValueMap map[string]string) (bool, error) {
 	result, err := client.executeCommand(C.MSetNX, utils.MapToString(keyValueMap))
 	if err != nil {
 		return false, err
 	}
-	return handleBooleanResponse(result), nil
+


Does this something that can be handled by gofmt?

umit requested a review from a team as a code owner September 4, 2024 19:28

umit force-pushed the go/hash-commands-implementation branch 3 times, most recently from 5da6299 to 184a6a4 Compare September 4, 2024 20:20

avifenesh requested a review from eifrah-aws September 5, 2024 10:15

avifenesh assigned umit Sep 5, 2024

avifenesh added the go golang wrapper label Sep 5, 2024

eifrah-aws requested changes Sep 5, 2024

View reviewed changes

umit force-pushed the go/hash-commands-implementation branch 2 times, most recently from 9ed4260 to 3f9ec9d Compare September 5, 2024 16:23

Yury-Fridlyand reviewed Sep 5, 2024

View reviewed changes

umit force-pushed the go/hash-commands-implementation branch from 70723d9 to 420f031 Compare September 5, 2024 18:38

Yury-Fridlyand reviewed Sep 5, 2024

View reviewed changes

go/api/commands.go Outdated Show resolved Hide resolved

umit force-pushed the go/hash-commands-implementation branch 2 times, most recently from f476f8e to 14200f1 Compare September 5, 2024 20:04

jonathanl-bq reviewed Sep 6, 2024

View reviewed changes

go/src/lib.rs Outdated Show resolved Hide resolved

janhavigupta007 reviewed Sep 9, 2024

View reviewed changes

go/api/response_handlers.go Outdated Show resolved Hide resolved

eifrah-aws requested changes Sep 10, 2024

View reviewed changes

avifenesh requested a review from eifrah-aws September 13, 2024 14:54

umit force-pushed the go/hash-commands-implementation branch from 14200f1 to 4e776c1 Compare September 13, 2024 16:51

umit requested review from Yury-Fridlyand and janhavigupta007 September 13, 2024 16:52

ravisi21 reviewed Sep 16, 2024

View reviewed changes

eifrah-aws reviewed Sep 16, 2024

View reviewed changes

Yury-Fridlyand approved these changes Sep 16, 2024

View reviewed changes

janhavigupta007 reviewed Sep 17, 2024

View reviewed changes

janhavigupta007 requested changes Sep 19, 2024

View reviewed changes

add HashCommands interface and tests for hash-related commands

632d990

Signed-off-by: umit <[email protected]>

umit force-pushed the go/hash-commands-implementation branch from ae02a9a to 632d990 Compare September 19, 2024 17:51

janhavigupta007 requested changes Sep 20, 2024

View reviewed changes

update method comments and style

a5c3570

Signed-off-by: umit <[email protected]>

eifrah-aws self-requested a review September 23, 2024 06:48

eifrah-aws approved these changes Sep 23, 2024

View reviewed changes

janhavigupta007 approved these changes Sep 23, 2024

View reviewed changes

eifrah-aws merged commit 634f1c8 into valkey-io:main Sep 23, 2024
13 checks passed

		let (vec_ptr, len) = convert_vec_to_pointer(result);
		let (vec_elements_len_ptr, _) = convert_vec_to_pointer(vec_elements_len);

		// For every field that does not exist in the hash, a null value is returned.
		// If key does not exist, it is treated as an empty hash, and it returns an array of null values.

Go: Implementing Hash commands #2226

Go: Implementing Hash commands #2226

Conversation

umit commented Sep 4, 2024

Implement Valkey Hash Commands

eifrah-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eifrah-aws left a comment

Choose a reason for hiding this comment

Yury-Fridlyand left a comment

Choose a reason for hiding this comment

janhavigupta007 left a comment

Choose a reason for hiding this comment

janhavigupta007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment