tonyroberts commented on code in PR #50843:
URL: https://github.com/apache/arrow/pull/50843#discussion_r3756399691
##########
cpp/src/arrow/type.h:
##########
@@ -196,6 +197,22 @@ class ARROW_EXPORT DataType : public
std::enable_shared_from_this<DataType>,
/// subclasses of FixedWidthType
virtual int bit_width() const { return -1; }
+ /// \brief Returns the number of bytes needed to store `num_elements`
+ /// values of this fixed-width type, rounding up for bit-packed types
+ /// (e.g. boolean) that use less than one byte per value. Returns -1
+ /// for non-fixed-width types, and should only be used for subclasses
+ /// of FixedWidthType
+ virtual int64_t bytes_required(int64_t num_elements) const {
Review Comment:
I did consider using `bit_util::BytesForBits(length * bit_width)`, but that
reduces the range (from INT64_MAX to INT64_MAX/8). This would be a real
behavioral change, and though it might be unlikely, could result in currently
working code causing an overflow, hence why I opted for doing it that way I did
(which uses a check consistent with bit_width and byte_width).
I did also consider having this as a utility function in vector_replace.cc,
but this is closely related to bit_width and byte_width and so the
implementation fits well where it is. I made it virtual deliberately so that
implementation types can provide their own implementations later, if needed (as
they do for both bit_width *and* byte_width, not just one or the other), but I
didn't go as far as doing that for all types as it's not on a hot path and that
didn't seem necessary right now.
There are other places where it would made sense to use this instead of
byte_witdth() * count, such as in grouper.cc. I didn't go in and change all of
these as I wanted to keep this commit minimal and only fix this one known,
critical, crashing, bug, and I haven't confirmed if the other places cause
issues or if they all are strictly numeric types. But, since this pattern is
repeated and may cause other similar bugs elsewhere (either now or in the
future) I considered it was worth pulling this out as a new method.
Regardless, I'm happy for you to discuss with the other maintainers, and for
you to implement a fix another way as you see fit, but I do think it's
important this bug gets fixed since it's a real, easily reproducible, bug that
causes a hard crash.
--
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]
