Bitmap command support #129

alexr-bq · 2025-10-31T22:20:17Z

Adds support for bitmap commands set out in #52

Signed-off-by: Alex Rehnby-Martin <[email protected]>

…alexr/bitmap

currantw · 2025-11-06T17:12:01Z

.github/workflows/tests.yml

        name: net${{ matrix.dotnet }}, server ${{ matrix.server.version }}, ${{ matrix.host.TARGET }}
        needs: get-matrices
-        timeout-minutes: 100
+        timeout-minutes: 200


Is this still necessary? If so, is there an issue to investigate and try to sped these up?

currantw · 2025-11-06T17:12:34Z

README.md

+| Engine Type           |  6.2  |  7.0  |   7.1  |  7.2  |  8.0  |  8.1  |  9.0  |
+|-----------------------|-------|-------|--------|-------|-------|-------|-------|
+| Valkey                |   -   |   -   |   -    |   V   |   V   |   V   |   V   |
+| Redis                 |   V   |   V   |   V    |   V   |   -   |   -   |   -   |


Thanks for making this update!

currantw · 2025-11-06T17:37:04Z

sources/Valkey.Glide/Commands/IBitmapCommands.cs

+namespace Valkey.Glide.Commands;
+
+/// <summary>
+/// Supports commands for the "Bitmap Commands" group for standalone and cluster clients.


Nit. Maybe a bit simpler?

Suggested change

/// Supports commands for the "Bitmap Commands" group for standalone and cluster clients.

/// Supports bitmap commands for standalone and cluster clients.

currantw · 2025-11-06T17:43:48Z

sources/Valkey.Glide/Commands/IBitmapCommands.cs

+    /// <summary>
+    /// Sets or clears the bit at offset in the string value stored at key.
+    /// </summary>
+    /// <seealso href="https://valkey.io/commands/setbit"/>
+    /// <param name="key">The key of the string.</param>
+    /// <param name="offset">The offset in the string to set the bit at.</param>
+    /// <param name="value">The bit value to set (true for 1, false for 0).</param>
+    /// <param name="flags">The flags to use for this operation. Currently flags are ignored.</param>
+    /// <returns>The original bit value stored at offset.</returns>
+    /// <remarks>
+    /// <example>
+    /// <code>
+    /// bool oldBit = await client.StringSetBitAsync("mykey", 1, true);
+    /// Console.WriteLine(oldBit); // Output: false (original bit value)
+    /// </code>
+    /// </example>
+    /// </remarks>
+    Task<bool> StringSetBitAsync(ValkeyKey key, long offset, bool value, CommandFlags flags = CommandFlags.None);


Is it worth specifying the behaviour when the key doesn't exist or when the offset is bigger than the string? Or are you happy just deferring to the linked documentation? I think either approach work, but you seem to have different levels of detail/defferal in StringGetBitAsync compared to StringSetBitAsync?

currantw · 2025-11-06T17:58:57Z

sources/Valkey.Glide/Internals/Request.BitmapCommands.cs

+
+    public static Cmd<long, long> BitPositionAsync(ValkeyKey key, bool bit, long start = 0, long end = -1, StringIndexType indexType = StringIndexType.Byte)
+    {
+        List<GlideString> args = [key.ToGlideString(), (bit ? 1 : 0).ToGlideString(), start.ToGlideString(), end.ToGlideString()];


You've got this repeated in a few places. Is it worth adding a ToGlideString extension for bool so that we can directly do these conversions? I imagine that there are a few other places that could use that?

Suggested change

List<GlideString> args = [key.ToGlideString(), (bit ? 1 : 0).ToGlideString(), start.ToGlideString(), end.ToGlideString()];

List<GlideString> args = [key.ToGlideString(), bit.ToGlideString(), start.ToGlideString(), end.ToGlideString()];

currantw · 2025-11-06T18:41:27Z

sources/Valkey.Glide/BaseClient.BitmapCommands.cs

+    /// <inheritdoc/>
+    public async Task<bool> StringGetBitAsync(ValkeyKey key, long offset, CommandFlags flags = CommandFlags.None)
+    {
+        Utils.Requires<NotImplementedException>(flags == CommandFlags.None, "Command flags are not supported by GLIDE");


This line is duplicated so many times. Can we extract it to a BaseClient method like ThrowIfCommandFlags()? Also happy to raise a separate issue/separate PR in the interest of getting this merged 🤷 (If you can raise it, I'm happy to complete it!)

currantw · 2025-11-06T18:43:57Z

sources/Valkey.Glide/Internals/Request.BitmapCommands.cs

+    public static Cmd<long, bool> SetBitAsync(ValkeyKey key, long offset, bool value)
+        => new(RequestType.SetBit, [key.ToGlideString(), offset.ToGlideString(), (value ? 1 : 0).ToGlideString()], false, response => response != 0);
+
+    public static Cmd<long, long> BitCountAsync(ValkeyKey key, long start = 0, long end = -1, StringIndexType indexType = StringIndexType.Byte)


Does this (and other methods here) need default values? It seems like the methods that call into here already have defaults for all these values, so (a) will these values ever not be populated when this (and similar) methods are called, and (b) are we duplicating these defaults?

currantw · 2025-11-06T18:59:21Z

tests/Valkey.Glide.IntegrationTests/BitmapCommandTests.cs

+        // Test bit positions in 'A' (01000001)
+        bool bit0 = await client.StringGetBitAsync(key, 0); // Should be false (0)
+        bool bit1 = await client.StringGetBitAsync(key, 1); // Should be true (1)
+        bool bit2 = await client.StringGetBitAsync(key, 2); // Should be false (0)
+        bool bit6 = await client.StringGetBitAsync(key, 6); // Should be false (0)
+        bool bit7 = await client.StringGetBitAsync(key, 7); // Should be true (1)
+
+        Assert.False(bit0);
+        Assert.True(bit1);
+        Assert.False(bit2);
+        Assert.False(bit6);
+        Assert.True(bit7);


I noticed that SETBIT supports negative offsets. Does GETBIT do the same?

Also nit. Doesn't seem necessary to have these comments or store results instead of testing them directly? 🤷

Similar to other tests below. Just seems like more code to read, maintain, etc.?

Suggested change

// Test bit positions in 'A' (01000001)

bool bit0 = await client.StringGetBitAsync(key, 0); // Should be false (0)

bool bit1 = await client.StringGetBitAsync(key, 1); // Should be true (1)

bool bit2 = await client.StringGetBitAsync(key, 2); // Should be false (0)

bool bit6 = await client.StringGetBitAsync(key, 6); // Should be false (0)

bool bit7 = await client.StringGetBitAsync(key, 7); // Should be true (1)

Assert.False(bit0);

Assert.True(bit1);

Assert.False(bit2);

Assert.False(bit6);

Assert.True(bit7);

// Test bit positions in 'A' (01000001)

Assert.False(await client.StringGetBitAsync(key, 0));

Assert.True(await client.StringGetBitAsync(key, 1));

Assert.False(await client.StringGetBitAsync(key, 2));

Assert.False(await client.StringGetBitAsync(key, 6));

Assert.True(await client.StringGetBitAsync(key, 7));

currantw · 2025-11-06T19:02:45Z

tests/Valkey.Glide.IntegrationTests/BitmapCommandTests.cs

+    [Theory(DisableDiscoveryEnumeration = true)]
+    [MemberData(nameof(Config.TestClients), MemberType = typeof(TestConfiguration))]
+    public async Task BitField_AutoOptimization_UsesReadOnlyForGetOperations(BaseClient client)
+    {
+        string key = Guid.NewGuid().ToString();
+
+        // Set initial value "A" (ASCII 65 = 01000001)
+        await client.StringSetAsync(key, "A");
+
+        // Test with only GET operations - should automatically use BITFIELD_RO
+        var readOnlySubCommands = new BitFieldOptions.IBitFieldSubCommand[]
+        {
+            new BitFieldOptions.BitFieldGet(BitFieldOptions.Encoding.Unsigned(8), new BitFieldOptions.BitOffset(0)),
+            new BitFieldOptions.BitFieldGet(BitFieldOptions.Encoding.Unsigned(4), new BitFieldOptions.BitOffset(0)),
+            new BitFieldOptions.BitFieldGet(BitFieldOptions.Encoding.Unsigned(4), new BitFieldOptions.BitOffset(4))
+        };
+
+        // This should internally use BITFIELD_RO since all commands are GET
+        long[] results = await client.StringBitFieldAsync(key, readOnlySubCommands);
+
+        Assert.Equal(3, results.Length);
+        Assert.Equal(65, results[0]);  // Full 8 bits: 01000001 = 65
+        Assert.Equal(4, results[1]);   // First 4 bits: 0100 = 4
+        Assert.Equal(1, results[2]);   // Next 4 bits: 0001 = 1
+
+        // Test with mixed operations - should use regular BITFIELD
+        var mixedSubCommands = new BitFieldOptions.IBitFieldSubCommand[]
+        {
+            new BitFieldOptions.BitFieldGet(BitFieldOptions.Encoding.Unsigned(8), new BitFieldOptions.BitOffset(0)),
+            new BitFieldOptions.BitFieldSet(BitFieldOptions.Encoding.Unsigned(8), new BitFieldOptions.BitOffset(0), 100)
+        };
+
+        long[] mixedResults = await client.StringBitFieldAsync(key, mixedSubCommands);
+
+        Assert.Equal(2, mixedResults.Length);
+        Assert.Equal(65, mixedResults[0]); // Original value 'A'
+        Assert.Equal(65, mixedResults[1]); // Old value before SET
+    }


How is this test actually verifying that BITFIELD_RO is called internally?

… 7.0.0+ Signed-off-by: Alex Rehnby-Martin <[email protected]>

alexr-bq added 7 commits October 15, 2025 10:04

Add bitadd

b9f3999

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Add setbit

9b153f1

Signed-off-by: Alex Rehnby-Martin <[email protected]>

bitcount

1eb3450

Signed-off-by: Alex Rehnby-Martin <[email protected]>

bitop

41354dc

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Implement bitfield

7d791df

Signed-off-by: Alex Rehnby-Martin <[email protected]>

resolve merge conflict

1caa4b4

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Batch and unit tests

be58852

Signed-off-by: Alex Rehnby-Martin <[email protected]>

alexr-bq requested a review from a team as a code owner October 31, 2025 22:20

yipin-chen requested a review from jbrinkman November 3, 2025 16:58

alexr-bq added 3 commits November 3, 2025 15:56

Update version in readme

872b1d8

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Merge branch 'main' into alexr/bitmap

a86f508

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Format

8cb17a1

Signed-off-by: Alex Rehnby-Martin <[email protected]>

yipin-chen requested a review from currantw November 5, 2025 16:15

alexr-bq added 2 commits November 5, 2025 08:53

Attempt to set higher timeout for tests

56f47c0

Signed-off-by: Alex Rehnby-Martin <[email protected]>

Merge branch 'main' of github.com:valkey-io/valkey-glide-csharp into …

d4d71ec

…alexr/bitmap

currantw reviewed Nov 6, 2025

View reviewed changes

Add skip for tests of functionality only supported in server versions…

71bce95

… 7.0.0+ Signed-off-by: Alex Rehnby-Martin <[email protected]>

yipin-chen requested review from jeremyprime and removed request for jbrinkman November 7, 2025 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bitmap command support #129

Bitmap command support #129

alexr-bq commented Oct 31, 2025 •

edited

Loading

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

currantw Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	/// Supports commands for the "Bitmap Commands" group for standalone and cluster clients.
	/// Supports bitmap commands for standalone and cluster clients.

	List<GlideString> args = [key.ToGlideString(), (bit ? 1 : 0).ToGlideString(), start.ToGlideString(), end.ToGlideString()];
	List<GlideString> args = [key.ToGlideString(), bit.ToGlideString(), start.ToGlideString(), end.ToGlideString()];

Bitmap command support #129

Are you sure you want to change the base?

Bitmap command support #129

Conversation

alexr-bq commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexr-bq commented Oct 31, 2025 •

edited

Loading