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.

Are there any user-facing changes?

Yes. 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 🙏

kou · 2026-03-08T11:56:13Z

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

      std::memcpy(out_data->buffers[1]->mutable_data(), right_offsets, offset_length);

-      auto right_data_length = right_offsets[right.length] - right_offsets[0];
+      OffsetType base = right_offsets[0];
+      auto* out_offsets =
+          reinterpret_cast<OffsetType*>(out_data->buffers[1]->mutable_data());
+      for (int64_t i = 0; i <= cond.length; ++i) {
+        out_offsets[i] = right_offsets[i] - base;
+      }


This change always (1) copies right offsets and (2) recomputes right offsets, right?

Can we use do the following?

If right array isn't sliced, we use only (1)

If right array is sliced, we use only (2)

Copilot

Pull request overview

Fixes if_else fast paths for BaseBinary arrays when one input is a null scalar and the other input is a sliced (non-zero offset) binary/utf8 array, which previously could produce invalid offsets/data.

Changes:

Adjust ASA/AAS fast paths in scalar_if_else.cc to normalize copied offsets and copy only the sliced data range.
Add a typed regression test covering sliced utf8, binary, large_utf8, and large_binary for both ASA and AAS paths.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
cpp/src/arrow/compute/kernels/scalar_if_else.cc	Fixes offset normalization and data copy base for null-scalar fast paths with sliced BaseBinary arrays.
cpp/src/arrow/compute/kernels/scalar_if_else_test.cc	Adds regression coverage for sliced BaseBinary arrays in both ASA and AAS shortcut paths.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-09T01:31:06Z

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

+      if (right.offset == 0) {
+        std::memcpy(out_data->buffers[1]->mutable_data(), right_offsets, offset_length);
+      } else {
+        OffsetType base = right_offsets[0];
+        auto* out_offsets =
+            reinterpret_cast<OffsetType*>(out_data->buffers[1]->mutable_data());
+        for (int64_t i = 0; i <= cond.length; ++i) {
+          out_offsets[i] = right_offsets[i] - base;
+        }


The decision to memcpy offsets based on right.offset == 0 is insufficient. Binary arrays can be valid with offset == 0 but right_offsets[0] != 0 (ValidateFull only requires bounds/monotonicity). In that case this branch would copy non-zero-based offsets while the data copy starts at right_data + right_offsets[0], producing invalid offsets relative to the copied data. Consider keying this optimization off right_offsets[0] == 0 (or always normalizing by subtracting right_offsets[0]) so the output offsets are consistent with the copied data in all valid inputs.

Copilot · 2026-03-09T01:31:07Z

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

+      if (left.offset == 0) {
+        std::memcpy(out_data->buffers[1]->mutable_data(), left_offsets, offset_length);
+      } else {
+        OffsetType base = left_offsets[0];
+        auto* out_offsets =
+            reinterpret_cast<OffsetType*>(out_data->buffers[1]->mutable_data());
+        for (int64_t i = 0; i <= cond.length; ++i) {
+          out_offsets[i] = left_offsets[i] - base;
+        }


Same issue as ASA path: using left.offset == 0 to decide whether to memcpy offsets can produce invalid output if left.offset == 0 but left_offsets[0] != 0. Since the data is copied starting at left_data + left_offsets[0], the output offsets should be normalized whenever left_offsets[0] is non-zero (or unconditionally), not only when the array has a non-zero element offset.

…y offset

Ebraam-Ashraf · 2026-03-09T11:30:46Z

@kou
regarding copilot's feedback applied it this is more correct since the data copy already keys on offsets[0] so the branch condition should match
I initially thought offset == 0 with offsets[0] != 0 was impossible but after checking the spec and ValidateFull() both allow it so it's possible to craft such an array manually
do you think a test is needed for this edge case or is the existing sufficient?

kou · 2026-03-10T05:46:40Z

Can we create such test data easily? If it's difficult/complex, the current test is enough.

Ebraam-Ashraf · 2026-03-10T07:44:39Z

It's doable but requires manually crafting raw buffers via ArrayData::Make() like

std::vector<OffsetType> raw_offsets = {8, 8, 9, 10};
std::string raw_data(8, 'x');
raw_data += "xx";
auto array_data = ArrayData::Make(type, 3,
    {nullptr, Buffer::Wrap(raw_offsets), data_buf}, 1, 0);

I can add it if you think it's worth covering but I think adding it not a problem

kou · 2026-03-10T08:20:59Z

OK. Could you add it?

…irst offset

Ebraam-Ashraf · 2026-03-10T09:23:31Z

added the edge case test
before fix

[  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

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

after fix

[  PASSED  ] TestIfElseBaseBinary/0.IfElseBaseBinarySliced
[  PASSED  ] TestIfElseBaseBinary/1.IfElseBaseBinarySliced
[  PASSED  ] TestIfElseBaseBinary/2.IfElseBaseBinarySliced
[  PASSED  ] TestIfElseBaseBinary/3.IfElseBaseBinarySliced
[==========] 4 tests passed.

@kou

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

kou reviewed Mar 8, 2026

View reviewed changes

apacheGH-49410: [C++] optimize offset handling

9c88e00

Ebraam-Ashraf requested a review from kou March 8, 2026 14:17

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

5979e54

kou requested a review from Copilot March 9, 2026 01:28

Copilot started reviewing on behalf of kou March 9, 2026 01:28 View session

Copilot AI reviewed Mar 9, 2026

View reviewed changes

apacheGH-49410: [C++] Key offset branch on offsets[0] instead of arra…

f31b8c0

…y offset

apacheGH-49410: [C++] add edge case test for offset=0 with non-zero f…

4303c72

…irst offset

Merge branch 'main' into apacheGH-49410-fix-if-else-sliced-string-chunks

978a85f

		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 by kou 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?

Are there any user-facing changes?

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!

kou Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Ebraam-Ashraf commented Mar 9, 2026

Uh oh!

kou commented Mar 10, 2026

Uh oh!

Ebraam-Ashraf commented Mar 10, 2026

Uh oh!

kou commented Mar 10, 2026

Uh oh!

Ebraam-Ashraf commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ebraam-Ashraf commented Mar 3, 2026 •

edited by kou

Loading

kou Mar 8, 2026 •

edited

Loading