Skip to content

GH-45723: [C++] FixedSizeListBuilder should have UnsafeAppend methods #46126

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
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

GH-45723: [C++] FixedSizeListBuilder should have UnsafeAppend methods #46126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a1c4cf3
Set up build environment for FixedSizeListBuilder UnsafeAppend contri…
cramosme Apr 10, 2025
3998fe9
Merge branch 'apache:main' into fixedsize-unsafe
cramosme Apr 10, 2025
30d9903
Adding the functions
cramosme Apr 10, 2025
e681165
Changed Appends in files
cramosme Apr 11, 2025
f0f057d
Change another instance in scalar_sting_ascii.cc
cramosme Apr 11, 2025
e2ea78b
Merge branch 'apache:main' into fixedsize-unsafe
cramosme Apr 13, 2025
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
14 changes: 14 additions & 0 deletions cpp/src/arrow/array/builder_nested.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,10 @@ Status FixedSizeListBuilder::Append() {
return Status::OK();
}

void FixedSizeListBuilder::UnsafeAppend() {
UnsafeAppendToBitmap(true);
}

Status FixedSizeListBuilder::AppendValues(int64_t length, const uint8_t* valid_bytes) {
RETURN_NOT_OK(Reserve(length));
UnsafeAppendToBitmap(valid_bytes, length);
Expand All @@ -216,12 +220,22 @@ Status FixedSizeListBuilder::AppendNull() {
return value_builder_->AppendNulls(list_size_);
}

void FixedSizeListBuilder::UnsafeAppendNull() {
UnsafeAppendToBitmap(false);
(void) value_builder_->AppendNulls(list_size_);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to ignore error returns, so instead we should call a hypothetical UnsafeAppendNulls(list_size_).
If such a method doesn't exist, it should be added.

}

Status FixedSizeListBuilder::AppendNulls(int64_t length) {
RETURN_NOT_OK(Reserve(length));
UnsafeAppendToBitmap(length, false);
return value_builder_->AppendNulls(list_size_ * length);
}

void FixedSizeListBuilder::UnsafeAppendNulls(int64_t length) {
UnsafeAppendToBitmap(length, false);
(void) value_builder_->AppendNulls(list_size_ * length);
}

Status FixedSizeListBuilder::ValidateOverflow(int64_t new_elements) {
auto new_length = value_builder_->length() + new_elements;
if (new_elements != list_size_) {
Expand Down
3 changes: 3 additions & 0 deletions cpp/src/arrow/array/builder_nested.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,6 +672,7 @@ class ARROW_EXPORT FixedSizeListBuilder : public ArrayBuilder {
/// using the child array builder.
Status Append();

void UnsafeAppend();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can you please add docstrings for these new methods?
  2. Ideally, the definition of the methods should be inline here, so that they can be inlined in the caller.

/// \brief Vector append
///
/// If passed, valid_bytes will be read and any zero byte
Expand All @@ -687,12 +688,14 @@ class ARROW_EXPORT FixedSizeListBuilder : public ArrayBuilder {
/// The child array builder will have the appropriate number of nulls appended
/// automatically.
Status AppendNull() final;
void UnsafeAppendNull();

/// \brief Append length null fixed length lists.
///
/// The child array builder will have the appropriate number of nulls appended
/// automatically.
Status AppendNulls(int64_t length) final;
void UnsafeAppendNulls(int64_t length);

Status ValidateOverflow(int64_t new_elements);

Expand Down
3 changes: 2 additions & 1 deletion cpp/src/arrow/compute/kernels/scalar_nested.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,8 @@ struct ListSlice {
BuilderType* out_list_builder) {
if constexpr (kIsFixedSizeOutput) {
DCHECK_EQ(out_list_builder->type()->id(), Type::FIXED_SIZE_LIST);
return out_list_builder->Append();
out_list_builder->UnsafeAppend();
return Status::OK();
} else {
return out_list_builder->Append(/*is_valid=*/true, slice_length);
}
Expand Down
4 changes: 2 additions & 2 deletions cpp/src/arrow/compute/kernels/scalar_string_ascii.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2431,9 +2431,9 @@ struct ExtractRegexSpan : ExtractRegexBase {
int64_t size = found_values[i].size();
array_builders[i]->UnsafeAppend(static_cast<OffsetCType>(begin));
array_builders[i]->UnsafeAppend(static_cast<OffsetCType>(size));
ARROW_RETURN_NOT_OK(span_builders[i]->Append());
span_builders[i]->UnsafeAppend();
} else {
ARROW_RETURN_NOT_OK(span_builders[i]->AppendNull());
span_builders[i]->UnsafeAppendNull();
}
}
ARROW_RETURN_NOT_OK(struct_builder->Append());
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/testing/fixed_width_test_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ Status NestedListGenerator::AppendNestedList(ArrayBuilder* nested_builder,
if (type->id() == Type::FIXED_SIZE_LIST) {
auto* fsl_builder = checked_cast<FixedSizeListBuilder*>(builder);
assert(list_size == checked_cast<FixedSizeListType&>(*type).list_size());
RETURN_NOT_OK(fsl_builder->Append());
fsl_builder->UnsafeAppend();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should change this one. This is a testing utility, so we value robustness over performance.

builder = fsl_builder->value_builder();
} else { // type->id() == Type::LIST)
auto* list_builder = checked_cast<ListBuilder*>(builder);
Expand Down