ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/garnet
File: libs/common/Generator.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;
using System.Security.Cryptography;

namespace Garnet.common
{
    /// <summary>
    /// Collection of methods generating hex ids
    /// </summary>
    public static class Generator
    {
        /// <summary>
        /// Random hex id
        /// </summary>
        /// <param name="size"></param>
        /// <returns></returns>
        public static string CreateHexId(int size = 40)
        {
            Span<byte> nodeIdBuffer = stackalloc byte[size];
            RandomNumberGenerator.Fill(nodeIdBuffer);
            return Convert.ToHexString(nodeIdBuffer).ToLowerInvariant();
        }

        /// <summary>
        /// Default hex id
        /// </summary>
        /// <param name="size"></param>
        /// <returns></returns>
        public static string DefaultHexId(int size = 40)
        {
            Span<byte> nodeIdBuffer = stackalloc byte[size];
            nodeIdBuffer.Clear();
            return Convert.ToHexString(nodeIdBuffer).ToLowerInvariant();
        }
    }
}

Review comment (suggestion):
Unfortunately, this is a breaking change. NodeId should be ASCII hex chars and RandomNumberGenerator.GetHexString is only available in net8.
Maybe an alternative:

            Span<byte> nodeIdBuffer = stackalloc byte[size];
            for (int i = 0; i < nodeIdBuffer.Length; i++)
                nodeIdBuffer[i] = hexchars[RandomNumberGenerator.GetInt32(int.MaxValue) & 0xf];
            return System.Text.Encoding.ASCII.GetString(nodeIdBuffer);

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;
using System.Security.Cryptography;

namespace Garnet.common
{
    /// <summary>
    /// Collection of methods generating hex ids
    /// </summary>
    public static class Generator
    {
        /// <summary>
        /// Generates a random hex string of specified length
        /// </summary>
        /// <param name="size">The length of the hex identifier string</param>
        /// <returns></returns>
        public static string CreateHexId(int size = 40)
        {
            Span<byte> nodeIdBuffer = stackalloc byte[size / 2];
            RandomNumberGenerator.Fill(nodeIdBuffer);
            return Convert.ToHexString(nodeIdBuffer).ToLowerInvariant();
        }

        /// <summary>
        /// Generates a default hex string of specified length (all zeros)
        /// </summary>
        /// <param name="size">The length of the hex identifier string</param>
        /// <returns></returns>
        public static string DefaultHexId(int size = 40)
        {
            Span<byte> nodeIdBuffer = stackalloc byte[size / 2];
            nodeIdBuffer.Clear();
            return Convert.ToHexString(nodeIdBuffer).ToLowerInvariant();
        }
    }
}
Repository: microsoft/garnet
File: libs/server/API/GarnetStatus.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

namespace Garnet.server
{
    /// <summary>
    /// Garnet API call return status
    /// </summary>
    public enum GarnetStatus : byte
    {
        /// <summary>
        /// OK
        /// </summary>
        OK,
        /// <summary>
        /// Not Found
        /// </summary>
        NOTFOUND,
        /// <summary>
        /// Moved
        /// </summary>
        MOVED,
        /// <summary>
        /// WRONGTYPE
        /// </summary>
        WRONGTYPE
    }
}

Review comment (bug):
Thanks for flagging this, this is a wider issue of bubbling errors up from the store, I've discussed this with the team and we've decided that we're going to tackle this in a wider change. If you're ok with it, please ignore wrong type issues in this PR and we'll fix it widely soon! 

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

namespace Garnet.server
{
    /// <summary>
    /// Garnet API call return status
    /// </summary>
    public enum GarnetStatus : byte
    {
        /// <summary>
        /// OK
        /// </summary>
        OK,
        /// <summary>
        /// Not Found
        /// </summary>
        NOTFOUND,
        /// <summary>
        /// Moved
        /// </summary>
        MOVED
    }
}
Repository: microsoft/garnet
File: test/Garnet.test/Extensions/TxnCustomCmd.cs (C#)

Code before:
            AddKey(mainStoreKey, LockType.Exclusive, false);

            var myDictKey = GetNextArg(ref procInput, ref offset);
            AddKey(myDictKey, LockType.Exclusive, true);

            return true;
        }

        public override void Main<TGarnetApi>(TGarnetApi api, ref CustomProcedureInput procInput, ref MemoryResult<byte> output)
        {
            var offset = 0;

            var mainStoreKey = GetNextArg(ref procInput, ref offset);
            var mainStoreValue = GetNextArg(ref procInput, ref offset);

            api.SET(mainStoreKey, mainStoreValue);

            var myDictKey = GetNextArg(ref procInput, ref offset);
            var myDictField = GetNextArg(ref procInput, ref offset);
            var myDictValue = GetNextArg(ref procInput, ref offset);

            var args = new ArgSlice[2];
            args[0] = myDictField;
            args[1] = myDictValue;

            ExecuteCustomCommand(api, "MYDICTSET", myDictKey, args, out var _output);

            WriteSimpleString(ref output, "OK");
        }
    }
}

Review comment (suggestion):
Looks like this will have to match and retrieve the command every single time main is called, which is on the critical path. Users should be able to, in Prepare or just the first time Main is called, get the actual ID so that the critical path call is very light.

Code after:
            _ = GetNextArg(ref procInput, ref offset); // mainStoreValue

            AddKey(mainStoreKey, LockType.Exclusive, false);

            var myDictKey = GetNextArg(ref procInput, ref offset);
            AddKey(myDictKey, LockType.Exclusive, true);

            if (!ParseCustomObjectCommand("MYDICTSET", out customObjectCommand))
                return false;

            return true;
        }

        public override void Main<TGarnetApi>(TGarnetApi api, ref CustomProcedureInput procInput, ref MemoryResult<byte> output)
        {
            var offset = 0;

            var mainStoreKey = GetNextArg(ref procInput, ref offset);
            var mainStoreValue = GetNextArg(ref procInput, ref offset);

            api.SET(mainStoreKey, mainStoreValue);

            var myDictKey = GetNextArg(ref procInput, ref offset);
            var myDictField = GetNextArg(ref procInput, ref offset);
            var myDictValue = GetNextArg(ref procInput, ref offset);

            var args = new ArgSlice[2];
            args[0] = myDictField;
            args[1] = myDictValue;

            ExecuteCustomObjectCommand(api, customObjectCommand, myDictKey, args, out var _output);

            WriteSimpleString(ref output, "OK");
        }
    }
}
Repository: microsoft/garnet
File: libs/server/Metrics/Latency/GarnetLatencyMetricsSession.cs (C#)

Code before:

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void Stop(LatencyMetricsType cmd)
        {
            int idx = (int)cmd;
            metrics[idx].RecordValue(Version);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void RecordValue(LatencyMetricsType cmd, long value)
        {
            int idx = (int)cmd;
            metrics[idx].RecordValue(Version, value);
        }

        public void ResetAll()
        {
            foreach (var cmd in defaultLatencyTypes)
                Reset(cmd);
        }

        public void Reset(LatencyMetricsType cmd)
        {
            int idx = (int)cmd;
            disposeLock.WriteLock();
            if(metrics != null)
            {
                metrics[idx].latency[PriorVersion].Reset();
            }
            disposeLock.WriteUnlock();
        }
    }
}

Review comment (suggestion):
Minor issue with formatting.
```suggestion
            if (metrics != null)
```

Code after:
            int old_idx = (int)oldCmd;
            int new_idx = (int)newCmd;
            metrics[new_idx].startTimestamp = metrics[old_idx].startTimestamp;
            metrics[old_idx].startTimestamp = 0;
            metrics[new_idx].RecordValue(Version);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void Stop(LatencyMetricsType cmd)
        {
            int idx = (int)cmd;
            metrics[idx].RecordValue(Version);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void RecordValue(LatencyMetricsType cmd, long value)
        {
            int idx = (int)cmd;
            metrics[idx].RecordValue(Version, value);
        }

        public void ResetAll()
        {
            foreach (var cmd in defaultLatencyTypes)
                Reset(cmd);
        }

        public void Reset(LatencyMetricsType cmd)
        {
            int idx = (int)cmd;
            try
            {
                disposeLock.WriteLock();
                if (metrics != null)
                {
                    metrics[idx].latency[PriorVersion].Reset();
                }
            }
            finally
            {
                disposeLock.WriteUnlock();
            }
        }
    }
}
Repository: microsoft/garnet
File: libs/server/Resp/ClientCommands.cs (C#)

Code before:
                        SendAndReset();
                    return true;
                }

                if (session.storeWrapper?.itemBroker is not null)
                {
                    var isBlocked = session.storeWrapper.itemBroker.TryGetObserver(session.ObjectStoreSessionID, out var observer);

                    if (!isBlocked)
                    {
                        while (!RespWriteUtils.WriteInteger(0, ref dcurr, dend))
                            SendAndReset();
                        return true;
                    }

                    if (toThrowError)
                    {
                        observer.CancellationTokenSource.Cancel();
                    }
                    else
                    {
                        observer.ResultFoundSemaphore.Release();
                    }
                }

                while (!RespWriteUtils.WriteInteger(1, ref dcurr, dend))
                    SendAndReset();
            }

            return true;
        }
    }
}

Review comment (suggestion):
The client may be unblocked between you checking if it's blocked and attempting to unblock it (in which case you should return 0). In other words, you should get some feedback from the observer that indicates if you initiated the unblocking or not.

Code after:
                    return true;
                }

                if (session.storeWrapper?.itemBroker is not null)
                {
                    var isBlocked = session.storeWrapper.itemBroker.TryGetObserver(session.ObjectStoreSessionID, out var observer);

                    if (!isBlocked)
                    {
                        while (!RespWriteUtils.TryWriteInt32(0, ref dcurr, dend))
                            SendAndReset();
                        return true;
                    }

                    var result = observer.TryForceUnblock(toThrowError);

                    while (!RespWriteUtils.TryWriteInt32(result ? 1 : 0, ref dcurr, dend))
                        SendAndReset();
                }
                else
                {
                    while (!RespWriteUtils.TryWriteInt32(0, ref dcurr, dend))
                        SendAndReset();
                }
            }
            else
            {
                while (!RespWriteUtils.TryWriteError(CmdStrings.RESP_ERR_UBLOCKING_CLINET, ref dcurr, dend))
                    SendAndReset();
            }

            return true;
        }
    }
}
Repository: microsoft/garnet
File: libs/server/Auth/Settings/ConnectionProtectionOption.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

namespace Garnet.server.Auth.Settings
{
    public enum ConnectionProtectionOption
    {
        Block = 0,
        AllowForLocalConnections = 1,
        AllowForAll = 2
    }

    public static class ConnectionProtectionOptionExtensions
    {
        public static ConnectionProtectionOption FromString(string s)
        {
            return s switch
            {
                "yes" or "all" => ConnectionProtectionOption.AllowForAll,
                "local" => ConnectionProtectionOption.AllowForLocalConnections,
                _ => ConnectionProtectionOption.Block
            };
        }
    }
}

Review comment (bug):
Why is this not called `DebugCommandOption` or something similar? This might be a bit confusing as to the purpose of this configuration. Could also use some XML comments to describe its function.

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

namespace Garnet.server.Auth.Settings
{
    /// <summary>
    /// Certain commands can be set at configuration to be limited to a particular connection type.
    /// They can be blocked for all connections, allowed only for local connections or be allowed for all connections.
    /// </summary>
    public enum ConnectionProtectionOption
    {
        /// <summary>
        /// Block for all connections.
        /// </summary>
        No = 0,
        /// <summary>
        ///  Allow only for local connections.
        /// </summary>
        Local = 1,
        /// <summary>
        /// Allow for every connection including remote connections.
        /// </summary>
        Yes = 2
    }
}
Repository: microsoft/garnet
File: libs/common/RespStrings.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;

namespace Garnet.common
{
    public class RespStrings
    {
        public static ReadOnlySpan<byte> EMPTYARRAY => "*0\r\n"u8;
        public static ReadOnlySpan<byte> EMPTYSET => "~0\r\n"u8;
        public static ReadOnlySpan<byte> EMPTYMAP => "%0\r\n"u8;
        public static ReadOnlySpan<byte> INFINITY_UPPERCASE => "INF"u8;
        public static ReadOnlySpan<byte> INTEGERZERO => ":0\r\n"u8;
        public static ReadOnlySpan<byte> INTEGERONE => ":1\r\n"u8;
        public static ReadOnlySpan<byte> RESP2_NULLARRAY => "*-1\r\n"u8;
        public static ReadOnlySpan<byte> RESP2_NULLBULK => "$-1\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_NULL => "_\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_FALSE => "#f\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_TRUE => "#t\r\n"u8;

        public static ReadOnlySpan<byte> VerbatimMarkdown => "mkd"u8;
        public static ReadOnlySpan<byte> VerbatimTxt => "txt"u8;
    }
}

Review comment (suggestion):
Please rename to `INFINITY`, the variable name suggests that it is uppercase (similar to `CmdStrings`)

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;

namespace Garnet.common
{
    public class RespStrings
    {
        public static ReadOnlySpan<byte> EMPTYARRAY => "*0\r\n"u8;
        public static ReadOnlySpan<byte> EMPTYSET => "~0\r\n"u8;
        public static ReadOnlySpan<byte> EMPTYMAP => "%0\r\n"u8;
        public static ReadOnlySpan<byte> INFINITY => "INF"u8;
        public static ReadOnlySpan<byte> POS_INFINITY => "+INF"u8;
        public static ReadOnlySpan<byte> NEG_INFINITY => "-INF"u8;
        public static ReadOnlySpan<byte> INTEGERZERO => ":0\r\n"u8;
        public static ReadOnlySpan<byte> INTEGERONE => ":1\r\n"u8;
        public static ReadOnlySpan<byte> RESP2_NULLARRAY => "*-1\r\n"u8;
        public static ReadOnlySpan<byte> RESP2_NULLBULK => "$-1\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_NULL => "_\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_FALSE => "#f\r\n"u8;
        public static ReadOnlySpan<byte> RESP3_TRUE => "#t\r\n"u8;

        public static ReadOnlySpan<byte> VerbatimMarkdown => "mkd"u8;
        public static ReadOnlySpan<byte> VerbatimTxt => "txt"u8;
    }
}
Repository: microsoft/garnet
File: libs/common/AsciiUtils.cs (C#)

Code before:
            Debug.Assert(b2 is >= 65 and <= 90);

            if (b1 == b2 || b1 - 32 == b2)
                continue;
            return false;
        }

        return true;
    }

    /// <summary>
    /// Check if two byte spans are equal, where right is an all-upper-case span, ignoring case if there are ASCII bytes.
    /// </summary>
    public static bool EqualsUpperCaseSpanIgnoringCase(this ReadOnlySpan<byte> left, ReadOnlySpan<byte> right, bool allowAllPrintableCharacters)
    {
        if (left.SequenceEqual(right))
            return true;
        if (left.Length != right.Length)
            return false;

        for (var i = 0; i < left.Length; i++)
        {
            var b1 = left[i];
            var b2 = right[i];

            if ((allowAllPrintableCharacters) && ((b2 is < 65 and >= 32) || (b2 is < 127 and > 96)))
            {
                if (b1 == b2)
                    continue;
                return false;
            }

            // Debug assert that b2 is an upper case letter 'A'-'Z'
            Debug.Assert(b2 is >= 65 and <= 90);

            if (b1 == b2 || b1 - 32 == b2)
                continue;
            return false;
        }

        return true;
    }
}

Review comment (bug):
Perhaps more compact / better perf?
Also we probably want to fail here (not just Debug.Assert) if allowAllPrintableCharacters == false and we encounter a character out of range.

```cs
if (!allowNonAlphabeticChars && b2 is < 65 or > 90)
    return false;

if (b1 == b2 || ((!allowNonAlphabeticChars || b2 is >= 65 and <= 90) && b1 - 32 == b2))
    continue;
```

Code after:
            Debug.Assert(b2 is >= 65 and <= 90);

            if (b1 == b2 || b1 - 32 == b2)
                continue;
            return false;
        }

        return true;
    }

    /// <summary>
    /// Check if two byte spans are equal, where right is an all-upper-case span, ignoring case if there are ASCII bytes.
    /// </summary>
    public static bool EqualsUpperCaseSpanIgnoringCase(this ReadOnlySpan<byte> left, ReadOnlySpan<byte> right, bool allowNonAlphabeticChars)
    {
        if (left.SequenceEqual(right))
            return true;
        if (left.Length != right.Length)
            return false;

        for (var i = 0; i < left.Length; i++)
        {
            var b1 = left[i];
            var b2 = right[i];

            if (!allowNonAlphabeticChars && b2 is < 65 or > 90)
                return false;

            if (b1 == b2 || ((!allowNonAlphabeticChars || b2 is >= 65 and <= 90) && (b1 - 32 == b2)))
                continue;
            return false;
        }

        return true;
    }
}
Repository: microsoft/garnet
File: libs/common/ExceptionInjectionHelper.cs (C#)

Code before:

        /// <summary>
        /// Wait on set condition
        /// </summary>
        /// <param name="exceptionType"></param>
        /// <returns></returns>
        public static async Task WaitOnSet(ExceptionInjectionType exceptionType)
        {
            var flag = ExceptionInjectionTypes[(int)exceptionType];
            if (flag)
            {
                // Reset and wait to signaled to go forward
                ExceptionInjectionTypes[(int)exceptionType] = false;
                while (!ExceptionInjectionTypes[(int)exceptionType])
                    await Task.Yield();
            }
        }

        /// <summary>
        /// Wait on clear condition
        /// </summary>
        /// <param name="exceptionType"></param>
        /// <returns></returns>
        public static async Task WaitOnClear(ExceptionInjectionType exceptionType)
        {
            var flag = ExceptionInjectionTypes[(int)exceptionType];
            if (flag)
            {
                while (ExceptionInjectionTypes[(int)exceptionType])
                    await Task.Yield();
            }
        }
    }
}

Review comment (security):
nit: Maybe a bit cleaner?
```cs
var flag = ExceptionInjectionTypes[(int)exceptionType];
while (flag)
{
    await Task.Yield();
    flag = ExceptionInjectionTypes[(int)exceptionType];
}
```

Code after:

        /// <summary>
        /// Wait on set condition
        /// </summary>
        /// <param name="exceptionType"></param>
        /// <returns></returns>
        public static async Task WaitOnSet(ExceptionInjectionType exceptionType)
        {
            var flag = ExceptionInjectionTypes[(int)exceptionType];
            if (flag)
            {
                // Reset and wait to signaled to go forward
                ExceptionInjectionTypes[(int)exceptionType] = false;
                while (!ExceptionInjectionTypes[(int)exceptionType])
                    await Task.Yield();
            }
        }

        /// <summary>
        /// Wait on clear condition
        /// </summary>
        /// <param name="exceptionType"></param>
        /// <returns></returns>
        public static async Task WaitOnClearAsync(ExceptionInjectionType exceptionType)
        {
            while (ExceptionInjectionTypes[(int)exceptionType])
                await Task.Yield();
        }
    }
}
Repository: microsoft/garnet
File: test/Garnet.test/DiskANN/DiskANNGridTests.cs (C#)

Code before:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using NUnit.Framework;
using NUnit.Framework.Legacy;
using StackExchange.Redis;

namespace Garnet.test.DiskANN
{
    [TestFixture]
    class DiskANNGridTests
    {
        GarnetServer server;

        [SetUp]
        public void Setup()
        {
            TestUtils.DeleteDirectory(TestUtils.MethodTestDir, wait: true);
            server = TestUtils.CreateGarnetServer(TestUtils.MethodTestDir);
            server.Start();
        }

        [TearDown]
        public void TearDown()
        {
            server.Dispose();
            TestUtils.DeleteDirectory(TestUtils.MethodTestDir);
        }

        [Test]
        [TestCase(100, 1, "NOQUANT")]
        [TestCase(10, 2, "NOQUANT")]
        [TestCase(3, 7, "NOQUANT")]
        [TestCase(4, 5, "NOQUANT")]
        public void SearchVectorsInGrid(int gridSize, int dimension, string quantType)
        {

Review comment (suggestion):
style nit: we're inconsistent in Garnet, but I would prefer if we had visibility modifiers on all these members.

So `public class DiskANNGridTests`, `private GarnetServer server;`, `private List<GridVector> GenerateGridVectors(int dimensions, int gridSize)` et. al., `private sealed class GridVector`, etc.

Code after:
﻿// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using Garnet.server;
using NUnit.Framework;
using NUnit.Framework.Legacy;
using StackExchange.Redis;

namespace Garnet.test.DiskANN
{
    [TestFixture]
    public class DiskANNGridTests
    {
        private GarnetServer server;

        [SetUp]
        public void Setup()
        {
            TestUtils.DeleteDirectory(TestUtils.MethodTestDir, wait: true);
            server = TestUtils.CreateGarnetServer(TestUtils.MethodTestDir);
            server.Start();
        }

        [TearDown]
        public void TearDown()
        {
            server.Dispose();
            TestUtils.DeleteDirectory(TestUtils.MethodTestDir);
        }

        [Test]
        [TestCase(100, 1, VectorQuantType.XPreQ8)]
        [TestCase(10, 2, VectorQuantType.XPreQ8)]
        [TestCase(3, 7, VectorQuantType.XPreQ8)]
        [TestCase(4, 5, VectorQuantType.XPreQ8)]
        public void SearchVectorsInGrid(int gridSize, int dimension, VectorQuantType quantType)