Skip to content

GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice #49132

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

Merged
pitrou merged 5 commits into from
Feb 18, 2026
Merged

GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice #49132

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
846af7e
GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice
abhishek593 Feb 3, 2026
8b8176a
GH-38184: [C++] Add systematic tests for Builder::AppendArraySlice
abhishek593 Feb 3, 2026
189b69b
chore: trigger CI
abhishek593 Feb 7, 2026
e87de7f
GH-38184: [C++] Refactor AppendArraySlice tests
abhishek593 Feb 17, 2026
06f00c7
GH-38184: [C++] Refactor AppendArraySlice tests
abhishek593 Feb 18, 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
133 changes: 133 additions & 0 deletions cpp/src/arrow/array/array_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
#include "arrow/array/builder_primitive.h"
#include "arrow/array/builder_run_end.h"
#include "arrow/array/builder_time.h"
#include "arrow/array/concatenate.h"
#include "arrow/array/data.h"
#include "arrow/array/util.h"
#include "arrow/buffer.h"
Expand Down Expand Up @@ -997,6 +998,138 @@ TEST_F(TestArray, TestAppendArraySlice) {
}
}

class TestBuilderAppendArraySlice : public TestArray {
public:
virtual void AssertResult(const Array& expected, const Array& actual) {
AssertArraysEqual(expected, actual, true);
}

void CheckAppendArraySlice(const std::shared_ptr<DataType>& type) {
auto rag = random::RandomArrayGenerator(0xdeadbeef);
const int64_t total_length = 100;

for (auto null_probability : {0.0, 0.1, 0.5, 1.0}) {
auto array = rag.ArrayOf(type, total_length, null_probability);

std::unique_ptr<ArrayBuilder> builder;
ASSERT_OK(MakeBuilder(pool_, type, &builder));

// Slice the array into multiple pieces
ArrayVector slices;
std::vector<int64_t> offsets = {0, 10, 10, 25, 60, 100};
for (size_t i = 0; i < offsets.size() - 1; ++i) {
int64_t start = offsets[i];
int64_t length = offsets[i + 1] - offsets[i];
auto slice = array->Slice(start, length);
slices.push_back(slice);

ArraySpan span(*slice->data());
ASSERT_OK(builder->AppendArraySlice(span, 0, slice->length()));
}

std::shared_ptr<Array> actual;
ASSERT_OK(builder->Finish(&actual));
ASSERT_OK(actual->ValidateFull());

ASSERT_OK_AND_ASSIGN(auto expected, Concatenate(slices, pool_));
AssertResult(*expected, *actual);
}
}
};

template <typename Type>
class TestBuilderAppendArraySliceTyped : public TestBuilderAppendArraySlice {};

TYPED_TEST_SUITE(TestBuilderAppendArraySliceTyped, PrimitiveArrowTypes);
TYPED_TEST(TestBuilderAppendArraySliceTyped, Primitives) {
this->CheckAppendArraySlice(TypeTraits<TypeParam>::type_singleton());
}

template <typename Type>
class TestBuilderAppendArraySliceBinary : public TestBuilderAppendArraySlice {};

using BinaryAppendArraySliceTypes =
::testing::Types<BinaryType, LargeBinaryType, StringType, LargeStringType>;
TYPED_TEST_SUITE(TestBuilderAppendArraySliceBinary, BinaryAppendArraySliceTypes);
TYPED_TEST(TestBuilderAppendArraySliceBinary, Binary) {
this->CheckAppendArraySlice(TypeTraits<TypeParam>::type_singleton());
}

@pitrou pitrou Feb 17, 2026

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.

We don't actually need templating, we can just generate the types dynamically using PrimitiveTypes(), BaseBinaryTypes() etc. This will reduce code generation and compile times slightly.

@pitrou pitrou Feb 17, 2026

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.

Besides, we should also test binary_view and string_view types.

@abhishek593 abhishek593 Feb 17, 2026

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.

Done. BaseBinaryTypes, binary_view, string_view, etc. is already present in PrimitiveTypes.

@pitrou pitrou Feb 18, 2026

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.

Thanks. Is there a particular reason for keeping the other test functions separate?

Organisation-wise, we should either have one single test function with all types, or have more granular test functions for the "dynamic" types as well (one for primitives, one for temporals etc.).

I'll leave it to your preference :-)

@abhishek593 abhishek593 Feb 18, 2026

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.

No, particular reason. I simply incorporated the feedback given previously. I agree with better organisation of tests, so I have grouped the tests better now.


TEST_F(TestBuilderAppendArraySlice, List) { CheckAppendArraySlice(list(int32())); }

TEST_F(TestBuilderAppendArraySlice, LargeList) {
CheckAppendArraySlice(large_list(int32()));
}

TEST_F(TestBuilderAppendArraySlice, ListView) {
CheckAppendArraySlice(list_view(int32()));
}

TEST_F(TestBuilderAppendArraySlice, LargeListView) {
CheckAppendArraySlice(large_list_view(int32()));
}

TEST_F(TestBuilderAppendArraySlice, FixedSizeBinary) {
CheckAppendArraySlice(fixed_size_binary(10));
}

TEST_F(TestBuilderAppendArraySlice, Decimal128) {
Comment thread
pitrou marked this conversation as resolved.
Outdated
CheckAppendArraySlice(decimal128(10, 2));
}

TEST_F(TestBuilderAppendArraySlice, Decimal256) {
CheckAppendArraySlice(decimal256(10, 2));
}

TEST_F(TestBuilderAppendArraySlice, FixedSizeList) {
CheckAppendArraySlice(fixed_size_list(int32(), 3));
}

TEST_F(TestBuilderAppendArraySlice, Struct) {
CheckAppendArraySlice(struct_({field("a", int32()), field("b", utf8())}));
}

TEST_F(TestBuilderAppendArraySlice, SparseUnion) {
CheckAppendArraySlice(sparse_union({field("a", int32()), field("b", utf8())}));
}

TEST_F(TestBuilderAppendArraySlice, DenseUnion) {
CheckAppendArraySlice(dense_union({field("a", int32()), field("b", utf8())}));
}

TEST_F(TestBuilderAppendArraySlice, RunEndEncoded) {
CheckAppendArraySlice(run_end_encoded(int32(), int32()));
Comment thread
pitrou marked this conversation as resolved.
Outdated
}

class TestBuilderAppendArraySliceDictionary : public TestBuilderAppendArraySlice {
public:
void AssertResult(const Array& expected, const Array& actual) override {
Comment thread
pitrou marked this conversation as resolved.
const auto& expected_dict = internal::checked_cast<const DictionaryArray&>(expected);
const auto& actual_dict = internal::checked_cast<const DictionaryArray&>(actual);
const auto& expected_values = *expected_dict.dictionary();
const auto& actual_values = *actual_dict.dictionary();

ASSERT_EQ(expected.length(), actual.length());
for (int64_t i = 0; i < expected.length(); ++i) {
if (expected.IsNull(i)) {
ASSERT_TRUE(actual.IsNull(i));
} else {
ASSERT_FALSE(actual.IsNull(i));
ASSERT_OK_AND_ASSIGN(auto expected_val,
expected_values.GetScalar(expected_dict.GetValueIndex(i)));
ASSERT_OK_AND_ASSIGN(auto actual_val,
actual_values.GetScalar(actual_dict.GetValueIndex(i)));
AssertScalarsEqual(*expected_val, *actual_val);
}
}
}
};

TEST_F(TestBuilderAppendArraySliceDictionary, Dictionary) {
CheckAppendArraySlice(dictionary(int8(), utf8()));
CheckAppendArraySlice(dictionary(int32(), utf8()));
}

// GH-39976: Test out-of-line data size calculation in
// BinaryViewBuilder::AppendArraySlice.
TEST_F(TestArray, TestBinaryViewAppendArraySlice) {
Expand Down
Loading