GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into from
Jan 18, 2024
Merged

GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max) #39383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b88558b
Give explicit error in ExecBatchBuilder when appending data exceeds o…
zanmato1984 Dec 28, 2023
9797e24
Add tests
zanmato1984 Dec 28, 2023
eb91fce
Update cpp/src/arrow/compute/light_array.cc
zanmato1984 Jan 17, 2024
a421d8e
Address comments
zanmato1984 Jan 18, 2024
f9b7ff8
Address comment
zanmato1984 Jan 18, 2024
c0db9bd
Fix lint
zanmato1984 Jan 18, 2024
c1ae591
Fix compiler error
zanmato1984 Jan 18, 2024
34d2d41
Skip test on 32-bit systems
pitrou Jan 18, 2024
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
47 changes: 27 additions & 20 deletions cpp/src/arrow/compute/light_array.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

if (!column_metadata.is_fixed_length) {
int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
buffers_[kFixedLengthBuffer]->data())[num_rows_]);
int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
ARROW_DCHECK(var_len_buf_size_ > 0);
if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
while (new_size < min_new_size) {
new_size *= 2;
}
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

if (!metadata.is_fixed_length) {
const uint8_t* ptr_base = column->buffers[2]->data();
const uint32_t* offsets =
reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
const int32_t* offsets = column->GetValues<int32_t>(1);
for (int i = 0; i < num_rows; ++i) {
uint16_t row_id = row_ids[i];
const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
process_value_fn(i, field_ptr, field_length);
}
} else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
const uint8_t* field_ptr =
column->buffers[1]->data() +
(column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
process_value_fn(i, field_ptr, metadata.fixed_length);
process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));

@zanmato1984zanmato1984Jan 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.

@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.

Cool, thank you!

}
}
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
break;
case 1:
Visit(source, num_rows_to_append, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
target->mutable_data(1)[num_rows_before + i] = *ptr;
});
break;
case 2:
Visit(
source, num_rows_to_append, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
*reinterpret_cast<const uint16_t*>(ptr);
});
break;
case 4:
Visit(
source, num_rows_to_append, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
*reinterpret_cast<const uint32_t*>(ptr);
});
break;
case 8:
Visit(
source, num_rows_to_append, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
*reinterpret_cast<const uint64_t*>(ptr);
});
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
num_rows_to_append -
NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
Visit(source, num_rows_to_process, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
uint64_t* dst = reinterpret_cast<uint64_t*>(
target->mutable_data(1) +
static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
if (num_rows_to_append > num_rows_to_process) {
Visit(source, num_rows_to_append - num_rows_to_process,
row_ids + num_rows_to_process,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
uint64_t* dst = reinterpret_cast<uint64_t*>(
target->mutable_data(1) +
static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

// Step 1: calculate target offsets
//
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
Visit(source, num_rows_to_append, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
offsets[num_rows_before + i] = num_bytes;
});
for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
offsets[num_rows_before + i] = sum;
sum += length;
int32_t new_sum_maybe_overflow = 0;
if (ARROW_PREDICT_FALSE(
arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
num_rows_before + i + 1, "-th element of length ", length,
" bytes to current length ", sum, " bytes");
}
sum = new_sum_maybe_overflow;
}
offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
num_rows_to_append -
NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
Visit(source, num_rows_to_process, row_ids,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
offsets[num_rows_before + i]);
const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
}
});
Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
[&](int i, const uint8_t* ptr, uint32_t num_bytes) {
[&](int i, const uint8_t* ptr, int32_t num_bytes) {
uint64_t* dst = reinterpret_cast<uint64_t*>(
target->mutable_data(2) +
offsets[num_rows_before + num_rows_to_process + i]);
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/arrow/compute/light_array.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
MemoryPool* pool_;
int num_rows_;
int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
static constexpr int kMaxBuffers = 3;
std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down
64 changes: 64 additions & 0 deletions cpp/src/arrow/compute/light_array_test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {

@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?

@zanmato1984zanmato1984Jan 17, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Sure, will do.

@zanmato1984zanmato1984Jan 18, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done.

// GH-39332: check appending variable-length data past 2GB.
if constexpr (sizeof(void*) == 4) {
GTEST_SKIP() << "Test only works on 64-bit platforms";
}

std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
MemoryPool* pool = owned_pool.get();
constexpr auto eight_mb = 8 * 1024 * 1024;
constexpr auto eight_mb_minus_one = eight_mb - 1;
// String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
// of int32_max bytes.
std::string str_8mb(eight_mb, 'a');
// String of size (8mb - 1) to be the last element of an array of int32_max bytes.
std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
std::shared_ptr<Array> values_8mb_minus_1 =
ConstantArrayGenerator::String(1, str_8mb_minus_1);

ExecBatch batch_8mb({values_8mb}, 1);
ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
std::vector<uint16_t> body_row_ids(num_rows, 0);
std::vector<uint16_t> tail_row_id(1, 0);

{
// Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
// error of overflow.
ExecBatchBuilder builder;
ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
/*num_cols=*/1));
std::stringstream ss;
ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
<< "-th element of length " << eight_mb << " bytes to current length "
<< eight_mb * num_rows << " bytes";
ASSERT_RAISES_WITH_MESSAGE(
Invalid, ss.str(),
builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
/*num_cols=*/1));
}

{
// Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
ExecBatchBuilder builder;
ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
/*num_cols=*/1));
ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
/*num_cols=*/1));
ExecBatch built = builder.Flush();
auto datum = built[0];
ASSERT_TRUE(datum.is_array());
auto array = datum.array_as<StringArray>();
ASSERT_EQ(array->length(), num_rows + 1);
for (int i = 0; i < num_rows; ++i) {
ASSERT_EQ(array->GetString(i), str_8mb);
}
ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
ASSERT_NE(0, pool->bytes_allocated());
}

ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
ExecBatch batch =
JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down
9 changes: 8 additions & 1 deletion cpp/src/arrow/testing/generator.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
std::string value) {
return ConstantArray<StringType>(size, value);
using BuilderType = typename TypeTraits<StringType>::BuilderType;
auto type = TypeTraits<StringType>::type_singleton();
auto builder_fn = [&](BuilderType* builder) {
DCHECK_OK(builder->Append(std::string_view(value.data())));
};
return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
.ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)#39383

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 8 commits into

from

Jan 18, 2024
 Merged


GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max)
#39383
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
b88558b

Give explicit error in ExecBatchBuilder when appending data exceeds o…


zanmato1984 Dec 28, 2023
9797e24

Add tests


zanmato1984 Dec 28, 2023
eb91fce

Update cpp/src/arrow/compute/light_array.cc


zanmato1984 Jan 17, 2024
a421d8e

Address comments


zanmato1984 Jan 18, 2024
f9b7ff8

Address comment


zanmato1984 Jan 18, 2024
c0db9bd

Fix lint


zanmato1984 Jan 18, 2024
c1ae591

Fix compiler error


zanmato1984 Jan 18, 2024
34d2d41

Skip test on 32-bit systems


pitrou Jan 18, 2024

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


47 changes: 27 additions & 20 deletions
cpp/src/arrow/compute/light_array.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -20,6 +20,8 @@
#include <type_traits>

#include "arrow/util/bitmap_ops.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/macros.h"

namespace arrow {
namespace compute {
Expand DownExpand Up@@ -325,11 +327,10 @@ Status ResizableArrayData::ResizeVaryingLengthBuffer() {
 column_metadata = ColumnMetadataFromDataType(data_type_).ValueOrDie();

 if (!column_metadata.is_fixed_length) {
 int min_new_size = static_cast<int>(reinterpret_cast<const uint32_t*>(
 buffers_[kFixedLengthBuffer]->data())[num_rows_]);
 int64_t min_new_size = buffers_[kFixedLengthBuffer]->data_as<int32_t>()[num_rows_];
 ARROW_DCHECK(var_len_buf_size_ > 0);
 if (var_len_buf_size_ < min_new_size) {
int new_size = var_len_buf_size_;
int64_t new_size = var_len_buf_size_;
 while (new_size < min_new_size) {
 new_size *= 2;
 }
Expand DownExpand Up@@ -465,12 +466,11 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r

 if (!metadata.is_fixed_length) {
 const uint8_t* ptr_base = column->buffers[2]->data();
 const uint32_t* offsets =
 reinterpret_cast<const uint32_t*>(column->buffers[1]->data()) + column->offset;
 const int32_t* offsets = column->GetValues<int32_t>(1);
 for (int i = 0; i < num_rows; ++i) {
 uint16_t row_id = row_ids[i];
 const uint8_t* field_ptr = ptr_base + offsets[row_id];
uint32_t field_length = offsets[row_id + 1] - offsets[row_id];
int32_t field_length = offsets[row_id + 1] - offsets[row_id];
 process_value_fn(i, field_ptr, field_length);
 }
 } else {
Expand All@@ -480,7 +480,7 @@ void ExecBatchBuilder::Visit(const std::shared_ptr<ArrayData>& column, int num_r
 const uint8_t* field_ptr =
 column->buffers[1]->data() +
 (column->offset + row_id) * static_cast<int64_t>(metadata.fixed_length);
 process_value_fn(i, field_ptr, metadata.fixed_length);
 process_value_fn(i, field_ptr, static_cast<int32_t>(metadata.fixed_length));
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I wanted to change the type of metadata.fixed_length to int32_t, but that would bring big amount related changes overwhelming to this small PR. So I tend to leave it as is and do a simple cast here. Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
@pitroupitrouJan 18, 2024

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.

Changing fixed_length of RowTableMetadata and KeyColumnMetaData to signed type could be a future enhancement.
Cool, thank you!
 }
 }
}
Expand DownExpand Up@@ -511,30 +511,30 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 break;
 case 1:
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 target->mutable_data(1)[num_rows_before + i] = *ptr;
 });
 break;
 case 2:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint16_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint16_t*>(ptr);
 });
 break;
 case 4:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint32_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint32_t*>(ptr);
 });
 break;
 case 8:
 Visit(
 source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 reinterpret_cast<uint64_t*>(target->mutable_data(1))[num_rows_before + i] =
 *reinterpret_cast<const uint64_t*>(ptr);
 });
Expand All@@ -544,7 +544,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) * (num_rows_before + i));
Expand All@@ -558,7 +558,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 if (num_rows_to_append > num_rows_to_process) {
 Visit(source, num_rows_to_append - num_rows_to_process,
 row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(1) +
 static_cast<int64_t>(num_bytes) *
Expand All@@ -575,16 +575,23 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source

 // Step 1: calculate target offsets
 //
uint32_t* offsets = reinterpret_cast<uint32_t*>(target->mutable_data(1));
uint32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
int32_t* offsets = reinterpret_cast<int32_t*>(target->mutable_data(1));
int32_t sum = num_rows_before == 0 ? 0 : offsets[num_rows_before];
 Visit(source, num_rows_to_append, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 offsets[num_rows_before + i] = num_bytes;
 });
 for (int i = 0; i < num_rows_to_append; ++i) {
uint32_t length = offsets[num_rows_before + i];
int32_t length = offsets[num_rows_before + i];
 offsets[num_rows_before + i] = sum;
 sum += length;
 int32_t new_sum_maybe_overflow = 0;
 if (ARROW_PREDICT_FALSE(
 arrow::internal::AddWithOverflow(sum, length, &new_sum_maybe_overflow))) {
 return Status::Invalid("Overflow detected in ExecBatchBuilder when appending ",
 num_rows_before + i + 1, "-th element of length ", length,
 " bytes to current length ", sum, " bytes");
 }
 sum = new_sum_maybe_overflow;
 }
 offsets[num_rows_before + num_rows_to_append] = sum;

Expand All@@ -598,7 +605,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 num_rows_to_append -
 NumRowsToSkip(source, num_rows_to_append, row_ids, sizeof(uint64_t));
 Visit(source, num_rows_to_process, row_ids,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(target->mutable_data(2) +
 offsets[num_rows_before + i]);
 const uint64_t* src = reinterpret_cast<const uint64_t*>(ptr);
Expand All@@ -608,7 +615,7 @@ Status ExecBatchBuilder::AppendSelected(const std::shared_ptr<ArrayData>& source
 }
 });
 Visit(source, num_rows_to_append - num_rows_to_process, row_ids + num_rows_to_process,
 [&](int i, const uint8_t* ptr, uint32_t num_bytes) {
 [&](int i, const uint8_t* ptr, int32_t num_bytes) {
 uint64_t* dst = reinterpret_cast<uint64_t*>(
 target->mutable_data(2) +
 offsets[num_rows_before + num_rows_to_process + i]);
Expand Down

2 changes: 1 addition & 1 deletion
cpp/src/arrow/compute/light_array.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -353,7 +353,7 @@ class ARROW_EXPORT ResizableArrayData {
 MemoryPool* pool_;
 int num_rows_;
 int num_rows_allocated_;
int var_len_buf_size_;
int64_t var_len_buf_size_;
 static constexpr int kMaxBuffers = 3;
 std::shared_ptr<ResizableBuffer> buffers_[kMaxBuffers];
};
Expand Down

64 changes: 64 additions & 0 deletions
cpp/src/arrow/compute/light_array_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -407,6 +407,70 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {
@pitroupitrouJan 17, 2024

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.

Can you add a comment referring to the GH issue?
@zanmato1984zanmato1984Jan 17, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Sure, will do.
@zanmato1984zanmato1984Jan 18, 2024

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Done.
 // GH-39332: check appending variable-length data past 2GB.
 if constexpr (sizeof(void*) == 4) {
 GTEST_SKIP() << "Test only works on 64-bit platforms";
 }

 std::unique_ptr<MemoryPool> owned_pool = MemoryPool::CreateDefault();
 MemoryPool* pool = owned_pool.get();
 constexpr auto eight_mb = 8 * 1024 * 1024;
 constexpr auto eight_mb_minus_one = eight_mb - 1;
 // String of size 8mb to repetitively fill the heading multiple of 8mbs of an array
 // of int32_max bytes.
 std::string str_8mb(eight_mb, 'a');
 // String of size (8mb - 1) to be the last element of an array of int32_max bytes.
 std::string str_8mb_minus_1(eight_mb_minus_one, 'b');
 std::shared_ptr<Array> values_8mb = ConstantArrayGenerator::String(1, str_8mb);
 std::shared_ptr<Array> values_8mb_minus_1 =
 ConstantArrayGenerator::String(1, str_8mb_minus_1);

 ExecBatch batch_8mb({values_8mb}, 1);
 ExecBatch batch_8mb_minus_1({values_8mb_minus_1}, 1);

 auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
 std::vector<uint16_t> body_row_ids(num_rows, 0);
 std::vector<uint16_t> tail_row_id(1, 0);

 {
 // Building an array of (int32_max + 1) = (8mb * num_rows + 8mb) bytes should raise an
 // error of overflow.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 std::stringstream ss;
 ss << "Invalid: Overflow detected in ExecBatchBuilder when appending " << num_rows + 1
 << "-th element of length " << eight_mb << " bytes to current length "
 << eight_mb * num_rows << " bytes";
 ASSERT_RAISES_WITH_MESSAGE(
 Invalid, ss.str(),
 builder.AppendSelected(pool, batch_8mb, 1, tail_row_id.data(),
 /*num_cols=*/1));
 }

 {
 // Building an array of int32_max = (8mb * num_rows + 8mb - 1) bytes should succeed.
 ExecBatchBuilder builder;
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, body_row_ids.data(),
 /*num_cols=*/1));
 ASSERT_OK(builder.AppendSelected(pool, batch_8mb_minus_1, 1, tail_row_id.data(),
 /*num_cols=*/1));
 ExecBatch built = builder.Flush();
 auto datum = built[0];
 ASSERT_TRUE(datum.is_array());
 auto array = datum.array_as<StringArray>();
 ASSERT_EQ(array->length(), num_rows + 1);
 for (int i = 0; i < num_rows; ++i) {
 ASSERT_EQ(array->GetString(i), str_8mb);
 }
 ASSERT_EQ(array->GetString(num_rows), str_8mb_minus_1);
 ASSERT_NE(0, pool->bytes_allocated());
 }

 ASSERT_EQ(0, pool->bytes_allocated());
}

TEST(KeyColumnArray, FromExecBatch) {
 ExecBatch batch =
 JSONToExecBatch({int64(), boolean()}, "[[1, true], [2, false], [null, null]]");
Expand Down

9 changes: 8 additions & 1 deletion
cpp/src/arrow/testing/generator.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -38,6 +38,7 @@
#include "arrow/type.h"
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/logging.h"
#include "arrow/util/macros.h"
#include "arrow/util/string.h"

Expand DownExpand Up@@ -103,7 +104,13 @@ std::shared_ptr<arrow::Array> ConstantArrayGenerator::Float64(int64_t size,

std::shared_ptr<arrow::Array> ConstantArrayGenerator::String(int64_t size,
 std::string value) {
 return ConstantArray<StringType>(size, value);
 using BuilderType = typename TypeTraits<StringType>::BuilderType;
 auto type = TypeTraits<StringType>::type_singleton();
 auto builder_fn = [&](BuilderType* builder) {
 DCHECK_OK(builder->Append(std::string_view(value.data())));
 };
 return ArrayFromBuilderVisitor(type, value.size() * size, size, builder_fn)
 .ValueOrDie();
}

std::shared_ptr<arrow::Array> ConstantArrayGenerator::Zeroes(
Expand Down