Add unsigned 64-bit numeric config in module API by artikell · Pull Request #1546 · valkey-io/valkey

artikell · 2025-01-12T04:03:24Z

Supports VALKEYMODULE_CONFIG_UNSIGNED and UNSIGNED_CONFIG for a wider range of configurations.
Supported the API for ValkeyModule_RegisterUnsignedNumericConfig
string2ull method supports the length parameter, making it more secure

Signed-off-by: artikell <739609084@qq.com>

codecov · 2025-01-12T04:17:21Z

Codecov Report

❌ Patch coverage is 14.89362% with 40 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.92%. Comparing base (fc217fc) to head (8a777ed).
⚠️ Report is 16 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/config.c	23.07%	20 Missing ⚠️
src/module.c	4.76%	20 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1546      +/-   ##
============================================
- Coverage     75.05%   74.92%   -0.14%     
============================================
  Files           129      129              
  Lines         71553    71677     +124     
============================================
- Hits          53705    53702       -3     
- Misses        17848    17975     +127

Files with missing lines	Coverage Δ
src/server.h	`100.00% <ø> (ø)`
src/config.c	`77.63% <23.07%> (-0.93%)`	⬇️
src/module.c	`26.38% <4.76%> (-0.08%)`	⬇️

... and 27 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.

Signed-off-by: artikell <739609084@qq.com>

Signed-off-by: skyfirelee <739609084@qq.com>

madolson · 2025-01-14T17:18:01Z

Is there a concrete need for this, or are you just looking to make the API more usable by extending the use cases? We had discussed previously in Redis, and had decided to avoid having a large number of APIs, we would only support signed until someone indicated they clearly needed a 64 bit unsigned value. You can always restrict the value to be positive and cast it to unsigned (but just miss some of the high values).

artikell · 2025-01-17T14:26:12Z

Is there a concrete need for this, or are you just looking to make the API more usable by extending the use cases? We had discussed previously in Redis, and had decided to avoid having a large number of APIs, we would only support signed until someone indicated they clearly needed a 64 bit unsigned value. You can always restrict the value to be positive and cast it to unsigned (but just miss some of the high values).

Yes, we encountered some scenarios.

In some designs, uint64 is used as the ID, and the higher bits are used to store some information.
Logically, use creatULongLongConfig to manage configurations.

However, for the creatULongLongConfig method, it can only exceed ULLONG_MAX in the MEMORY_CONFIG configuration item. so, I think we need a configuration to support a larger range.

madolson · 2025-01-23T05:53:02Z

In some designs, uint64 is used as the ID, and the higher bits are used to store some information.
Logically, use creatULongLongConfig to manage configurations.

Hmm, interesting. OK, that makes sense to me.

artikell · 2025-02-27T01:48:42Z

Do you have any suggestions regarding this PR? Maybe you can help review it? Thank you. @madolson

madolson · 2025-02-27T02:51:24Z

Hey yes! Sorry, it wasn't on my list for 8.1, I'm just now catching up on other items. Will prioritize it tomorrow.

madolson

Mostly looks good to me, just some comments about structure and if we can simplify the new API.

madolson · 2025-02-27T19:44:59Z

    result = ValkeyModule_RegisterNumericConfig(ctx, "numeric", -1, VALKEYMODULE_CONFIG_DEFAULT, -5, 2000, getNumericConfigCommand, setNumericConfigCommand, longlongApplyFunc, &longval);
    response_ok |= (result == VALKEYMODULE_OK);
+
+    result = ValkeyModule_RegisterUnsignedNumericConfig(ctx, "unsigned_numeric", 1, VALKEYMODULE_CONFIG_DEFAULT | VALKEYMODULE_CONFIG_UNSIGNED, 0, LLONG_MAX * 1ULL + 1000, getUnsignedNumericConfigCommand, setUnsignedNumericConfigCommand, unsignedlonglongApplyFunc, &mutable_ull_val);


I'm not sure why we need both the API and the new enum value. Can't RegisterUnsignedNumericConfig imply VALKEYMODULE_CONFIG_UNSIGNED ? We need the new API anyways because there are typed differences.

That can be implemented, but it will introduce some difficult to understand issues because the parameters of the ValkeyModule_RegisterNumericConfig method are all of type long long. For example, min and max values.

What's your opinion?

madolson · 2025-02-27T19:45:34Z

@@ -1,9 +1,12 @@
 #include "valkeymodule.h"
 #include <strings.h>
+#include <limits.h>


Is this used?

Yes, the LLONG_MAX constant was used

madolson · 2025-02-27T19:58:05Z

+        unsigned long long ull;
+        int ok = string2ull(value, sdslen(value), &ull);
+        if (ok) {
+            *res = (long long)ull;


This feels weird, we're casting this to a ll and then casting it back. I originally thought this truncated the top bit, but apparently it preserves the bit pattern. I still think we maybe should have a dedicated function for handling unsigned numerics here.

This is a reference to the implementation of memtoull in the MEMORY_CONFIG configuration.
Do you mean a similar method needs to be implemented to cast the operation? Or add a numericParseString method for unsigned types?

I think this is fine, but let's add some code comments about it. When parsing an unsigned number, although it's returned as a signed number, the caller needs to interpret it as an unsigned long long.

This is what getModuleUnsignedNumericConfig and other functions do.

Signed-off-by: artikell <739609084@qq.com>

artikell · 2025-03-02T01:35:38Z

Mostly looks good to me, just some comments about structure and if we can simplify the new API.

In fact, unsigned longlong is special because its numerical range is the largest. For other types of data, it is not necessary to use an unsigned type, just control the data range.

Add an API that can also defend against a large number of type casts in encoding, avoiding various errors caused by them.

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

- Fix release date to Mon Mar 18 2026 - Fix typos: duplicate 'load', 'keyes' -> 'keys', duplicate 'INFO' - Remove reverted contributor (arshidkv12, valkey-io#3137) - Add 7 new release-notes entries from upstream/unstable merge: CLUSTERSCAN (valkey-io#2934), MSETEX (valkey-io#3121), availability-zone (valkey-io#3156), stream range optimization (valkey-io#3002), RDB as AOF preamble (valkey-io#1901), unsigned 64-bit module config (valkey-io#1546), fast_float -> ffc (valkey-io#3329) Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

- Supports `VALKEYMODULE_CONFIG_UNSIGNED` and `UNSIGNED_CONFIG` for a wider range of configurations. - Supported the API for `ValkeyModule_RegisterUnsignedNumericConfig` - `string2ull` method supports the length parameter, making it more secure --------- Signed-off-by: artikell <739609084@qq.com> Signed-off-by: skyfirelee <739609084@qq.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

Support unsigned numeric config

2c0d54e

Signed-off-by: artikell <739609084@qq.com>

format code

6156c1c

Signed-off-by: artikell <739609084@qq.com>

artikell force-pushed the expand_num_range branch from 9787901 to 6156c1c Compare January 12, 2025 06:45

format code

87709c2

Signed-off-by: skyfirelee <739609084@qq.com>

madolson reviewed Feb 27, 2025

View reviewed changes

Remove unnecessary logic

5124b35

Signed-off-by: artikell <739609084@qq.com>

artikell force-pushed the expand_num_range branch from 3fe194c to 5124b35 Compare February 28, 2025 01:47

artikell requested a review from madolson March 3, 2025 03:27

madolson added this to Valkey 9.1 Sep 5, 2025

madolson added enhancement New feature or request major-decision-pending Major decision pending by TSC team labels Sep 5, 2025

madolson moved this to Todo in Valkey 9.1 Sep 5, 2025

madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Oct 27, 2025

madolson moved this from Todo to Needs Review in Valkey 9.1 Feb 2, 2026

zuiderkwast approved these changes Mar 5, 2026

View reviewed changes

Merge branch 'unstable' into expand_num_range

8a777ed

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

zuiderkwast changed the title ~~Support unsigned numeric config~~ Add unsigned 64-bit numeric config in module API Mar 11, 2026

zuiderkwast added the release-notes This issue should get a line item in the release notes label Mar 11, 2026

zuiderkwast merged commit 7403b42 into valkey-io:unstable Mar 11, 2026
92 checks passed

github-project-automation Bot moved this from Needs Review to Done in Valkey 9.1 Mar 11, 2026

Uh oh!

Conversation

artikell commented Jan 12, 2025

Uh oh!

codecov Bot commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

madolson commented Jan 14, 2025

Uh oh!

artikell commented Jan 17, 2025

Uh oh!

madolson commented Jan 23, 2025

Uh oh!

artikell commented Feb 27, 2025

Uh oh!

madolson commented Feb 27, 2025

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

madolson Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

artikell Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

madolson Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

artikell Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

madolson Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

artikell Feb 28, 2025

Choose a reason for hiding this comment

Uh oh!

zuiderkwast Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artikell commented Mar 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 12, 2025 •

edited

Loading