Skip to content

GH-47447: [C++] Fix replace_with_mask for null type arrays#49950

Open
AnkitAhlawat7742 wants to merge 5 commits into
apache:mainfrom
AnkitAhlawat7742:fix/replace_with_mask_crash
Open

GH-47447: [C++] Fix replace_with_mask for null type arrays#49950
AnkitAhlawat7742 wants to merge 5 commits into
apache:mainfrom
AnkitAhlawat7742:fix/replace_with_mask_crash

Conversation

@AnkitAhlawat7742
Copy link
Copy Markdown
Contributor

@AnkitAhlawat7742 AnkitAhlawat7742 commented May 8, 2026

Fix #47447

Rationale for this change

replace_with_mask crashes when called with null type arrays because a code path incorrectly returns Status::OK() from a function with return type Result<int64_t>.
This PR fixes the issue by ensuring the function returns a valid int64_t value instead of Status::OK() for successful executon

What changes are included in this PR?

  1. Fixed the replace_with_mask kernel implementation for null type arrays.
  2. Replaced the invalid Status::OK() return path with a valid int64_t result.
  3. Prevented construction of Result<int64_t> with an OK Status.

Are these changes tested?

Yes, manually run the test case

Are there any user-facing changes?

No

This PR contains a "Critical Fix".

This change fixes a crash in replace_with_mask when operating on null type arrays. The crash occurs due to an invalid successful return path internally constructing Result with Status::OK().

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

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

@AnkitAhlawat7742
Copy link
Copy Markdown
Contributor Author

Hi @raulcd ,
Could you please re-run the failed job? i see download build timed out while downloading the ORC dependency (orc-2.2.1.tar.gz)

 --- LOG END ---
          error: downloading 'https://dlcdn.apache.org/orc/orc-2.2.1/orc-2.2.1.tar.gz' failed
          status_code: 22
          status_string: "HTTP response code said error"
          log:
          --- LOG BEGIN ---
            Trying 151.101.2.132:443...

@AnkitAhlawat7742
Copy link
Copy Markdown
Contributor Author

Hi @raulcd ,
Please review the changes ,let me know if any modification needed

Copy link
Copy Markdown
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Thanks @AnkitAhlawat7742 for this PR.

Can you add some C++ tests in the existing file vector_replace_test.cc?
You can get some inspiration from the existing TestReplaceBoolean.

b = pa.array([None], pa.null())

result = pc.replace_with_mask(a, True, b)
assert result.type == pa.null()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, but can you test the actual result values?
Something like:

Suggested change
assert result.type == pa.null()
assert result.type == pa.null()
result.validate(full=True)
assert result.to_pylist() == [None]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the Test case
Please have a look

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 13, 2026
…p and updated the python test with null type
@AnkitAhlawat7742
Copy link
Copy Markdown
Contributor Author

Thanks @AnkitAhlawat7742 for this PR.

Can you add some C++ tests in the existing file vector_replace_test.cc? You can get some inspiration from the existing TestReplaceBoolean.

HI @pitrou ,
Thanks for the review ..!!
I have updated the python Test case and added the new test case in vector_replace_test file .and thanks for the reference of existing class

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[C++][Python] replace_with_mask crashes when null type inputs are used

2 participants