Copilot commented on code in PR #50652:
URL: https://github.com/apache/arrow/pull/50652#discussion_r3779159843
##########
cpp/src/arrow/array/builder_base.cc:
##########
@@ -73,271 +73,23 @@ Status ArrayBuilder::Resize(int64_t capacity) {
return null_bitmap_builder_.Resize(capacity);
}
-namespace {
-
-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 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());
- }
- }
- }
- return Status::OK();
- }
-
- Status Visit(const DataType& type) {
- return Status::NotImplemented("AppendScalar for type ", type);
- }
-
- 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
-
Status ArrayBuilder::AppendScalar(const Scalar& scalar, int64_t n_repeats) {
- if (!scalar.type->Equals(type())) {
+ if (scalar.type->id() == Type::NA) {
+ return AppendNulls(n_repeats);
+ }
+ if (scalar.type->id() != type()->id()) {
return Status::Invalid("Cannot append scalar of type ",
scalar.type->ToString(),
Review Comment:
ArrayBuilder::AppendScalar now special-cases Type::NA and otherwise compares
only type ids. That relaxes the previous contract (which required type
equality) and is unsafe for parameterized types (e.g., Decimal precision/scale,
FixedSizeBinary byte_width, Timestamp timezone, nested/union children),
potentially leading to silent data corruption or inconsistent builder state. It
also changes behavior by allowing a NullScalar to append nulls to any builder.
Please restore strict type equality here (and align the new per-builder
overrides in this PR to use Equals as well).
##########
cpp/src/arrow/array/builder_base.cc:
##########
@@ -73,271 +73,23 @@ Status ArrayBuilder::Resize(int64_t capacity) {
return null_bitmap_builder_.Resize(capacity);
}
-namespace {
-
-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 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());
- }
- }
- }
- return Status::OK();
- }
-
- Status Visit(const DataType& type) {
- return Status::NotImplemented("AppendScalar for type ", type);
- }
-
- 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
-
Status ArrayBuilder::AppendScalar(const Scalar& scalar, int64_t n_repeats) {
- if (!scalar.type->Equals(type())) {
+ if (scalar.type->id() == Type::NA) {
+ return AppendNulls(n_repeats);
+ }
+ if (scalar.type->id() != type()->id()) {
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();
+ return Status::NotImplemented("AppendScalar for builder for ", *type());
}
Status ArrayBuilder::AppendScalars(const ScalarVector& scalars) {
if (scalars.empty()) return Status::OK();
- const auto ty = 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 ", 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();
Review Comment:
ArrayBuilder::AppendScalars now appends sequentially via AppendScalar
without pre-validating the entire input. This can leave the builder partially
modified if a later scalar has a mismatched type (or the derived AppendScalar
returns NotImplemented), whereas the previous implementation rejected mixed
types before mutating. Consider reintroducing a pre-validation pass (and then
append) to keep failure behavior consistent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
