pitrou commented on code in PR #50763:
URL: https://github.com/apache/arrow/pull/50763#discussion_r4080711242
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -561,12 +586,14 @@ struct ArrayWriterV1 {
const auto& fw_type = checked_cast<const FixedWidthType&>(*values.type());
if (prim_values.values()) {
+ if constexpr (is_boolean_type<T>::value) {
+ return WriteBitmap(prim_values.values()->data(), values.length(),
+ prim_values.offset());
+ }
const uint8_t* buffer =
Review Comment:
Since the above is `if constexpr`, we might want to use an `else` branch
here (making it `constexpr` too).
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0,
/*offset=*/1);
+ auto array = MakeArray(data);
+ auto table = Table::Make(schema({field("flag", boolean())}),
+ {std::make_shared<ChunkedArray>(array)});
+
+ DoWrite(*table);
+ ASSERT_GT(output_->size(), 8);
+ ASSERT_EQ(output_->data()[8], 0x01);
Review Comment:
And by the way, if `output_`'s size is 8, then you cannot read beyond
`output_->data()[7]`.
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0,
/*offset=*/1);
+ auto array = MakeArray(data);
+ auto table = Table::Make(schema({field("flag", boolean())}),
+ {std::make_shared<ChunkedArray>(array)});
+
+ DoWrite(*table);
+ ASSERT_GT(output_->size(), 8);
+ ASSERT_EQ(output_->data()[8], 0x01);
Review Comment:
Is this testing the raw Feather bytes? Can you add a comment explaining
these checks?
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
+ GTEST_SKIP() << "This test targets the Feather V1 bitmap writer";
+ }
+
+ // The second byte is outside the declared buffer. It must not be read into
+ // the unused bits of the serialized one-bit slice.
+ std::array<uint8_t, 2> storage = {0x02, 0x01};
+ auto values = std::make_shared<Buffer>(storage.data(), 1);
+ auto data =
+ ArrayData::Make(boolean(), 1, {nullptr, values}, /*null_count=*/0,
/*offset=*/1);
Review Comment:
Can we have a loop running `offset` from 0 to 7?
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -561,12 +586,14 @@ struct ArrayWriterV1 {
const auto& fw_type = checked_cast<const FixedWidthType&>(*values.type());
if (prim_values.values()) {
+ if constexpr (is_boolean_type<T>::value) {
+ return WriteBitmap(prim_values.values()->data(), values.length(),
+ prim_values.offset());
+ }
const uint8_t* buffer =
prim_values.values()->data() + (prim_values.offset() *
fw_type.bit_width() / 8);
- int64_t bit_offset = (prim_values.offset() * fw_type.bit_width()) % 8;
- return WriteBuffer(buffer,
- bit_util::BytesForBits(values.length() *
fw_type.bit_width()),
- bit_offset);
+ return WriteBuffer(
Review Comment:
Let's add a `DCHECK_EQ(fw_type.bit_width() % 8, 0)` to make sure we're not
handling any non-byte-aligned type here?
##########
cpp/src/arrow/ipc/feather_test.cc:
##########
@@ -323,6 +324,30 @@ TEST_P(TestFeather, SliceBooleanRoundTrip) {
CheckSlices(batch);
}
+TEST_P(TestFeather, ExactSizeSlicedBooleanBuffer) {
+ if (GetParam().version != kFeatherV1Version) {
Review Comment:
Add a comment at top pointing to the GH issue?
##########
cpp/src/arrow/ipc/feather.cc:
##########
@@ -75,24 +75,31 @@ inline int64_t PaddedLength(int64_t nbytes) {
return ((nbytes + alignment - 1) / alignment) * alignment;
}
-Status WritePaddedWithOffset(io::OutputStream* stream, const uint8_t* data,
- int64_t bit_offset, const int64_t length,
- int64_t* bytes_written) {
+Status WritePaddedBitmap(io::OutputStream* stream, const uint8_t* data,
+ int64_t bit_offset, const int64_t bit_length,
+ int64_t* bytes_written) {
data = data + bit_offset / 8;
- uint8_t bit_shift = static_cast<uint8_t>(bit_offset % 8);
- if (bit_offset == 0) {
+ const uint8_t bit_shift = static_cast<uint8_t>(bit_offset % 8);
+ const int64_t length = bit_util::BytesForBits(bit_length);
+ if (bit_shift == 0) {
RETURN_NOT_OK(stream->Write(data, length));
} else {
constexpr int64_t buffersize = 256;
uint8_t buffer[buffersize];
const uint8_t lshift = static_cast<uint8_t>(8 - bit_shift);
+ const uint8_t* data_end =
+ data + bit_util::BytesForBits(bit_shift + bit_length);
const uint8_t* buffer_end = buffer + buffersize;
uint8_t* buffer_it = buffer;
- for (const uint8_t* end = data + length; data != end;) {
+ for (int64_t i = 0; i < length; ++i) {
uint8_t r = static_cast<uint8_t>(*data++ >> bit_shift);
- uint8_t l = static_cast<uint8_t>(*data << lshift);
+ uint8_t l =
+ data == data_end ? 0 : static_cast<uint8_t>(*data << lshift);
uint8_t value = l | r;
+ if (i == length - 1 && bit_length % 8 != 0) {
+ value &= bit_util::LeastSignificantBitMask<uint8_t>(bit_length % 8);
+ }
Review Comment:
Is this actually required? I don't think it matters that we are writing
potentially non-zero bits past the logical end of the bitmap.
--
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]
