asmirnov82 commented on code in PR #35342:
URL: https://github.com/apache/arrow/pull/35342#discussion_r1187973894
##########
csharp/src/Apache.Arrow/BitUtility.cs:
##########
@@ -62,6 +62,64 @@ public static void SetBit(Span<byte> data, int index, bool
value)
: (byte)(data[idx] & ~BitMask[mod]);
}
+ /// <summary>
+ /// Set the number of bits in a span of bytes starting
+ /// at a specific index, and limiting to length.
+ /// </summary>
+ /// <param name="data">Span to set bits value.</param>
+ /// <param name="offset">Bit index to start counting from.</param>
+ /// <param name="length">Maximum of bits in the span to
consider.</param>
+ public static void SetBits(Span<byte> data, int index, int length,
bool value)
Review Comment:
You are right. However, after taking a closer look at BitUtility class I
have some concern that BitUtility should be a public API. And here are reasons:
1) There aren't any argument checking in all other methods (however, there
are debug.asserts - I think it's done due to perfomance reason)
2) This class is implemented with an assumption of being used with Bitmap
only, when we know that the number of meaningful bits should be less than
Max.Int (because each bit corresponds to the validity of value in Value.Span,
wich length is limit by Int). In general case the amount of bit inside a
Span<byte> can be Max,Int * 8 and correct methods for working with bits inside
a span should have long value for bit index and for length.
I added required checks into SetBits and CountBits methods (as this two are
very similar) and didn't change other methods. You may see the resulting code
in PR, but actually it looks intermediate compromise between two solutions:
change all BitUtility methods to use longs for bit indices (with possible
performance drawdown)
or
make BitUtility internal (breaking change, in case somebody uses BitUtility
in their code)
Would you please make some inputs on that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
