Adds HGETDEL Support to Valkey

[roshkhatri]

Fixes #2850.

Adds support for HGETDEL to Valkey and aligns with Redis 8.0 feature.
Maintains syntax compatibility
Retrieves all the values, and null if fields dont exists and deletes once retrieved.

127.0.0.1:6379> HGETDEL key FIELDS numfields field [ field ... ]

127.0.0.1:6379> HSET foo field1 bar1 field2 bar2 field3 bar3
(integer) 3
127.0.0.1:6379> HGETDEL foo FIELDS 1 field2
1) "bar2"
127.0.0.1:6379> HGETDEL foo FIELDS 1 field2
1) (nil)
127.0.0.1:6379> HGETALL foo
1) "field1"
2) "bar1"
3) "field3"
4) "bar3"
127.0.0.1:6379>  HGETDEL foo FIELDS 2 field2 field3
1) (nil)
2) "bar3"
127.0.0.1:6379> HGETALL foo
1) "field1"
2) "bar1"
127.0.0.1:6379> HGETDEL foo FIELDS 3 field1 non-exist-field
(error) ERR syntax error
127.0.0.1:6379> HGETDEL foo FIELDS 2 field1 non-exist-field
1) "bar1"
2) (nil)
127.0.0.1:6379> HGETALL foo
(empty array)
127.0.0.1:6379> 

[codecov]

Codecov Report

❌ Patch coverage is 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.41%. Comparing base (9562bdc) to head (809de8d).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/t_hash.c	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #2851   +/-   ##
=========================================
  Coverage     72.41%   72.41%           
=========================================
  Files           128      128           
  Lines         70439    70469   +30     
=========================================
+ Hits          51006    51029   +23     
- Misses        19433    19440    +7     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/server.h	100.00% <ø> (ø)
src/t_hash.c	96.25% <96.66%> (+0.09%)	⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[hwware]

@valkey-io/core-team I would like to add this pr to next meeting agenda to discuss more, Thanks

[hwware]

I think it is one feature to align with Redis 8

[zuiderkwast]

If it's the same syntax as in Redis 8, I vote YES.

[roshkhatri]

If it's the same syntax as in Redis 8, I vote YES.

It is not, but I can make it the same

[zuiderkwast]

Ah... Redis insist on using this very ugly syntax for all new commands. It appears to be designed for machine generated command lines, not humans.

HGETDEL key FIELDS numfields field [field ...]

I think your syntax HGETDEL key field [field ...] is much nicer. I don't know if Redis compatibility is important or not. Maybe it's best to discuss it in a meeting then.

@valkey-io/client-maintainers WDYT?

[roshkhatri]

Yeah, it is the same as HTTL

HTTL key FIELDS numfields field [ field ... ]

https://valkey.io/commands/httl/

The one I implemented is similar to HMGET: HMGET key field [ field ... ]

I think we have similar syntax in valkey as well so it would be generally better to keep compatibility

[JimB123]

I like consistency with HMGET better than creating a weird syntax just to align with Redis.

That said, IF the 3rd parameter is a number AND the number equals the number of remaining parameters, we could just silently ignore it. But, unfortunately, that becomes ambiguous in a case like this: HGETDEL key 3 4 5 6 - is "3" a field? or a count of fields?

[hpatro]

Regarding the syntax, users migrating from Redis to Valkey will have problems if we don't choose to stick with it. We have decided to stick through with so many of features so far (Hash field expiration / Search / JSON). So, I think we should continue have symmetry with them atleast on the data path commands which were introduced there first.

[nihohit]

Breaking API with Redis will mean that client maintainers might need to choose sides, and only fully support one DB, and offer partial support for the other, which will also lead to poor UX for the client users, since these details are easy to miss, and expectations won't match reality.
Unless there's a clear benefit that's worth the potential harm, I would prefer for valkey to be consistent with Redis where possible (and the other way around too, of course).

[roshkhatri]

Updated the top comment and this PR now maintain syntax compatibility.

[hwware]

I have one question: In Redis 8.0, it introduces this command HGETDEL, the open source license was changed before Redis 8.0. I am not sure how many users migrating from Redis 8.0 to Valkey? Do we need still align with the Redis weird syntax?

Honestly, I do not like the format: HGETDEL key FIELDS numfields field [field ...]

[ranshid]

Gave a quick one (and will continue tomorrow)
Looks good overall. I would be happy if we can also include a test in hashexpire.tcl to test the command when there are expired fields.

[ranshid]

I have one question: In Redis 8.0, it introduces this command HGETDEL, the open source license was changed before Redis 8.0. I am not sure how many users migrating from Redis 8.0 to Valkey? Do we need still align with the Redis weird syntax?

Honestly, I do not like the format: HGETDEL key FIELDS numfields field [field ...]

I also do not like the FIELDS (although it has advantages for adding future command arguments), but I agree with the state that we should be more aligned with the client ecosystem needs and I think it is overriding personal taste ATM.

[rueian]

As a client maintainer, I suggest aligning with the weird syntax as long as the command name remains the same.

[yangbodong22011]

I vote for use Redis syntax. (for save time and compatibility, Redis client for Valkey, Valkey client for Redis)

P.S. Actually, client can support both two syntax use two api.

[ranshid]

@valkey-io/core-team Let me summarize the decision points:

• Should we support this new API? this functionality is already available since Valkey 9.0 (eg HGETEX myhash EX 0 FIELDS ....)
• Should we duplicate the Redis API? The majority of comments here suggest we do. This will probably make it easier for client libraries to support it ifor both Redis and Valkey
• Order regarding command complexity. When we introduced HFE we decided to set the complexity of variadic commands.
  I think O(N) sounds better but in that case we need to take an action item to align other commands (can/should be in a separate PR)
• keyspace notifications: Currently the decision is to produce keyspace evenets ONLY when there are actually mutations oin the objects. we will trigger a SINGLE hdel event in case we deleted at least 1 field and del event in case the object was deleted after all fields deleted.

[hwware]

If it's the same syntax as in Redis 8, I vote YES.

It is not, but I can make it the same

Now the syntax is same as redis https://redis.io/docs/latest/commands/hgetdel/

[ranshid]

@valkey-io/core-team do you want to try and take a decision offline (on this PR) or should I schedule this to be a topic on Monday?
Seems to me it might not require an extensive talk so maybe we can push this by aligning offline?

[enjoy-binbin]

Also +1 for Wen's comments. (Yes, align with Redis syntax, O(n), single hdel event.)

[madolson]

1. Weekly meeting. This feature is probably OK since compatibility is useful and the cost for this feature is quite low. We would like to have better long term guidance on what features we implement from Redis though.

[ranshid]

LGTM