GH-49410: [C++] Add regression test for if_else with sliced BaseBinary chunks#49443
GH-49410: [C++] Add regression test for if_else with sliced BaseBinary chunks#49443Ebraam-Ashraf wants to merge 2 commits intoapache:mainfrom
Conversation
|
|
|
hi @kou |
| {ArrayFromJSON(int64(), "[-1]"), ArrayFromJSON(int32(), "[0]")})); | ||
| } | ||
|
|
||
| TEST_F(TestIfElseKernel, IfElseBaseBinarySlicedChunk) { |
There was a problem hiding this comment.
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?
| } | ||
|
|
||
| TEST_F(TestIfElseKernel, IfElseBaseBinarySlicedChunk) { | ||
| for (auto type : {utf8(), binary(), large_utf8(), large_binary()}) { |
There was a problem hiding this comment.
Do we need to test all these types?
If so, TYPED_TEST(TestIfElseBaseBinary, ...) may be better than TEST_F(TestifEleseKernel, ...).
| 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); |
There was a problem hiding this comment.
We can reproduce this problem without chunked array, right?
If so, we don't need to test chunked array.
|
thanks for the feedback @kou I switched from TEST_F(TestIfElseKernel, ...) to TYPED_TEST(TestIfElseBaseBinary, IfElseBaseBinarySliced) |
2 participants
Rationale for this change
if_elsewith a null scalar and a sliced BaseBinary array (offset != 0) produces invalid output. The ASA and AAS shortcut paths inscalar_if_else.cccopy offsets without adjusting for the slice offset, and copy data from byte 0 instead ofdata + offsets[0].all existing BaseBinary tests use arrays built directly from
ArrayFromJSONwhere offset is always 0.A proposed fix (adjusting offsets by
offsets[0]and copying data fromdata + offsets[0]in both the ASA and AAS paths) is ready and will be added in a follow-up commit once this test is reviewed and confirmed correct.What changes are included in this PR?
A regression test
IfElseBaseBinarySlicedChunkthat reproduces the bug acrossutf8,binary,large_utf8, andlarge_binarytypes, covering the ASA path, AAS path, and the full round-trip from the original issue report.The test currently fails:
Are these changes tested?
Yes. The new test reproduces the bug. Fix will follow in a separate commit.
This PR contains a "Critical Fix". The bug causes incorrect/invalid data to be produced when
if_elseis called with a null scalar and a sliced BaseBinary array.