From 0733e285d3fb1c7b9fba04ff06eb0a8f47271e49 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Tue, 8 Sep 2026 18:02:42 -0700 Subject: [PATCH 1/4] GH-51224: [C++] Keep the correct nulls when winsorizing a sliced array The output is zero-offset, so sharing a sliced input's validity buffer made readers interpret it from bit 0 and move the nulls. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../compute/kernels/vector_statistics.cc | 12 ++++++++- .../compute/kernels/vector_statistics_test.cc | 26 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index 074f2ec0a738..bdcce1400a2e 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -32,6 +32,7 @@ #include "arrow/scalar.h" #include "arrow/status.h" #include "arrow/util/bit_run_reader.h" +#include "arrow/util/bitmap_ops.h" #include "arrow/util/checked_cast.h" #include "arrow/util/logging_internal.h" @@ -127,7 +128,16 @@ 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]; + // The output is zero-offset, so a sliced input's validity bitmap cannot be shared as is: + // it would be read from bit 0 instead of from `data.offset`. Copy the slice's bits out. + if (data.buffers[0]) { + ARROW_ASSIGN_OR_RAISE( + out->buffers[0], arrow::internal::CopyBitmap(ctx->memory_pool(), + data.buffers[0]->data(), data.offset, + data.length)); + } else { + out->buffers[0] = nullptr; + } ARROW_ASSIGN_OR_RAISE(out->buffers[1], ctx->Allocate(out->length * sizeof(CType))); // Avoid leaving uninitialized memory under null entries std::memset(out->buffers[1]->mutable_data(), 0, out->length * sizeof(CType)); diff --git a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc index 97715cdaedd2..1354babe3a23 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc @@ -87,6 +87,32 @@ 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, Integral) { for (auto type : IntTypes()) { options_.lower_limit = 0.25; From f75624f7495cfa82208276225d0bb133e29b20b3 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 00:27:16 -0700 Subject: [PATCH 2/4] GH-51224: [C++] Share the validity bitmap when the input is not sliced A zero-offset input is read from bit 0 in both the input and the output, so copying it only adds an allocation. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com> --- cpp/src/arrow/compute/kernels/vector_statistics.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index bdcce1400a2e..fea7e69ae756 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -128,15 +128,16 @@ struct Winsorize { DCHECK_EQ(out->buffers.size(), data.buffers.size()); out->null_count = data.null_count.load(); out->length = data.length; - // The output is zero-offset, so a sliced input's validity bitmap cannot be shared as is: - // it would be read from bit 0 instead of from `data.offset`. Copy the slice's bits out. - if (data.buffers[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. + if (data.buffers[0] && data.offset != 0) { ARROW_ASSIGN_OR_RAISE( out->buffers[0], arrow::internal::CopyBitmap(ctx->memory_pool(), data.buffers[0]->data(), data.offset, data.length)); } else { - out->buffers[0] = nullptr; + out->buffers[0] = data.buffers[0]; } ARROW_ASSIGN_OR_RAISE(out->buffers[1], ctx->Allocate(out->length * sizeof(CType))); // Avoid leaving uninitialized memory under null entries From 31bb7291231452c6aa26bf7348524c4679a5e1de Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 01:13:46 -0700 Subject: [PATCH 3/4] GH-51224: [C++] Apply clang-format Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com> --- cpp/src/arrow/compute/kernels/vector_statistics.cc | 6 +++--- cpp/src/arrow/compute/kernels/vector_statistics_test.cc | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index fea7e69ae756..668b8f515a3f 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -133,9 +133,9 @@ struct Winsorize { // instead of from `data.offset`, so copy the slice's bits out. if (data.buffers[0] && data.offset != 0) { ARROW_ASSIGN_OR_RAISE( - out->buffers[0], arrow::internal::CopyBitmap(ctx->memory_pool(), - data.buffers[0]->data(), data.offset, - data.length)); + out->buffers[0], + arrow::internal::CopyBitmap(ctx->memory_pool(), data.buffers[0]->data(), + data.offset, data.length)); } else { out->buffers[0] = data.buffers[0]; } diff --git a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc index 1354babe3a23..d9e031c0f6b2 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc @@ -110,7 +110,8 @@ TEST_F(TestWinsorize, SlicedInput) { 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]")); + CheckWinsorize(dense->Slice(1, 5), + ArrayFromJSON(float64(), "[3.0, 3.0, 44.0, 55.0, 55.0]")); } TEST_F(TestWinsorize, Integral) { From c0882dbed77501a7d91fb41d47ae0fa29ffad459 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 01:17:41 -0700 Subject: [PATCH 4/4] GH-51224: [C++] Clear the output offset when winsorizing ExecChunked seeds the output from the input chunk, so a sliced chunk carried its offset into buffers built for the slice alone. Generated-by: GitHub Copilot CLI (Claude Opus 5) Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../arrow/compute/kernels/vector_statistics.cc | 4 ++++ .../compute/kernels/vector_statistics_test.cc | 15 +++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index 668b8f515a3f..df997ac25bf9 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -128,6 +128,10 @@ struct Winsorize { DCHECK_EQ(out->buffers.size(), data.buffers.size()); out->null_count = data.null_count.load(); out->length = data.length; + // 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. diff --git a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc index d9e031c0f6b2..119481f59493 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc @@ -114,6 +114,21 @@ TEST_F(TestWinsorize, SlicedInput) { 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. + options_.lower_limit = 0.0; + options_.upper_limit = 1.0; + auto parent = ArrayFromJSON(float64(), "[1.1, 2.2, null, 4.4, null, 6.6, 7.7, 8.8]"); + auto chunked = std::make_shared( + ArrayVector{parent->Slice(2, 3), parent->Slice(5, 3)}); + auto expected = std::make_shared(ArrayVector{ + ArrayFromJSON(float64(), "[null, 4.4, null]"), + ArrayFromJSON(float64(), "[6.6, 7.7, 8.8]"), + }); + CheckWinsorize(chunked, expected); +} + TEST_F(TestWinsorize, Integral) { for (auto type : IntTypes()) { options_.lower_limit = 0.25;