adamreeve commented on code in PR #251:
URL: https://github.com/apache/arrow-dotnet/pull/251#discussion_r2752192644
##########
src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -470,7 +470,7 @@ private ArrowBuffer[] ImportFixedWidthBuffers(CArrowArray*
cArray, int bitWidth)
int length = checked((int)cArray->offset +
(int)cArray->length);
int valuesLength;
if (bitWidth >= 8)
- valuesLength = checked(length * bitWidth / 8);
+ valuesLength = checked((int)((ulong)length *
(ulong)bitWidth / 8ul));
Review Comment:
If we can assume that `bitWidth` is always a multiple of 8 then we could
just use `length * (bitWidth / 8)`.
If we can't assume that, then this code is incorrect because we round down
when dividing by 8 but should round up.
Eg. if `bitWidth = 9` and `length = 1`, we'd need 2 bytes to store all
values, not just 1.
I can see we assume `bitWidth` is a multiple of 8 [here for
example](https://github.com/apache/arrow-dotnet/blob/70ab46691df24225e2407ad3cc0d22fd534eb18b/src/Apache.Arrow/Arrays/ArrayDataConcatenator.cs#L387),
and I can't think of a situation in which that wouldn't be true except for
booleans which fall through to the `bitWidth < 8` case below. Maybe we should
check that `bitWidth % 8 == 0` and throw an exception if that isn't true?
--
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]
