Skip to content

GH-50596: [C++] AppendScalar implementation uses polymorphism #50652

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
KHARSHAVARDHAN-eng wants to merge 2 commits into
base: main
Choose a base branch
from
+572 −266
Open

GH-50596: [C++] AppendScalar implementation uses polymorphism #50652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4841ad1
GH-50596: [C++] AppendScalar implementation uses polymorphism
KHARSHAVARDHAN-eng Aug 3, 2026
52809ae
GH-50596: [C++] Restore AppendScalar type validation
KHARSHAVARDHAN-eng Aug 16, 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
46 changes: 46 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 @@ -767,6 +767,52 @@ TEST_F(TestArray, TestMakeArrayFromScalar) {
}
}

TEST_F(TestArray, TestAppendScalarValidation) {
// Untyped NullScalar (Type::NA) rejected by Int32Builder
Int32Builder int_builder(pool_);
ASSERT_RAISES(Invalid, int_builder.AppendScalar(NullScalar(), 1));

// Typed null scalar accepted
auto typed_null = MakeNullScalar(int32());
ASSERT_OK(int_builder.AppendScalar(*typed_null, 1));
ASSERT_EQ(int_builder.length(), 1);
ASSERT_EQ(int_builder.null_count(), 1);

// Parameterized type mismatches
// FixedSizeBinary byte width mismatch
FixedSizeBinaryBuilder fsb_builder(fixed_size_binary(3), pool_);
FixedSizeBinaryScalar fsb_scalar(Buffer::FromString("12345"), fixed_size_binary(5));
ASSERT_RAISES(Invalid, fsb_builder.AppendScalar(fsb_scalar, 1));

// Decimal scale mismatch
Decimal128Builder dec_builder(decimal128(10, 2), pool_);
Decimal128Scalar dec_scalar(Decimal128(12345), decimal128(10, 4));
ASSERT_RAISES(Invalid, dec_builder.AppendScalar(dec_scalar, 1));

// Timestamp unit mismatch
TimestampBuilder ts_builder(timestamp(TimeUnit::MICRO), pool_);
TimestampScalar ts_scalar(100, timestamp(TimeUnit::SECOND));
ASSERT_RAISES(Invalid, ts_builder.AppendScalar(ts_scalar, 1));

// FixedSizeList size mismatch
auto fsl_type = fixed_size_list(int32(), 3);
std::unique_ptr<ArrayBuilder> fsl_builder_ptr;
ASSERT_OK(MakeBuilder(pool_, fsl_type, &fsl_builder_ptr));
Int32Builder child_builder(pool_);
ASSERT_OK(child_builder.AppendValues({1, 2, 3, 4, 5}));
ASSERT_OK_AND_ASSIGN(auto child_array, child_builder.Finish());
FixedSizeListScalar fsl_scalar(child_array, fixed_size_list(int32(), 5));
ASSERT_RAISES(Invalid, fsl_builder_ptr->AppendScalar(fsl_scalar, 1));
}

TEST_F(TestArray, TestAppendScalarsAtomicPreValidation) {
Int32Builder builder(pool_);
ScalarVector scalars = {MakeScalar<int32_t>(10), MakeScalar<int32_t>(20),
MakeScalar("string_type_mismatch")};
ASSERT_RAISES(Invalid, builder.AppendScalars(scalars));
ASSERT_EQ(builder.length(), 0);
}

TEST_F(TestArray, TestMakeArrayFromScalarSliced) {
// Regression test for ARROW-13437
auto scalars = GetScalars();
Expand Down
274 changes: 22 additions & 252 deletions cpp/src/arrow/array/builder_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,271 +73,41 @@ Status ArrayBuilder::Resize(int64_t capacity) {
return null_bitmap_builder_.Resize(capacity);
}

namespace {
namespace internal {

template <typename ConstIterator>
struct AppendScalarImpl {
template <typename T>
Status HandleFixedWidth(const T&) {
auto builder = checked_cast<typename TypeTraits<T>::BuilderType*>(builder_);
RETURN_NOT_OK(builder->Reserve(n_repeats_ * (scalars_end_ - scalars_begin_)));

for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
const auto& scalar = checked_cast<const typename TypeTraits<T>::ScalarType&>(*it);
if (scalar.is_valid) {
builder->UnsafeAppend(scalar.value);
} else {
builder->UnsafeAppendNull();
}
}
}
return Status::OK();
}

template <typename T>
enable_if_t<has_c_type<T>::value, Status> Visit(const T& t) {
return HandleFixedWidth(t);
}

Status Visit(const FixedSizeBinaryType& t) { return HandleFixedWidth(t); }
Status Visit(const Decimal32Type& t) { return HandleFixedWidth(t); }
Status Visit(const Decimal64Type& t) { return HandleFixedWidth(t); }
Status Visit(const Decimal128Type& t) { return HandleFixedWidth(t); }
Status Visit(const Decimal256Type& t) { return HandleFixedWidth(t); }

template <typename T>
enable_if_has_string_view<T, Status> Visit(const T&) {
int64_t data_size = 0;
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
const auto& scalar = checked_cast<const typename TypeTraits<T>::ScalarType&>(*it);
if (scalar.is_valid) {
data_size += scalar.value->size();
}
}

auto builder = checked_cast<typename TypeTraits<T>::BuilderType*>(builder_);
RETURN_NOT_OK(builder->Reserve(n_repeats_ * (scalars_end_ - scalars_begin_)));
RETURN_NOT_OK(builder->ReserveData(n_repeats_ * data_size));

for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
const auto& scalar = checked_cast<const typename TypeTraits<T>::ScalarType&>(*it);
if (scalar.is_valid) {
builder->UnsafeAppend(std::string_view{*scalar.value});
} else {
builder->UnsafeAppendNull();
}
}
}
return Status::OK();
}

template <typename T>
enable_if_t<is_list_view_type<T>::value || is_list_like_type<T>::value, Status> Visit(
const T&) {
auto builder = checked_cast<typename TypeTraits<T>::BuilderType*>(builder_);
int64_t num_children = 0;
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
if (!it->is_valid) continue;
num_children += checked_cast<const BaseListScalar&>(*it).value->length();
}
RETURN_NOT_OK(builder->value_builder()->Reserve(num_children * n_repeats_));

for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
if (it->is_valid) {
const Array& list = *checked_cast<const BaseListScalar&>(*it).value;
if constexpr (T::type_id == Type::MAP || T::type_id == Type::FIXED_SIZE_LIST) {
RETURN_NOT_OK(builder->Append());
} else {
RETURN_NOT_OK(builder->Append(/*is_valid=*/true, list.length()));
}
for (int64_t i = 0; i < list.length(); i++) {
ARROW_ASSIGN_OR_RAISE(auto scalar, list.GetScalar(i));
RETURN_NOT_OK(builder->value_builder()->AppendScalar(*scalar));
}
} else {
RETURN_NOT_OK(builder_->AppendNull());
}
}
}
return Status::OK();
}

Status Visit(const StructType& type) {
auto* builder = checked_cast<StructBuilder*>(builder_);
auto count = n_repeats_ * (scalars_end_ - scalars_begin_);
RETURN_NOT_OK(builder->Reserve(count));
for (int field_index = 0; field_index < type.num_fields(); ++field_index) {
RETURN_NOT_OK(builder->field_builder(field_index)->Reserve(count));
}
for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
const auto& scalar = checked_cast<const StructScalar&>(*it);
for (int field_index = 0; field_index < type.num_fields(); ++field_index) {
if (!scalar.is_valid || !scalar.value[field_index]) {
RETURN_NOT_OK(builder->field_builder(field_index)->AppendNull());
} else {
RETURN_NOT_OK(builder->field_builder(field_index)
->AppendScalar(*scalar.value[field_index]));
}
}
RETURN_NOT_OK(builder->Append(scalar.is_valid));
}
}
return Status::OK();
}

Status Visit(const SparseUnionType& type) { return MakeUnionArray(type); }

Status Visit(const DenseUnionType& type) { return MakeUnionArray(type); }

Status AppendUnionScalar(const DenseUnionType& type, const Scalar& s,
DenseUnionBuilder* builder) {
const auto& scalar = checked_cast<const DenseUnionScalar&>(s);
const auto scalar_field_index = type.child_ids()[scalar.type_code];
RETURN_NOT_OK(builder->Append(scalar.type_code));

for (int field_index = 0; field_index < type.num_fields(); ++field_index) {
auto* child_builder = builder->child_builder(field_index).get();
if (field_index == scalar_field_index) {
if (scalar.is_valid) {
RETURN_NOT_OK(child_builder->AppendScalar(*scalar.value));
} else {
RETURN_NOT_OK(child_builder->AppendNull());
}
}
}
return Status::OK();
}

Status AppendUnionScalar(const SparseUnionType& type, const Scalar& s,
SparseUnionBuilder* builder) {
// For each scalar,
// 1. append the type code,
// 2. append the value to the corresponding child,
// 3. append null to the other children.
const auto& scalar = checked_cast<const SparseUnionScalar&>(s);
RETURN_NOT_OK(builder->Append(scalar.type_code));

for (int field_index = 0; field_index < type.num_fields(); ++field_index) {
auto* child_builder = builder->child_builder(field_index).get();
if (field_index == scalar.child_id) {
if (scalar.is_valid) {
RETURN_NOT_OK(child_builder->AppendScalar(*scalar.value[field_index]));
} else {
RETURN_NOT_OK(child_builder->AppendNull());
}
} else {
RETURN_NOT_OK(child_builder->AppendNull());
}
}
return Status::OK();
}

template <typename T>
Status MakeUnionArray(const T& type) {
using BuilderType = typename TypeTraits<T>::BuilderType;

auto* builder = checked_cast<BuilderType*>(builder_);
const auto count = n_repeats_ * (scalars_end_ - scalars_begin_);

RETURN_NOT_OK(builder->Reserve(count));

DCHECK_EQ(type.num_fields(), builder->num_children());
for (int field_index = 0; field_index < type.num_fields(); ++field_index) {
RETURN_NOT_OK(builder->child_builder(field_index)->Reserve(count));
}

for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
RETURN_NOT_OK(AppendUnionScalar(type, *it, builder));
}
}
return Status::OK();
Status ValidateAppendScalar(const ArrayBuilder& builder, const Scalar& scalar) {
if (!scalar.type->Equals(*builder.type())) {
return Status::Invalid("Cannot append scalar of type ", scalar.type->ToString(),
" to builder for type ", builder.type()->ToString());
}
return Status::OK();
}

Status Visit(const RunEndEncodedType&) {
auto builder = checked_cast<RunEndEncodedBuilder*>(builder_);

RETURN_NOT_OK(builder->Reserve(n_repeats_ * (scalars_end_ - scalars_begin_)));

for (int64_t i = 0; i < n_repeats_; i++) {
for (auto it = scalars_begin_; it != scalars_end_; ++it) {
if (it->is_valid) {
const auto& scalar_value = *checked_cast<const RunEndEncodedScalar&>(*it).value;
RETURN_NOT_OK(builder->AppendScalar(scalar_value, 1));
} else {
RETURN_NOT_OK(builder_->AppendNull());
}
}
Status ValidateAppendScalars(const ArrayBuilder& builder, const ScalarVector& scalars) {
if (scalars.empty()) return Status::OK();
const auto& ty = *builder.type();
for (const auto& scalar : scalars) {
if (!scalar->type->Equals(ty)) {
return Status::Invalid("Cannot append scalar of type ", scalar->type->ToString(),
" to builder for type ", ty.ToString());
}
return Status::OK();
}

Status Visit(const DataType& type) {
return Status::NotImplemented("AppendScalar for type ", type);
}
return Status::OK();
}

Status Convert() { return VisitTypeInline(*scalars_begin_->type, this); }

ConstIterator scalars_begin_;
ConstIterator scalars_end_;
int64_t n_repeats_;
ArrayBuilder* builder_;
};

// Wraps a const_iterator that has a pointer (or pointer-like) to Scalar as the
// value_type and turns it into an iterator with Scalar as value_type.
template <typename ConstIterator>
struct DerefConstIterator {
ConstIterator it;

using value_type = Scalar;
using pointer = const Scalar*;
using difference_type = typename ConstIterator::difference_type;

const value_type& operator*() const { return *(*it); }

DerefConstIterator& operator++() {
++it;
return *this;
}

difference_type operator-(const DerefConstIterator& other) const {
return it - other.it;
}

bool operator!=(const DerefConstIterator& other) const { return it != other.it; }

pointer operator->() const { return &(**it); }
};

} // namespace
} // namespace internal

Status ArrayBuilder::AppendScalar(const Scalar& scalar, int64_t n_repeats) {
if (!scalar.type->Equals(type())) {
return Status::Invalid("Cannot append scalar of type ", scalar.type->ToString(),
" to builder for type ", type()->ToString());
}
return AppendScalarImpl<const Scalar*>{&scalar, &scalar + 1, n_repeats, this}.Convert();
ARROW_RETURN_NOT_OK(internal::ValidateAppendScalar(*this, scalar));
return Status::NotImplemented("AppendScalar for builder for ", *type());
}

Status ArrayBuilder::AppendScalars(const ScalarVector& scalars) {
if (scalars.empty()) return Status::OK();
const auto ty = type();
ARROW_RETURN_NOT_OK(internal::ValidateAppendScalars(*this, scalars));
for (const auto& scalar : scalars) {
if (!scalar->type->Equals(ty)) {
return Status::Invalid("Cannot append scalar of type ", scalar->type->ToString(),
" to builder for type ", type()->ToString());
}
RETURN_NOT_OK(AppendScalar(*scalar, 1));
}

using Iterator = DerefConstIterator<ScalarVector::const_iterator>;
return AppendScalarImpl<Iterator>{Iterator{scalars.begin()}, Iterator{scalars.end()},
/*n_repeats=*/1, this}
.Convert();
return Status::OK();
}

Status ArrayBuilder::Finish(std::shared_ptr<Array>* out) {
Expand Down
6 changes: 6 additions & 0 deletions cpp/src/arrow/array/builder_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ namespace arrow {

namespace internal {

ARROW_EXPORT
Status ValidateAppendScalar(const ArrayBuilder& builder, const Scalar& scalar);

ARROW_EXPORT
Status ValidateAppendScalars(const ArrayBuilder& builder, const ScalarVector& scalars);

template <class Builder, class V>
class ArrayBuilderExtraOps {
public:
Expand Down
Loading
Loading