Why bitField offset argument is int type?

[psw0946]

Feature Request

Is your feature request related to a problem? Please describe

I have an issue when I try to do bitfield with set subcommands.
But, when I pass offset argument with a long value, it emits an error with message,

Offset must be greater or equal to 0
java.lang.IllegalArgumentException: Offset must be greater or equal to 0
...

When I pass integer value offset, it succeeds.

I can pass a long value offset with redis-cli, but is there any reason for lettuce to declare offset with int type?

Describe the solution you'd like

BitFieldArgs Offset inner classes declare the offset field with int type. (BitFieldArgs.java)
Maybe, we can just change the offset type to long. But, we should limit the offset value to 512 MB.

[tishun]

Apologies for the late reply on this topic.

Could you please share the exact code that lead to this issue? A minimum reproducible example would be perfect!

I am not exactly sure which code path you are following and could not give a good answer whether this is a bug or not.

[psw0946]

Thanks for your reply.
It is easily shown that I can't make BitFieldArgs offset with the number over Integer.MAX_VALUE.
For example,

RedisCommands<String, String> redis;
redis.bitfield("TEST_KEY", BitFieldArgs.Builder.set(unsigned(1), Integer.MAX_VALUE, 1)); // success
redis.bitfield("TEST_KEY", BitFieldArgs.Builder.set(unsigned(1), 4294967295, 1)); // compile failed because 4294967295 is (1 << 32) - 1.

It cannot be compiled because the offset parameter (second paramter) type of BitFieldArgs.Builder.set is int. It supports up to ((1 << 31) - 1).
But, I think it should support up to ((1 << 32) - 1) as the documentation.

The offset argument is required to be greater than or equal to 0, and smaller than 2^32 (this limits bitmaps to 512MB).

[tishun]

Oh, I get the problem now, thank you for clarifying.

The problem is that ...

• ... the Lettuce API has offset defined with a primitive Java int which is signed integer and only holds 2^31 -1 values
• ... while the offset value the RESP API accepts is unsigned int that could hold 2^32 -1 values

I was confused by the mention of long being used for offset, which would not be correct, as long values are 2^63 - 1

Since Java 1.8 the Integer class is able to hold unsigned i32 values so I believe we need to change from int to Integer

[psw0946]

Yeah, that's right.
But, I have a worry when we use Integer instead of long.

Although the doc says that Integer class is able to hold unsigned int values, we cannot handle the number directly. If we wanna handle the number itself, we should calculate it with string or minus value.

Integer x = 4294967295; // cannot compile
Integer y = Integer.parseUnsignedInt("4294967295"); // y = -1
// We should calculate the number over Integer.MAX_VALUE with the minus value. 

I think it's more complicated and harder to use. Is it better to declare it as long, and validate it before processing?
Even Integer.parseUnsignedInt method uses long internally.
It's just my opinion, please feel free to give me feedbacks. Thank you 🙇

[tishun]

I assume that the offset is something you are handling as long when calculating it, correct?

There is multiple ways you can convert from long to unsigned int:

        int unsigned = Long.valueOf(4294967295L).intValue();
        int anotherUnsigned = (int) 4294967295L;
        // etc ...
        int maxInt = Integer.MAX_VALUE;

        System.out.println("value: " + Integer.toUnsignedString(unsigned) +
                " is " + (Integer.compareUnsigned(unsigned, maxInt) > 0 ? " greater" : " smaller") +
                " than " + maxInt);

Basically the int type is capable of holding 2^32 values already, it is a matter of how you interpret them.

My suggestion to keep the argument as int (or Integer) was because we would not have any validations.

Using long we would have to:

• check for negative numbers (as long is also signed)
• check for numbers above 4294967295L

But this is also an option. What do you think?

[tishun]

Also - another thing - if we use the int approach we would not have to alter the existing API, otherwise we would have to deprecate the existing methods and overload them with ones that use a long argument for offset.

[psw0946]

Ah, I understand it and I agree with you. Good explanation!
If it is not an urgent issue, and it is a good first issue for non-contributors to contribute for the first time, may I implement it?

[tishun]

Ah, I understand it and I agree with you. Good explanation! If it is not an urgent issue, and it is a good first issue for non-contributors to contribute for the first time, may I implement it?

It would be much appreciated!

Should be a fairly easy change - I would focus the main change on the part of the code that translates the int argument when building the final command that is sent to the server (I see that internally we are already using long). We should complement this with some JavaDoc for the use case of using the 2^32 unsigned integers and a simple integration test that verifies the correct usage of a large unsigned int, such as the one you provided.

You can start by extending one of the tests in the BitCommandIntegrationTests and add a scenario with an unsigned int and just see what is needed to make it work.

(make sure you call make start before running the test so the server is up, as this is an integration test)

Let me know if you need any more help and thanks for working on this!

[tishun]

Pushing this for 6.6.0 for now, as we want 6.5.1 released in December with some critical fixes