GH-49410: [C++] Fix if_else null-scalar fast paths for sliced BaseBinary arrays by Ebraam-Ashraf · Pull Request #49443 · apache/arrow

Ebraam-Ashraf · 2026-03-03T21:45:43Z

Rationale for this change

if_else with a null scalar and a sliced BaseBinary array (offset != 0) produces invalid output. The ASA and AAS shortcut paths in scalar_if_else.cc copy offsets without adjusting for the slice offset, and copy data from byte 0 instead of data + offsets[0].

What changes are included in this PR?

Fix (scalar_if_else.cc): In both the ASA and AAS fast paths:

Replace the single memcpy for the offsets buffer with a loop that normalizes each offset by subtracting offsets[0] as a base, so output offsets always start at 0.
Change the data copy to start from data + base instead of data, so only the bytes belonging to the slice are copied.

Regression test (scalar_if_else_test.cc): Added TYPED_TEST(TestIfElseBaseBinary, IfElseBaseBinarySliced) covering utf8, binary, large_utf8, and large_binary types, testing both the ASA path and AAS path with a sliced array.

Are these changes tested?

Yes. The new IfElseBaseBinarySliced typed test reproduces the bug and passes after the fix. All 416 existing tests continue to pass.

This PR contains a "Critical Fix". The bug causes incorrect/invalid data to be produced when if_else is called with a null scalar and a sliced BaseBinary array.

GitHub Issue: [C++][Python] if_else kernel creates invalid results with chunked array string inputs with non-aligned chunks #49410

…eBinary chunks

github-actions · 2026-03-03T21:46:11Z

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

Ebraam-Ashraf · 2026-03-03T21:49:29Z

hi @kou
thanks for your guidance
as requested here's the test that reproduces the bug
Looking forward to any feedback also I'll add the fix in a follow up commit once you confirm the test is fine

kou · 2026-03-04T01:08:07Z

cpp/src/arrow/compute/kernels/scalar_if_else_test.cc

                   {ArrayFromJSON(int64(), "[-1]"), ArrayFromJSON(int32(), "[0]")}));
 }

+TEST_F(TestIfElseKernel, IfElseBaseBinarySlicedChunk) {


Could you move this TestIfEleseKernel test to the location where other TestIfEleseKernel tests exist instead of adding it after the TestChooseKernel test?

Can we use better name than IfElseBaseBinarySlicedChunk like other existing test names?

kou · 2026-03-04T01:10:05Z

cpp/src/arrow/compute/kernels/scalar_if_else_test.cc

 }

+TEST_F(TestIfElseKernel, IfElseBaseBinarySlicedChunk) {
+  for (auto type : {utf8(), binary(), large_utf8(), large_binary()}) {


Do we need to test all these types?

If so, TYPED_TEST(TestIfElseBaseBinary, ...) may be better than TEST_F(TestifEleseKernel, ...).

kou · 2026-03-04T01:43:35Z

cpp/src/arrow/compute/kernels/scalar_if_else_test.cc

+    auto cond_asa = ArrayFromJSON(boolean(), "[true, false, false]");
+    ASSERT_OK_AND_ASSIGN(auto result_asa,
+                        CallFunction("if_else", {cond_asa, MakeNullScalar(type), chunk1}));
+    ASSERT_OK(result_asa.make_array()->ValidateFull());
+    AssertArraysEqual(*ArrayFromJSON(type, R"([null, "x", "x"])"),
+                      *result_asa.make_array(), true);
+
+    auto cond_aas = ArrayFromJSON(boolean(), "[false, true, true]");
+    ASSERT_OK_AND_ASSIGN(auto result_aas,
+                        CallFunction("if_else", {cond_aas, chunk1, MakeNullScalar(type)}));
+    ASSERT_OK(result_aas.make_array()->ValidateFull());
+    AssertArraysEqual(*ArrayFromJSON(type, R"([null, "x", "x"])"),
+                      *result_aas.make_array(), true);
+
+    auto arr1 = std::make_shared<ChunkedArray>(ArrayVector{chunk0, chunk1});
+    auto mask = *CallFunction("is_null", {arr1});
+    ASSERT_OK_AND_ASSIGN(auto arr2_datum,
+                        CallFunction("if_else", {Datum(true), *Concatenate(arr1->chunks()), arr1}));
+    ASSERT_OK(arr2_datum.chunked_array()->ValidateFull());
+    ASSERT_OK_AND_ASSIGN(auto arr3_datum,
+                        CallFunction("if_else", {mask, MakeNullScalar(type), arr2_datum}));
+    ASSERT_OK(arr3_datum.chunked_array()->ValidateFull());
+    AssertDatumsEqual(Datum(arr1), arr3_datum);


We can reproduce this problem without chunked array, right?

If so, we don't need to test chunked array.

Ebraam-Ashraf · 2026-03-04T19:23:56Z

thanks for the feedback @kou

I switched from TEST_F(TestIfElseKernel, ...) to TYPED_TEST(TestIfElseBaseBinary, IfElseBaseBinarySliced)
and moved the test to its right place
also removed the chunked array portion

[----------] Global test environment tear-down
[==========] 4 tests from 4 test suites ran. (125 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] TestIfElseBaseBinary/0.IfElseBaseBinarySliced, where TypeParam = arrow::BinaryType
[  FAILED  ] TestIfElseBaseBinary/1.IfElseBaseBinarySliced, where TypeParam = arrow::LargeBinaryType
[  FAILED  ] TestIfElseBaseBinary/2.IfElseBaseBinarySliced, where TypeParam = arrow::StringType
[  FAILED  ] TestIfElseBaseBinary/3.IfElseBaseBinarySliced, where TypeParam = arrow::LargeStringType

Offset invariant failure: offset for slot 2 out of bounds: 3 > 2

kou

Thanks.

Could you push a fix for this problem to this branch?

kou · 2026-03-05T02:52:53Z

cpp/src/arrow/compute/kernels/scalar_if_else_test.cc

+  auto full_arr = ArrayFromJSON(type, R"([null, "x", "x", null, "x", "x"])");
+  auto sliced = full_arr->Slice(3);


Can we simplify test data?

Suggested change

auto full_arr = ArrayFromJSON(type, R"([null, "x", "x", null, "x", "x"])");

auto sliced = full_arr->Slice(3);

auto full_arr = ArrayFromJSON(type, R"(["not used", null, "x", "x"])");

auto sliced = full_arr->Slice(1);

…aseBinary arrays

Ebraam-Ashraf · 2026-03-05T15:07:46Z

Tests before fix

[   RUN    ] TestIfElseBaseBinary/0.IfElseBaseBinarySliced
[  FAILED  ] TestIfElseBaseBinary/0.IfElseBaseBinarySliced (20 ms)
[  FAILED  ] TestIfElseBaseBinary/1.IfElseBaseBinarySliced (1 ms)
[  FAILED  ] TestIfElseBaseBinary/2.IfElseBaseBinarySliced (1 ms)
[  FAILED  ] TestIfElseBaseBinary/3.IfElseBaseBinarySliced (0 ms)
[  PASSED  ] 0 tests.
[  FAILED  ] 4 tests.

Invalid: Offset invariant failure: offset for slot 1 out of bounds: 8 > 2

What the fix does

Replaced the single memcpy for the offsets buffer with a loop that subtracts offsets[0] as a base from each value
so the output offsets are always normalized to start at 0.

Then changed the data copy to start from data + base instead of data
so only the bytes belonging to the slice are copied.

The same two changes are applied symmetrically in both the ASA and AAS paths.

Tests after fix

[==========] 416 tests from 132 test suites ran. (31991 ms total)
[  PASSED  ] 416 tests.

Ebraam-Ashraf · 2026-03-07T05:51:13Z

@kou

kou · 2026-03-07T06:34:39Z

Could you fix the lint failure and update the PR title and description?

Ebraam-Ashraf · 2026-03-07T07:42:54Z

thanks @kou
I really appreciate your patience and guidance throughout this PR
looking forward to your review 🙏

apacheGH-49410: [C++] Add regression test for if_else with sliced Bas…

daac36e

…eBinary chunks

github-actions bot added Component: C++ awaiting review Awaiting review labels Mar 3, 2026

kou reviewed Mar 4, 2026

View reviewed changes

apacheGH-49410: [C++] Use TYPED_TEST and remove chunked array test

95b8bf8

kou reviewed Mar 5, 2026

View reviewed changes

apacheGH-49410: [C++] Fix if_else null-scalar fast paths for sliced B…

9f2edbc

…aseBinary arrays

Ebraam-Ashraf requested a review from kou March 6, 2026 07:13

apacheGH-49410: [C++] Fix clang-format style

a8b0515

Ebraam-Ashraf changed the title ~~GH-49410: [C++] Add regression test for if_else with sliced BaseBinary chunks~~ GH-49410: [C++] Fix if_else null-scalar fast paths for sliced BaseBinary arrays Mar 7, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-49410: [C++] Fix if_else null-scalar fast paths for sliced BaseBinary arrays#49443

GH-49410: [C++] Fix if_else null-scalar fast paths for sliced BaseBinary arrays#49443
Ebraam-Ashraf wants to merge 4 commits intoapache:mainfrom
Ebraam-Ashraf:GH-49410-fix-if-else-sliced-string-chunks

Ebraam-Ashraf commented Mar 3, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Ebraam-Ashraf commented Mar 3, 2026

Uh oh!

kou Mar 4, 2026

Uh oh!

kou Mar 4, 2026

Uh oh!

kou Mar 4, 2026

Uh oh!

Ebraam-Ashraf commented Mar 4, 2026

Uh oh!

kou left a comment

Uh oh!

kou Mar 5, 2026

Uh oh!

Ebraam-Ashraf commented Mar 5, 2026

Uh oh!

Ebraam-Ashraf commented Mar 7, 2026

Uh oh!

kou commented Mar 7, 2026

Uh oh!

Ebraam-Ashraf commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		auto full_arr = ArrayFromJSON(type, R"([null, "x", "x", null, "x", "x"])");
		auto sliced = full_arr->Slice(3);

Conversation

Ebraam-Ashraf commented Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

Ebraam-Ashraf commented Mar 3, 2026

Uh oh!

kou Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

kou Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

kou Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Ebraam-Ashraf commented Mar 4, 2026

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

kou Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Ebraam-Ashraf commented Mar 5, 2026

Uh oh!

Ebraam-Ashraf commented Mar 7, 2026

Uh oh!

kou commented Mar 7, 2026

Uh oh!

Ebraam-Ashraf commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Ebraam-Ashraf commented Mar 3, 2026 •

edited

Loading