Added support for GET option in SET command #322

yaten2302 · 2024-08-15T10:01:56Z

Fixes #207

JyotinderSingh

Thanks for the changes @yaten2302, have left a few comments.

core/eval.go

JyotinderSingh · 2024-08-15T18:07:31Z

core/eval.go

+				} else {
+					return RespNIL
+				}


You still need to Put() the key even if it did not previously exist when using the GET option.

refer: https://redis.io/docs/latest/commands/set/

JyotinderSingh · 2024-08-15T18:08:01Z

core/eval.go

@@ -193,6 +197,23 @@ func evalSET(args []string) []byte {
 			}
 		case constants.KEEPTTL, constants.Keepttl:
 			keepttl = true
+		case "GET":
+			// Get the key from the hash table
+			obj := Get(key)


An error is returned and SET aborted if the value stored at key is not a string.

refer: https://redis.io/docs/latest/commands/set/

@JyotinderSingh , I've a doubt, that by default dice stores everything as a string only, right? Then why this error is being shown?

The behavior should be correct from the implementation perspective. So we should do the check on our side.

Oh! Okay 👍
BTW @JyotinderSingh , I wanted to ask that could you please provide a sample test case like when the test is failing? like I've a doubt that how can I set the value to int or something else (anything other than string)?

If possible, may I give it a try please, which is causing this problem! (like if the value is not string and then it's creating a problem). Like, if anyone else is already working on it, then I can collaborate with them 🙏 ?

JyotinderSingh · 2024-08-15T18:10:54Z

tests/set_test.go

@@ -113,6 +113,11 @@ func TestSetWithOptions(t *testing.T) {
 			commands: []string{"SET k v XX EX 1", "GET k", "SLEEP 2", "GET k", "SET k v XX EX 1", "GET k"},
 			expected: []interface{}{"(nil)", "(nil)", "OK", "(nil)", "(nil)", "(nil)"},
 		},
+		{


Add test cases for the following:

Validate that SET succeeds with GET option even if the key does not exist.

Validate error is returned when using SET with GET option if the previously stored value was not a string.

Validate previous value is returned when using SET with the GET option.

Sure, I'll add these test cases 👍

@JyotinderSingh , actually, I've some doubts:

like for the 1st one, in redis it's given that if the key doesn't exists, then it should return (nil)?

Also, could you please guide that how can I validate that if the previous values was a string or not? Because dice stores everything as string, am I right? Like 2 will be stored as "2"?

I've got the 3rd point. I'll add that 👍

Even though it returns nil, the key is still set in the background. You return nil since there was no existing value, however, the new value will still be set.

Add the validation regardless.

lucifercr07

I remember there were few comments had posted on last PR for the same issue, can you please mention if they've been fixed?

AshwinKul28 · 2024-08-16T11:08:52Z

Linter is still failing :( will look into it.

yaten2302 · 2024-08-16T14:03:05Z

@lucifercr07 , yes, I've made all the changes in the PR which were suggested previously, but I haven't made this change, because the tests were failing if I remove the else block.

lucifercr07 · 2024-08-16T14:21:15Z

@lucifercr07 , yes, I've made all the changes in the PR which were suggested previously, but I haven't made this change, because the tests were failing if I remove the else block.

Can you detail on why it's failing? Any reasons

yaten2302 · 2024-08-16T15:26:14Z

Sure @lucifercr07 , here is the error on removing the else blocks:

lucifercr07 · 2024-08-16T15:37:22Z

Sure @lucifercr07 , here is the error on removing the else blocks:

We need to remove the else block but not the code inside it.

if XXoption {
				if obj == nil {
					return RespNIL
				} 
				
                                 Put(key, NewObj(value, exDurationMs, oType, oEnc))
			} else {
				if obj == nil {
					return Put(key, NewObj(value, exDurationMs, oType, oEnc))
				}
				
                                 return RespNIL
}

yaten2302 · 2024-08-17T04:35:25Z

@lucifercr07 , @JyotinderSingh , I've pushed some changes. could you please review those 👍
Also, @AshwinKul28 , could you please guide that is the linter failing due to some issues from my side? or it's something else?

JyotinderSingh · 2024-08-17T04:56:36Z

@lucifercr07 , @JyotinderSingh , I've pushed some changes. could you please review those 👍

Also, @AshwinKul28 , could you please guide that is the linter failing due to some issues from my side? or it's something else?

Lint checks are passing now since you merged with master.

JyotinderSingh · 2024-08-19T18:24:19Z

core/eval.go

@@ -193,6 +197,23 @@ func evalSET(args []string) []byte {
 			}
 		case constants.KEEPTTL, constants.Keepttl:
 			keepttl = true
+		case "GET":
+			// Get the key from the hash table
+			obj := Get(key)


The behavior should be correct from the implementation perspective. So we should do the check on our side.

JyotinderSingh · 2024-08-19T18:25:52Z

tests/set_test.go

@@ -113,6 +113,11 @@ func TestSetWithOptions(t *testing.T) {
 			commands: []string{"SET k v XX EX 1", "GET k", "SLEEP 2", "GET k", "SET k v XX EX 1", "GET k"},
 			expected: []interface{}{"(nil)", "(nil)", "OK", "(nil)", "(nil)", "(nil)"},
 		},
+		{


Even though it returns nil, the key is still set in the background. You return nil since there was no existing value, however, the new value will still be set.

Add the validation regardless.

JyotinderSingh · 2024-09-02T15:12:47Z

core/eval.go

+			 } else if nxOption {
+				//store.Put(key, store.NewObj(value, exDurationMs, oType, oEnc))
+				if obj == nil {
+					return Encode(obj.Value, false)


Shouldn't this return RespNIL?
If key k doesn't exit and you fire SET k v NX GET the result will be (nil).

redis> SET k v NX GET (nil)

Also, add a test for this once fixed.

JyotinderSingh · 2024-09-02T15:13:24Z

core/eval.go

+					return Encode(obj.Value, false)
+				}
+
+				return RespNIL


If the object is not nil, you should return obj.Value.

redis> SET k v1 "OK" redis> SET k v2 NX GET "v1"

JyotinderSingh · 2024-09-06T04:56:21Z

Please rebase the PR, we just had a large change go in which might've introduced conflicts.

arpitbbhayani · 2024-09-08T12:47:09Z

@yaten2302 can you rebase this? we would like to close this as soon as possible. Do you need any help?

JyotinderSingh requested review from soumya-codes and JyotinderSingh August 15, 2024 14:27

JyotinderSingh requested changes Aug 15, 2024

View reviewed changes

lucifercr07 reviewed Aug 16, 2024

View reviewed changes

JyotinderSingh requested changes Aug 19, 2024

View reviewed changes

yaten2302 mentioned this pull request Aug 20, 2024

Make SET command use the appropriate datatype when saving data in store #359

Closed

JyotinderSingh removed the request for review from soumya-codes September 2, 2024 15:04

JyotinderSingh requested changes Sep 2, 2024

View reviewed changes

yaten2302 closed this Sep 9, 2024

yaten2302 force-pushed the master branch from 6c0b72c to 7ec2ad0 Compare September 9, 2024 11:51

JyotinderSingh mentioned this pull request Sep 25, 2024

Add support for GET in SET command #207

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for GET option in SET command #322

Added support for GET option in SET command #322

yaten2302 commented Aug 15, 2024 •

edited by JyotinderSingh

Loading

JyotinderSingh left a comment

JyotinderSingh Aug 15, 2024

JyotinderSingh Aug 15, 2024

yaten2302 Aug 16, 2024

JyotinderSingh Aug 19, 2024

yaten2302 Aug 20, 2024

JyotinderSingh Aug 15, 2024

yaten2302 Aug 16, 2024

yaten2302 Aug 17, 2024

JyotinderSingh Aug 19, 2024

lucifercr07 left a comment

AshwinKul28 commented Aug 16, 2024

yaten2302 commented Aug 16, 2024 •

edited

Loading

lucifercr07 commented Aug 16, 2024

yaten2302 commented Aug 16, 2024

lucifercr07 commented Aug 16, 2024 •

edited

Loading

yaten2302 commented Aug 17, 2024

JyotinderSingh commented Aug 17, 2024

JyotinderSingh Aug 19, 2024

JyotinderSingh Aug 19, 2024

JyotinderSingh Sep 2, 2024

JyotinderSingh Sep 2, 2024

JyotinderSingh commented Sep 6, 2024

arpitbbhayani commented Sep 8, 2024

Added support for GET option in SET command #322

Added support for GET option in SET command #322

Conversation

yaten2302 commented Aug 15, 2024 • edited by JyotinderSingh Loading

JyotinderSingh 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

lucifercr07 left a comment

Choose a reason for hiding this comment

AshwinKul28 commented Aug 16, 2024

yaten2302 commented Aug 16, 2024 • edited Loading

lucifercr07 commented Aug 16, 2024

yaten2302 commented Aug 16, 2024

lucifercr07 commented Aug 16, 2024 • edited Loading

yaten2302 commented Aug 17, 2024

JyotinderSingh commented Aug 17, 2024

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

JyotinderSingh commented Sep 6, 2024

arpitbbhayani commented Sep 8, 2024

yaten2302 commented Aug 15, 2024 •

edited by JyotinderSingh

Loading

yaten2302 commented Aug 16, 2024 •

edited

Loading

lucifercr07 commented Aug 16, 2024 •

edited

Loading