pitrou commented on code in PR #51251:
URL: https://github.com/apache/arrow/pull/51251#discussion_r4039286019
##########
cpp/src/arrow/compute/kernels/vector_statistics_test.cc:
##########
@@ -87,6 +87,48 @@ TEST_F(TestWinsorize, FloatingPoint) {
}
}
+TEST_F(TestWinsorize, SlicedInput) {
+ // GH-51224: the output is zero-offset, so a sliced input's validity bitmap
must be
+ // copied from the slice rather than shared, otherwise it is read from bit 0.
Review Comment:
Explaining the underlying cause of the bug that led us to write this unit
test isn't useful IMHO.
##########
cpp/src/arrow/compute/kernels/vector_statistics.cc:
##########
@@ -127,7 +128,21 @@ struct Winsorize {
DCHECK_EQ(out->buffers.size(), data.buffers.size());
out->null_count = data.null_count.load();
out->length = data.length;
- out->buffers[0] = data.buffers[0];
+ // ExecChunked seeds the output from the input chunk, so it can arrive
carrying that
+ // chunk's offset. The buffers below are built for this slice alone and
are read from
+ // bit and element zero, so the output owns no offset of its own.
+ out->offset = 0;
+ // A zero-offset input can share its validity bitmap, because the output
is read from
+ // bit 0 as well. A sliced input cannot: sharing would read the bitmap
from bit 0
+ // instead of from `data.offset`, so copy the slice's bits out.
Review Comment:
You can use `GetOrCopyNullBitmapBuffer` from
`arrow/compute/kernels/util_internal.h`.
##########
cpp/src/arrow/compute/kernels/vector_statistics_test.cc:
##########
@@ -87,6 +87,48 @@ TEST_F(TestWinsorize, FloatingPoint) {
}
}
+TEST_F(TestWinsorize, SlicedInput) {
+ // GH-51224: the output is zero-offset, so a sliced input's validity bitmap
must be
+ // copied from the slice rather than shared, otherwise it is read from bit 0.
+ for (auto type : FloatingPointTypes()) {
+ options_.lower_limit = 0.0;
+ options_.upper_limit = 1.0;
+ // The parent's leading nulls sit at different positions than the slice's,
so sharing
+ // the bitmap would move the nulls.
+ auto parent = ArrayFromJSON(type, "[1.1, 2.2, null, 4.4, null, 6.6, 7.7,
8.8]");
+ auto expected = ArrayFromJSON(type, "[null, 4.4, null, 6.6, 7.7]");
+ CheckWinsorize(parent->Slice(2, 5), expected);
+ }
+ for (auto type : IntTypes()) {
+ options_.lower_limit = 0.0;
+ options_.upper_limit = 1.0;
+ auto parent = ArrayFromJSON(type, "[1, 2, null, 4, null, 6, 7, 8]");
+ auto expected = ArrayFromJSON(type, "[null, 4, null, 6, 7]");
+ CheckWinsorize(parent->Slice(2, 5), expected);
+ }
+ // A slice of an array with no nulls at all keeps the null-free fast path.
+ options_.lower_limit = 0.25;
+ options_.upper_limit = 0.75;
+ auto dense = ArrayFromJSON(float64(), "[1.0, 2.0, 3.0, 44.0, 55.0, 66.0,
77.0]");
+ CheckWinsorize(dense->Slice(1, 5),
+ ArrayFromJSON(float64(), "[3.0, 3.0, 44.0, 55.0, 55.0]"));
+}
+
+TEST_F(TestWinsorize, SlicedChunkedInput) {
+ // ExecChunked seeds each output from the input chunk, so a sliced chunk
carries a
+ // non-zero offset into ClipValues. The output buffers cover the slice alone.
Review Comment:
Similarly, this comment is verging too much into the specifics of a single
bug, while this is a more generally useful unit test.
##########
cpp/src/arrow/compute/kernels/vector_statistics_test.cc:
##########
@@ -87,6 +87,48 @@ TEST_F(TestWinsorize, FloatingPoint) {
}
}
+TEST_F(TestWinsorize, SlicedInput) {
+ // GH-51224: the output is zero-offset, so a sliced input's validity bitmap
must be
+ // copied from the slice rather than shared, otherwise it is read from bit 0.
+ for (auto type : FloatingPointTypes()) {
Review Comment:
There's no need to test all types as all will follow the same internal
output allocation path. Just test with one value type.
--
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]
