From 0f39a4ef6c551154bb809c84bdbea1f0e0cf5ed6 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Tue, 8 Sep 2026 18:02:42 -0700 Subject: [PATCH 1/6] 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. 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 9b6378e012a07b4f9b993a267c22430abd99911d Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 00:27:16 -0700 Subject: [PATCH 2/6] 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. 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 f309b67528dfa896db8ee1e14866d4a613ac7271 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 01:13:46 -0700 Subject: [PATCH 3/6] GH-51224: [C++] Apply clang-format 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 c49917025212ceda4f0ddcbc7d0ab0f1d33c5f94 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 9 Sep 2026 01:17:41 -0700 Subject: [PATCH 4/6] 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. 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; From 0fcff90e1155fbae736a7eb5478aeee84eb16bc9 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Thu, 17 Sep 2026 12:15:20 -0700 Subject: [PATCH 5/6] GH-51224: [C++] Reuse the bitmap helper in winsorize Signed-off-by: 1fanwang <1fannnw@gmail.com> --- .../compute/kernels/vector_statistics.cc | 18 +++---------- .../compute/kernels/vector_statistics_test.cc | 27 +++++-------------- 2 files changed, 9 insertions(+), 36 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index df997ac25bf9..543e57a691b8 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -26,13 +26,13 @@ #include "arrow/compute/function.h" #include "arrow/compute/kernel.h" #include "arrow/compute/kernels/codegen_internal.h" +#include "arrow/compute/kernels/util_internal.h" #include "arrow/compute/registry.h" #include "arrow/compute/registry_internal.h" #include "arrow/result.h" #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" @@ -128,21 +128,9 @@ 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. - 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] = data.buffers[0]; - } + ARROW_ASSIGN_OR_RAISE(out->buffers[0], + GetOrCopyNullBitmapBuffer(data, ctx->memory_pool())); 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 119481f59493..b8429b202e24 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc @@ -88,25 +88,12 @@ 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.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 expected = ArrayFromJSON(float64(), "[null, 4.4, null, 6.6, 7.7]"); + CheckWinsorize(parent->Slice(2, 5), expected); + 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]"); @@ -115,8 +102,6 @@ TEST_F(TestWinsorize, SlicedInput) { } 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]"); From 97da9e319d15bfed78cdf3750fab0b3ef6f93c21 Mon Sep 17 00:00:00 2001 From: 1fanwang <1fannnw@gmail.com> Date: Wed, 23 Sep 2026 23:09:40 -0700 Subject: [PATCH 6/6] GH-51224: [C++] Preserve sliced null and NaN-only arrays Signed-off-by: 1fanwang <1fannnw@gmail.com> --- cpp/src/arrow/compute/kernels/vector_statistics.cc | 1 + .../arrow/compute/kernels/vector_statistics_test.cc | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/vector_statistics.cc b/cpp/src/arrow/compute/kernels/vector_statistics.cc index 543e57a691b8..cc2fbc40164f 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics.cc @@ -72,6 +72,7 @@ struct Winsorize { // Only nulls and NaNs => return input as-is out_data->null_count = data->null_count.load(); out_data->length = data->length; + out_data->offset = data->offset; out_data->buffers = data->buffers; return Status::OK(); } diff --git a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc index b8429b202e24..72a765be1039 100644 --- a/cpp/src/arrow/compute/kernels/vector_statistics_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_statistics_test.cc @@ -114,6 +114,18 @@ TEST_F(TestWinsorize, SlicedChunkedInput) { CheckWinsorize(chunked, expected); } +TEST_F(TestWinsorize, SlicedInputWithoutQuantiles) { + for (const auto* json_input : + {"[1, 2, null, null, null, 3]", "[1, 2, NaN, null, NaN, 3]", + "[1, 2, NaN, NaN, NaN, 3]"}) { + auto parent = ArrayFromJSON(float64(), json_input); + auto sliced = parent->Slice(2, 3); + CheckWinsorize(sliced, sliced); + auto chunked = std::make_shared(ArrayVector{sliced}); + CheckWinsorize(chunked, chunked); + } +} + TEST_F(TestWinsorize, Integral) { for (auto type : IntTypes()) { options_.lower_limit = 0.25;