Skip to content

GH-51224: [C++] Keep the correct nulls when winsorizing a sliced array - #51251

Open
1fanwang wants to merge 4 commits into
apache:mainfrom
1fanwang:1fannnw/gh-51224-winsorize-slice-offset
Open

GH-51224: [C++] Keep the correct nulls when winsorizing a sliced array#51251
1fanwang wants to merge 4 commits into
apache:mainfrom
1fanwang:1fannnw/gh-51224-winsorize-slice-offset

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

winsorize on a sliced array returns the wrong null pattern: values come back where the input had nulls, and nulls where it had values. Limits of 0.0 and 1.0, which should return the input unchanged, still corrupt it.

flat   = pa.array([1.0, 2.0, None, 4.0, None, 6.0, 7.0, 8.0], pa.float64())
sliced = flat.slice(2, 5)                       # [None, 4.0, None, 6.0, 7.0]

pc.winsorize(sliced, lower_limit=0.0, upper_limit=1.0).to_pylist()
# before: [0.0, 4.0, None, 6.0, None]
# after:  [None, 4.0, None, 6.0, 7.0]

[valid, valid, null, valid, null] is the parent's bitmap read from bit 0, not from bit 2.

What changes are included in this PR?

ClipValues built a zero-offset output but assigned the input's validity buffer to it unchanged, so a non-zero data.offset was ignored when the bitmap was read back. The values loop was already correct, since GetValues applies the offset. The bitmap is now copied out of the slice with CopyBitmap, and a null-free input still shares the no-bitmap fast path.

Are these changes tested?

Yes. TestWinsorize.SlicedInput covers floating point and integer slices whose parent nulls sit at different positions, plus a null-free slice.

Raw logs
# before: restore the kernel to its pre-change state, keeping the new test
$ git checkout HEAD~1 -- cpp/src/arrow/compute/kernels/vector_statistics.cc
$ ninja arrow-compute-vector-test && ./debug/arrow-compute-vector-test --gtest_filter='TestWinsorize.SlicedInput'
Actual:
  [
    0,
    4,
    null,
    6,
    null
  ]
[  FAILED  ] TestWinsorize.SlicedInput (6 ms)
 1 FAILED TEST

# after: restore the change
$ git checkout HEAD -- cpp/src/arrow/compute/kernels/vector_statistics.cc
$ ninja arrow-compute-vector-test && ./debug/arrow-compute-vector-test --gtest_filter='TestWinsorize.*'
[  PASSED  ] 5 tests.

$ ./debug/arrow-compute-vector-test
[==========] 1146 tests from 152 test suites ran. (2656 ms total)
[  PASSED  ] 1146 tests.

Are there any user-facing changes?

Yes, winsorize returns the correct nulls for a sliced input.

Closes: #51224

…d 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>
Copilot AI lite review requested due to automatic review settings September 9, 2026 01:05
@1fanwang
1fanwang requested a review from pitrou as a code owner September 9, 2026 01:05
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51224 has been automatically assigned in GitHub to PR creator.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #51224 has no components, please add labels for components.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change correctly addresses the sliced-offset bitmap bug and includes targeted regression tests, with only a minor optional performance optimization noted.

Pull request overview

Fixes winsorize producing an incorrect null pattern when the input is a sliced ArrayData with a non-zero offset, by ensuring the output’s zero-offset validity bitmap matches the slice rather than the parent.

Changes:

  • Copy the validity bitmap slice into the zero-offset output in Winsorize::ClipValues (instead of sharing the input bitmap buffer).
  • Add regression coverage for sliced floating-point and integer inputs (plus a null-free sliced input case).
File summaries
File Description
cpp/src/arrow/compute/kernels/vector_statistics.cc Ensures winsorize output validity bitmap matches sliced input offsets by copying the relevant bits.
cpp/src/arrow/compute/kernels/vector_statistics_test.cc Adds a regression test verifying correct null placement for sliced inputs across numeric types.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread cpp/src/arrow/compute/kernels/vector_statistics.cc Outdated
… 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>
Copilot AI review requested due to automatic review settings September 9, 2026 07:28
@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

ClipValues doesn’t explicitly force out->offset = 0, which can break correctness and potentially cause out-of-bounds writes when invoked with a non-zero output offset (e.g., sliced chunks in the chunked path).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread cpp/src/arrow/compute/kernels/vector_statistics.cc
Generated-by: GitHub Copilot CLI (Claude Opus 5)
Signed-off-by: 1fanwang <1fannnw@gmail.com>
Copilot AI review requested due to automatic review settings September 9, 2026 08:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix directly addresses the reported offset/bitmap bug with a targeted, safe change and includes a focused regression test that fails before and passes after the patch.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

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>
Copilot AI review requested due to automatic review settings September 9, 2026 08:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix is localized, addresses the described root cause (offset-aware validity), and is covered by targeted regression tests.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++] winsorize on a sliced array ignores the offset: wrong rows come out null

2 participants