-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BITFIELD and BITFIELD_RO feature #2107
Open
slorello89
wants to merge
18
commits into
main
Choose a base branch
from
feature/bitfield
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
68d6d79
WIP
slorello89 1695639
Bitfield feature
slorello89 125885a
Merge branch 'main' into feature/bitfield
slorello89 5660713
accurate reporting of num args for incr
slorello89 bdb9128
removing unused parsing methods
slorello89 7554d6d
some formatting comment updates
slorello89 5daca75
merge
slorello89 c0fe65d
moving enums
slorello89 3928113
using new remarks pattern nick introduced
slorello89 b3c16c4
properly checking feature flag
slorello89 d3cc041
decreasing public-api surface area, using a builder.
slorello89 95eb393
Merge remote-tracking branch 'origin/main' into feature/bitfield
NickCraver 9dae591
Fix spacing
NickCraver 7c6f62f
Simplify a bit, update docs
NickCraver 5f07290
Save all the files dammit
NickCraver d87f66b
Merge branch 'main' into feature/bitfield
slorello89 1392d20
api updates per marcs comments
slorello89 a00aba5
encoding -> string
slorello89 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
|
||
namespace StackExchange.Redis; | ||
|
||
/// <summary> | ||
/// An abstract subcommand for a bitfield. | ||
/// </summary> | ||
public abstract class BitfieldSubCommand | ||
{ | ||
internal abstract int NumArgs { get; } | ||
|
||
internal abstract void AddArgs(IList<RedisValue> args); | ||
|
||
internal virtual bool IsReadonly => false; | ||
|
||
/// <summary> | ||
/// The encoding of the sub-command. A signed or unsigned integer of a given size. | ||
/// </summary> | ||
public BitfieldEncoding Encoding { get; } | ||
|
||
/// <summary> | ||
/// The offset into the bitfield the subcommand will traverse. | ||
/// </summary> | ||
public BitfieldOffset Offset { get; } | ||
|
||
internal BitfieldSubCommand(BitfieldEncoding encoding, BitfieldOffset offset) | ||
{ | ||
Encoding = encoding; | ||
Offset = offset; | ||
} | ||
|
||
} | ||
|
||
/// <summary> | ||
/// Represents a Bitfield GET, which returns the number stored in the specified offset of a bitfield at the given encoding. | ||
/// </summary> | ||
public sealed class BitfieldGet : BitfieldSubCommand | ||
{ | ||
/// <summary> | ||
/// Initializes a bitfield GET subcommand | ||
/// </summary> | ||
/// <param name="encoding">The encoding of the subcommand.</param> | ||
/// <param name="offset">The offset into the bitfield of the subcommand</param> | ||
public BitfieldGet(BitfieldEncoding encoding, BitfieldOffset offset) : base(encoding, offset) | ||
{ | ||
} | ||
|
||
internal override bool IsReadonly => true; | ||
|
||
internal override int NumArgs => 3; | ||
|
||
internal override void AddArgs(IList<RedisValue> args) | ||
{ | ||
args.Add(RedisLiterals.GET); | ||
args.Add(Encoding.AsRedisValue); | ||
args.Add(Offset.AsRedisValue); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Bitfield subcommand which SETs the specified range of bits to the specified value. | ||
/// </summary> | ||
public sealed class BitfieldSet : BitfieldSubCommand | ||
{ | ||
/// <summary> | ||
/// The value to set. | ||
/// </summary> | ||
public long Value { get; } | ||
|
||
/// <summary> | ||
/// Initializes a subcommand for a Bitfield SET. | ||
/// </summary> | ||
/// <param name="encoding">The number's encoding.</param> | ||
/// <param name="offset">The offset into the bitfield to set.</param> | ||
/// <param name="value">The value to set.</param> | ||
public BitfieldSet(BitfieldEncoding encoding, BitfieldOffset offset, long value) : base(encoding, offset) | ||
{ | ||
Value = value; | ||
} | ||
|
||
internal override int NumArgs => 4; | ||
|
||
internal override void AddArgs(IList<RedisValue> args) | ||
{ | ||
args.Add(RedisLiterals.SET); | ||
args.Add(Encoding.AsRedisValue); | ||
args.Add(Offset.AsRedisValue); | ||
args.Add(Value); | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// Bitfield subcommand INCRBY, which increments the number at the specified range of bits by the provided value | ||
/// </summary> | ||
public sealed class BitfieldIncrby : BitfieldSubCommand | ||
{ | ||
/// <summary> | ||
/// The value to increment by. | ||
/// </summary> | ||
public long Increment { get; } | ||
|
||
/// <summary> | ||
/// Determines how overflows are handled for the bitfield. | ||
/// </summary> | ||
public BitfieldOverflowHandling OverflowHandling { get; } | ||
|
||
/// <summary> | ||
/// Initializes a sub-command for a Bitfield INCRBY. | ||
/// </summary> | ||
/// <param name="encoding">The number's encoding.</param> | ||
/// <param name="offset">The offset into the bitfield to set.</param> | ||
/// <param name="increment">The value to set.</param> | ||
/// <param name="overflowHandling">How overflows will be handled when incrementing.</param> | ||
public BitfieldIncrby(BitfieldEncoding encoding, BitfieldOffset offset, long increment, BitfieldOverflowHandling overflowHandling = BitfieldOverflowHandling.Wrap) : base(encoding, offset) | ||
{ | ||
Increment = increment; | ||
OverflowHandling = overflowHandling; | ||
} | ||
|
||
internal override int NumArgs => OverflowHandling == BitfieldOverflowHandling.Wrap ? 4 : 6; | ||
|
||
internal override void AddArgs(IList<RedisValue> args) | ||
{ | ||
if (OverflowHandling != BitfieldOverflowHandling.Wrap) | ||
{ | ||
args.Add(RedisLiterals.OVERFLOW); | ||
args.Add(OverflowHandling.AsRedisValue()); | ||
} | ||
args.Add(RedisLiterals.INCRBY); | ||
args.Add(Encoding.AsRedisValue); | ||
args.Add(Offset.AsRedisValue); | ||
args.Add(Increment); | ||
} | ||
} | ||
|
||
|
||
|
||
/// <summary> | ||
/// An offset into a bitfield. This is either a literal offset (number of bits from the beginning of the bitfield) or an | ||
/// encoding based offset, based off the encoding of the sub-command. | ||
/// </summary> | ||
public readonly struct BitfieldOffset | ||
{ | ||
/// <summary> | ||
/// Returns the BitfieldOffset as a RedisValue | ||
/// </summary> | ||
internal RedisValue AsRedisValue => $"{(ByEncoding ? "#" : string.Empty)}{Offset}"; | ||
|
||
/// <summary> | ||
/// Whether or not the BitfieldOffset will work off of the sub-commands integer encoding. | ||
/// </summary> | ||
public bool ByEncoding { get; } | ||
|
||
/// <summary> | ||
/// The number of either bits or encoded integers to offset into the bitfield. | ||
/// </summary> | ||
public long Offset { get; } | ||
|
||
/// <summary> | ||
/// Initializes a bitfield offset | ||
/// </summary> | ||
/// <param name="byEncoding">Whether or not the BitfieldOffset will work off of the sub-commands integer encoding.</param> | ||
/// <param name="offset">The number of either bits or encoded integers to offset into the bitfield.</param> | ||
public BitfieldOffset(bool byEncoding, long offset) | ||
{ | ||
ByEncoding = byEncoding; | ||
Offset = offset; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// The encoding that a sub-command should use. This is either a signed or unsigned integer of a specified length. | ||
/// </summary> | ||
public readonly struct BitfieldEncoding | ||
{ | ||
internal RedisValue AsRedisValue => $"{Signedness.SignChar()}{Size}"; | ||
|
||
/// <summary> | ||
/// The signedness of the integer. | ||
/// </summary> | ||
public Signedness Signedness { get; } | ||
|
||
/// <summary> | ||
/// The size of the integer. | ||
/// </summary> | ||
public byte Size { get; } | ||
|
||
/// <summary> | ||
/// Initializes the BitfieldEncoding. | ||
/// </summary> | ||
/// <param name="signedness">The encoding's <see cref="Signedness"/></param> | ||
/// <param name="size">The size of the integer.</param> | ||
public BitfieldEncoding(Signedness signedness, byte size) | ||
{ | ||
Signedness = signedness; | ||
Size = size; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
using System; | ||
|
||
namespace StackExchange.Redis; | ||
|
||
/// <summary> | ||
/// Defines the overflow behavior of a BITFIELD command. | ||
/// </summary> | ||
public enum BitfieldOverflowHandling | ||
{ | ||
/// <summary> | ||
/// Wraps around to the most negative value of signed integers, or zero for unsigned integers | ||
/// </summary> | ||
Wrap, | ||
/// <summary> | ||
/// Uses saturation arithmetic, stopping at the highest possible value for overflows, and the lowest possible value for underflows. | ||
/// </summary> | ||
Saturate, | ||
/// <summary> | ||
/// If an overflow is encountered, associated subcommand fails, and the result will be NULL. | ||
/// </summary> | ||
Fail | ||
} | ||
|
||
internal static class BitfieldOverflowHandlingExtensions | ||
{ | ||
internal static RedisValue AsRedisValue(this BitfieldOverflowHandling handling) => handling switch | ||
{ | ||
BitfieldOverflowHandling.Fail => RedisLiterals.FAIL, | ||
BitfieldOverflowHandling.Saturate => RedisLiterals.SAT, | ||
BitfieldOverflowHandling.Wrap => RedisLiterals.WRAP, | ||
_ => throw new ArgumentOutOfRangeException(nameof(handling)) | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
using System; | ||
|
||
namespace StackExchange.Redis; | ||
|
||
/// <summary> | ||
/// Represents an integers signedness | ||
/// </summary> | ||
public enum Signedness | ||
{ | ||
/// <summary> | ||
/// An integer with no sign bit. | ||
/// </summary> | ||
Unsigned, | ||
/// <summary> | ||
/// An integer with a sign bit. | ||
/// </summary> | ||
Signed | ||
} | ||
|
||
internal static class SignednessExtensions | ||
{ | ||
internal static char SignChar(this Signedness sign) => sign switch | ||
{ | ||
Signedness.Signed => 'i', | ||
Signedness.Unsigned => 'u', | ||
_ => throw new ArgumentOutOfRangeException(nameof(sign)) | ||
}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the approach is a bit heavy here - we're doing a lot of class creation on what seems like should be structs local to the command generation itself and not public API surface area. I'm happy to yank this local and try other approaches - will try and play some tonight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested to see what you come up with, this seemed liked best way to me. It's a weird command because of how variadic and structured it is. There's other commands where you just pass in arrays of literals and you can work everything out from that, but in this case you're really expecting things in a very specific order. Kind of tricky to design an API around, might be why it's been around for 6 years without making it into the library. I will tell you though I give a talk on bit-operations and the inability to use bitfield natively has been a real bummer 😆 so I have a bit of skin in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking is: these are Get*Message methods in RedisDatabase like others - they are just specific overloads. I don't know why we'd need classes on these (maybe should change the implementation on the other PR already in as well). This approach allocates another object to make the message as well as exposes them on the public API but as far as I can tell they don't need to be public. I think we can simplify these subcommands to some of those methods and remove the extra classes/allocations here overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe for the individual commands yes, but the command itself is weirdly variadic (you can execute an arbitrary number of subcommand, it's almost like a script), so you need some structure to maintain them all in, which is the thinking behind having them all broken out into different classes with an abstract driver. The alternative would be to just expose the single-commands and just let folks batch/pipeline them, but it sort of breaks the command's API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I poked at this for a bit and then git just royally screwed me with a UI change recently on main vs. current branch merging in my tool and lost everything so I'm hands off tonight. Basically I had BitfieldSubCommand -> IBitfieldSubCommand, internal members on that interface still, explicitly implemented on members, and all the API changes that entails. However, I'm not sure this is a good path or not - I don't like the amount of allocations we're doing for what is intended to be, on the Redis side, a very efficient/optimized op. Generally our audiences who want this functionality will also care about the cost (I know I would).
Overall, this one may wait, would like to talk it over with @mgravell on surface area and I'm supposed to be out this week. Given git giving me signs, gonna step away for now :)