Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AnkitAhlawat7742 wants to merge 5 commits into
base: main
Choose a base branch
from
+68 −4
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
498166c
GH-47447: [C++] Fix replace_with_mask for null type arrays
AnkitAhlawat7742 May 8, 2026
9e8a563
GH-47447: [Python] Add pandas replace mask and combine_first scenario…
AnkitAhlawat7742 May 12, 2026
2f48adb
GH-47447: [Python] Remove pandas dependency from test
AnkitAhlawat7742 May 12, 2026
46803f4
GH-47447: [C++] [Python]Add newtest for replace_with_mask in cpp and …
AnkitAhlawat7742 May 14, 2026
970909a
GH-47447: [C++] Fix clang-format in vector_replace_test.cc
AnkitAhlawat7742 May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions cpp/src/arrow/compute/kernels/vector_replace.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,15 +217,15 @@ struct ReplaceMaskImpl<Type, enable_if_null<Type>> {
static Result<int64_t> ExecScalarMask(KernelContext* ctx, const ArraySpan& array,
const BooleanScalar& mask, ExecValue replacements,
int64_t replacements_offset, ExecResult* out) {
out->value = array;
return Status::OK();
out->value = array.ToArrayData();
return replacements_offset;
}
static Result<int64_t> ExecArrayMask(KernelContext* ctx, const ArraySpan& array,
const ArraySpan& mask, int64_t mask_offset,
ExecValue replacements,
int64_t replacements_offset, ExecResult* out) {
out->value = array;
return Status::OK();
out->value = array.ToArrayData();
return replacements_offset;
}
};

Expand Down
36 changes: 36 additions & 0 deletions cpp/src/arrow/compute/kernels/vector_replace_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,13 @@ class TestReplaceBoolean : public TestReplaceKernel<BooleanType> {
}
};

class TestReplaceNull : public TestReplaceKernel<NullType> {
protected:
std::shared_ptr<DataType> type() override {
return TypeTraits<NullType>::type_singleton();
}
};

class TestReplaceFixedSizeBinary : public TestReplaceKernel<FixedSizeBinaryType> {
protected:
std::shared_ptr<DataType> type() override { return fixed_size_binary(3); }
Expand Down Expand Up @@ -538,6 +545,35 @@ TEST_F(TestReplaceBoolean, ReplaceWithMask) {
}
}

TEST_F(TestReplaceNull, ReplaceWithMask) {
std::vector<ReplaceWithMaskCase> cases = {
{this->array("[]"), this->mask_scalar(false), this->array("[]"), this->array("[]")},
{this->array("[]"), this->mask_scalar(true), this->array("[]"), this->array("[]")},
{this->array("[]"), this->null_mask_scalar(), this->array("[]"), this->array("[]")},

{this->array("[null]"), this->mask_scalar(false), this->array("[]"),
this->array("[null]")},

{this->array("[null]"), this->mask_scalar(true), this->array("[null]"),
this->array("[null]")},

{this->array("[null]"), this->null_mask_scalar(), this->array("[]"),
this->array("[null]")},

{this->array("[null, null]"), this->mask("[false, false]"), this->array("[]"),
this->array("[null, null]")},
{this->array("[null, null]"), this->mask("[true, true]"),
this->array("[null, null]"), this->array("[null, null]")},
{this->array("[null, null]"), this->mask("[null, null]"), this->array("[]"),
this->array("[null, null]")},
};

for (auto test_case : cases) {
this->Assert(ReplaceWithMask, test_case.input, test_case.mask, test_case.replacements,
test_case.expected);
}
}

TEST_F(TestReplaceBoolean, ReplaceWithMaskErrors) {
EXPECT_RAISES_WITH_MESSAGE_THAT(
Invalid,
Expand Down
28 changes: 28 additions & 0 deletions python/pyarrow/tests/test_compute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1987,6 +1987,34 @@ def test_fill_null_array(arrow_type):
assert result.equals(expected)


def test_replace_with_mask_null_type():
# GH-47447: replace_with_mask crashed for null type arrays
a = pa.array([None], pa.null())
b = pa.array([None], pa.null())

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

@pitrou pitrou May 13, 2026

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

@AnkitAhlawat7742 AnkitAhlawat7742 May 14, 2026

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

result.validate(full=True)
assert result.to_pylist() == [None]

result = pc.replace_with_mask(a, False, b)
assert result.type == pa.null()
result.validate(full=True)
assert result.to_pylist() == [None]

mask = pa.array([True])
result = pc.replace_with_mask(a, mask, b)
assert result.type == pa.null()
result.validate(full=True)
assert result.to_pylist() == [None]

mask = pa.array([False])
result = pc.replace_with_mask(a, mask, b)
assert result.type == pa.null()
result.validate(full=True)
assert result.to_pylist() == [None]


@pytest.mark.parametrize('arrow_type', numerical_arrow_types)
def test_fill_null_chunked_array(arrow_type):
fill_value = pa.scalar(5, type=arrow_type)
Expand Down
Loading