Fixed code review findings

Author: asmirnov82

src/Microsoft.Data.Analysis/DateTimeComputation.cs

@@ -202,10 +202,8 @@ public void Max(PrimitiveColumnContainer<DateTime> column, out DateTime? ret)
                 var bitmapSpan = column.NullBitMapBuffers[b].ReadOnlySpan;
                 for (int i = 0; i < readOnlySpan.Length; i++)
                 {
-                    int byteIndex = (int)((uint)i / 8);
-
                     //Check if bit is not set (value is null) - skip
-                    if (((bitmapSpan[byteIndex] >> (i & 7)) & 1) == 0)
+                    if (!BitmapHelper.IsValid(bitmapSpan, i))
                         continue;
 
                     var val = readOnlySpan[i];
@@ -262,10 +260,8 @@ public void Min(PrimitiveColumnContainer<DateTime> column, out DateTime? ret)
 
                 for (int i = 0; i < readOnlySpan.Length; i++)
                 {
-                    int byteIndex = (int)((uint)i / 8);
-
                     //Check if bit is not set (value is null) - skip
-                    if (((bitmapSpan[byteIndex] >> (i & 7)) & 1) == 0)
+                    if (!BitmapHelper.IsValid(bitmapSpan, i))
                         continue;
 
                     var val = readOnlySpan[i];

src/Microsoft.Data.Analysis/PrimitiveColumnContainer.cs

@@ -12,8 +12,24 @@
 
 namespace Microsoft.Data.Analysis
 {
+    internal static class BitmapHelper
+    {
+        // Faster to use when we already have a span since it avoids indexing
+        public static bool IsValid(ReadOnlySpan<byte> bitMapBufferSpan, int index)
+        {
+            int nullBitMapSpanIndex = index / 8;
+            byte thisBitMap = bitMapBufferSpan[nullBitMapSpanIndex];
+            return IsBitSet(thisBitMap, index);
+        }
+
+        public static bool IsBitSet(byte curBitMap, int index)
+        {
+            return ((curBitMap >> (index & 7)) & 1) != 0;
+        }
+    }
+
     /// <summary>
-    /// PrimitiveDataFrameColumnContainer is just a store for the column data. APIs that want to change the data must be defined in PrimitiveDataFrameColumn
+    /// PrimitiveColumnContainer is just a store for the column data. APIs that want to change the data must be defined in PrimitiveDataFrameColumn
     /// </summary>
     /// <typeparam name="T"></typeparam>
     internal partial class PrimitiveColumnContainer<T> : IEnumerable<T?>
@@ -223,7 +239,7 @@ public void ApplyElementwise(Func<T?, long, T?> func)
                 for (int i = 0; i < mutableBuffer.Length; i++)
                 {
                     long curIndex = i + prevLength;
-                    bool isValid = IsValid(mutableNullBitMapBuffer, i);
+                    bool isValid = BitmapHelper.IsValid(mutableNullBitMapBuffer, i);
                     T? value = func(isValid ? mutableBuffer[i] : null, curIndex);
                     mutableBuffer[i] = value.GetValueOrDefault();
                     SetValidityBit(mutableNullBitMapBuffer, i, value != null);
@@ -246,22 +262,14 @@ public void Apply<TResult>(Func<T?, TResult?> func, PrimitiveColumnContainer<TRe
 
                 for (int i = 0; i < sourceBuffer.Length; i++)
                 {
-                    bool isValid = IsValid(sourceNullBitMap, i);
+                    bool isValid = BitmapHelper.IsValid(sourceNullBitMap, i);
                     TResult? value = func(isValid ? sourceBuffer[i] : null);
                     mutableResultBuffer[i] = value.GetValueOrDefault();
                     resultContainer.SetValidityBit(mutableResultNullBitMapBuffers, i, value != null);
                 }
             }
         }
 
-        // Faster to use when we already have a span since it avoids indexing
-        public bool IsValid(ReadOnlySpan<byte> bitMapBufferSpan, int index)
-        {
-            int nullBitMapSpanIndex = index / 8;
-            byte thisBitMap = bitMapBufferSpan[nullBitMapSpanIndex];
-            return IsBitSet(thisBitMap, index);
-        }
-
         public bool IsValid(long index) => NullCount == 0 || GetValidityBit(index);
 
         private byte SetBit(byte curBitMap, int index, bool value)
@@ -329,11 +337,6 @@ internal void SetValidityBit(long index, bool value)
             SetValidityBit(bitMapBuffer.Span, (int)index, value);
         }
 
-        private bool IsBitSet(byte curBitMap, int index)
-        {
-            return ((curBitMap >> (index & 7)) & 1) != 0;
-        }
-
         private bool GetValidityBit(long index)
         {
             if ((uint)index >= Length)
@@ -350,7 +353,7 @@ private bool GetValidityBit(long index)
             int bitMapBufferIndex = (int)((uint)index / 8);
             Debug.Assert(bitMapBuffer.Length > bitMapBufferIndex);
             byte curBitMap = bitMapBuffer[bitMapBufferIndex];
-            return IsBitSet(curBitMap, (int)index);
+            return BitmapHelper.IsBitSet(curBitMap, (int)index);
         }
 
         public long Length;
@@ -524,7 +527,7 @@ public PrimitiveColumnContainer<T> Clone<U>(PrimitiveColumnContainer<U> mapIndic
                         spanIndex = buffer.Length - 1 - i;
 
                     long mapRowIndex = mapIndicesIntSpan.IsEmpty ? mapIndicesLongSpan[spanIndex] : mapIndicesIntSpan[spanIndex];
-                    bool mapRowIndexIsValid = mapIndices.IsValid(mapIndicesNullBitMapSpan, spanIndex);
+                    bool mapRowIndexIsValid = BitmapHelper.IsValid(mapIndicesNullBitMapSpan, spanIndex);
                     if (mapRowIndexIsValid && (mapRowIndex < minRange || mapRowIndex >= maxRange))
                     {
                         int bufferIndex = (int)(mapRowIndex / maxCapacity);
@@ -539,7 +542,7 @@ public PrimitiveColumnContainer<T> Clone<U>(PrimitiveColumnContainer<U> mapIndic
                     {
                         mapRowIndex -= minRange;
                         value = thisSpan[(int)mapRowIndex];
-                        isValid = IsValid(thisNullBitMapSpan, (int)mapRowIndex);
+                        isValid = BitmapHelper.IsValid(thisNullBitMapSpan, (int)mapRowIndex);
                     }
 
                     retSpan[i] = isValid ? value : default;

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.Sort.cs

@@ -45,7 +45,7 @@ private Int64DataFrameColumn GetSortIndices(IComparer<T> comparer, out Int64Data
                 for (int i = 0; i < sortIndices.Length; i++)
                 {
                     int localSortIndex = sortIndices[i];
-                    if (_columnContainer.IsValid(nullBitMapSpan, localSortIndex))
+                    if (BitmapHelper.IsValid(nullBitMapSpan, localSortIndex))
                     {
                         nonNullSortIndices.Add(sortIndices[i]);
                     }

src/Microsoft.Data.Analysis/PrimitiveDataFrameColumn.cs

@@ -553,7 +553,7 @@ public override Dictionary<TKey, ICollection<long>> GroupColumnValues<TKey>(out
                     for (int i = 0; i < readOnlySpan.Length; i++)
                     {
                         long currentLength = i + previousLength;
-                        if (_columnContainer.IsValid(nullBitMapSpan, i))
+                        if (BitmapHelper.IsValid(nullBitMapSpan, i))
                         {
                             bool containsKey = multimap.TryGetValue(readOnlySpan[i], out ICollection<long> values);
                             if (containsKey)