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

zanmato1984 · 2023-12-28T03:41:35Z

Rationale for this change

When appending var length data in ExecBatchBuilder, the offset is potentially to overflow if the batch contains 4GB data or more. This may further result in segmentation fault during the subsequent data content copying. For details, please refer to this comment: #39332 (comment).

The solution is let user to use the "large" counterpart data type to avoid the overflow, but we may need explicit error information when such overflow happens.

What changes are included in this PR?

Detect the offset overflow in appending data in ExecBatchBuilder and explicitly throw.
Change the offset type from uint32_t to int32_t in ExecBatchBuilder and respects the BinaryBuilder::memory_limit() which is 2GB - 2B as the rest part of the codebase.

Are these changes tested?

UT included.

Are there any user-facing changes?

No.

Closes: SIGSEGV during join on large table #39332

github-actions · 2023-12-28T03:42:03Z

⚠️ GitHub issue #39332 has been automatically assigned in GitHub to PR creator.

pitrou · 2024-01-17T15:11:38Z

Thanks for the PR @zanmato1984 . I would ask you to rebase this once #39585 is merged.

zanmato1984 · 2024-01-17T16:33:59Z

Thanks for the PR @zanmato1984 . I would ask you to rebase this once #39585 is merged.

Thanks for looking. Rebase done.

cpp/src/arrow/compute/light_array.cc

pitrou · 2024-01-17T16:48:35Z

cpp/src/arrow/compute/light_array.cc

+      int64_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) {
+              offsets[num_rows_before + i] = num_bytes;


Is there a possibility for num_bytes to be greater than std::numeric_limits<int32_t>::max()?

Perhaps we should change Visit to pass a int32_t num_bytes instead.

If source is a well-constructed array, then num_bytes shouldn't be greater than int32 max.

But yeah, changing num_bytes to int32_t will be more consistent with other part. I'll change it.

pitrou · 2024-01-17T16:50:31Z

cpp/src/arrow/compute/light_array.cc

+      for (int i = 0; i < num_rows_to_append; ++i) {
+        int32_t length = offsets[num_rows_before + i];


For the record, I find it amusing that we're looping over the offsets a second time here. Perhaps Visit for variable-length types isn't useful?

I'm sorry I don't quite follow. Could you elaborate more on this? Thanks.

pitrou · 2024-01-17T16:52:12Z

cpp/src/arrow/compute/light_array.cc

+        offsets[num_rows_before + i] = static_cast<int32_t>(sum);
+        if (ARROW_PREDICT_FALSE(sum + length > BinaryBuilder::memory_limit())) {


This is correct, but an alternative would be to use AddWithOverflow instead of a 64-bit addition. Both will probably have the same performance, of course.

Good to know there is such handy function!

pitrou · 2024-01-17T16:52:50Z

cpp/src/arrow/compute/light_array.cc

@@ -19,7 +19,9 @@

 #include <type_traits>

+#include "arrow/array/builder_binary.h"


It's a bit overkill to include this just for the binary size limit. You can instead use std::numeric_limits<int32_t>::max().

Well, there is a pitfall about BaseBinaryBuilder::memory_limit() - it's std::numeric_limits<int32_t>::max() - 1. I only noticed this til my initial test failed. So I thought it might be necessary to keep using BaseBinaryBuilder::memory_limit() to avoid the potential mismatch. Does that make sense?

Hmm, it might be an artifact of how BinaryBuilder is implemented, but it is not being used here.

OK, I got the point. They are indeed not necessarily related. I'll update.

pitrou · 2024-01-17T16:56:03Z

cpp/src/arrow/compute/light_array_test.cc

+
+  auto num_rows = std::numeric_limits<int32_t>::max() / eight_mb;
+  StringBuilder values_ref_builder;
+  ASSERT_OK(values_ref_builder.Reserve(num_rows + 1));


You could also call ReserveData to avoid cascading resizes to grow the data buffer.

Good to know! Thanks for pointing this out. I'll change it.

pitrou · 2024-01-17T16:57:20Z

cpp/src/arrow/compute/light_array_test.cc

@@ -407,6 +408,63 @@ TEST(ExecBatchBuilder, AppendValuesBeyondLimit) {
  ASSERT_EQ(0, pool->bytes_allocated());
 }

+TEST(ExecBatchBuilder, AppendVarLengthBeyondLimit) {


Can you add a comment referring to the GH issue?

Sure, will do.

pitrou · 2024-01-17T16:58:00Z

cpp/src/arrow/compute/light_array_test.cc

+  std::string str_8mb(eight_mb, 'a');
+  std::string str_8mb_minus_1(eight_mb - 1, 'b');
+  std::string str_8mb_minus_2(eight_mb - 2,
+                              'b');  // BaseBinaryBuilder::memory_limit()


I'm not sure what this string has to do with BaseBinaryBuilder::memory_limit()?

I was trying to use this string to be the last element of an array that occupies exactly memory_limit() bytes. Unfortunately it is not a good comment. My bad. I'll change it.

pitrou · 2024-01-17T17:00:13Z

cpp/src/arrow/compute/light_array_test.cc

+    ExecBatchBuilder builder;
+    ASSERT_OK(builder.AppendSelected(pool, batch_8mb, num_rows, first_set_row_ids.data(),
+                                     /*num_cols=*/1));
+    ASSERT_RAISES(Invalid, builder.AppendSelected(pool, batch_8mb_minus_1, 1,


You could perhaps use ASSERT_RAISES_WITH_MESSAGE to make sure that the error message is as expected.

Good to know! I'll change it.

pitrou · 2024-01-17T17:03:54Z

cpp/src/arrow/compute/light_array_test.cc

+  ASSERT_OK(values_ref_builder.Append(str_8mb_minus_2));
+  ASSERT_OK_AND_ASSIGN(auto values_ref, values_ref_builder.Finish());
+  auto values_ref_1 = values_ref->Slice(0, num_rows);
+  ExecBatch batch_ref_1({values_ref_1}, num_rows);


This one isn't used, is it?

Sorry, my bad. Thanks for pointing this out.

zanmato1984 · 2024-01-17T19:29:22Z

Thank you @pitrou so much for the comprehensive review comments! Learned a lot. I will make the change later.

zanmato1984 · 2024-01-18T07:34:00Z

cpp/src/arrow/compute/light_array.cc

@@ -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));


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.

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

Cool, thank you!

pitrou · 2024-01-18T09:10:06Z

A couple compiler errors appeared on CI, can you take a look?

zanmato1984 · 2024-01-18T09:19:39Z

A couple compiler errors appeared on CI, can you take a look?

A namespace resolving issue. Fixed.

pitrou · 2024-01-18T09:25:03Z

@github-actions crossbow submit -g cpp

…ffset limit (int32 max)

Co-authored-by: Antoine Pitrou <[email protected]>

zanmato1984 · 2024-01-18T09:43:38Z

I saw the test failed by OOM in this run because the test would use about 2~3GB RAM. Will this be an issue?

pitrou · 2024-01-18T09:54:52Z

It's not an OOM, it's a 32-bit build that cannot go past 2GB of memory per process.

pitrou · 2024-01-18T09:55:04Z

@github-actions crossbow submit -g cpp

zanmato1984 · 2024-01-18T09:57:04Z

It's not an OOM, it's a 32-bit build that cannot go past 2GB of memory per process.

Got it. And thanks for the modification to the test code.

github-actions · 2024-01-18T09:57:32Z

Revision: 34d2d41

Submitted crossbow builds: ursacomputing/crossbow @ actions-9312816a35

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-fedora-38-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading

pitrou · 2024-01-18T11:44:09Z

Ok, the remaining CI failure is unrelated, so I'll merge. Thanks again for this @zanmato1984 !

pitrou · 2024-01-18T11:44:58Z

@raulcd This is another candidate for 15.0.0 if you cut another RC.

conbench-apache-arrow · 2024-01-18T17:54:38Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a2aa1c4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

…ng var length data exceeds offset limit (int32 max) (apache#39383) ### Rationale for this change When appending var length data in `ExecBatchBuilder`, the offset is potentially to overflow if the batch contains 4GB data or more. This may further result in segmentation fault during the subsequent data content copying. For details, please refer to this comment: apache#39332 (comment). The solution is let user to use the "large" counterpart data type to avoid the overflow, but we may need explicit error information when such overflow happens. ### What changes are included in this PR? 1. Detect the offset overflow in appending data in `ExecBatchBuilder` and explicitly throw. 2. Change the offset type from `uint32_t` to `int32_t` in `ExecBatchBuilder` and respects the `BinaryBuilder::memory_limit()` which is `2GB - 2B` as the rest part of the codebase. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: apache#39332 Lead-authored-by: zanmato <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Rossi(Ruoxi) Sun <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

… length data exceeds offset limit (int32 max) (#39383) ### Rationale for this change When appending var length data in `ExecBatchBuilder`, the offset is potentially to overflow if the batch contains 4GB data or more. This may further result in segmentation fault during the subsequent data content copying. For details, please refer to this comment: #39332 (comment). The solution is let user to use the "large" counterpart data type to avoid the overflow, but we may need explicit error information when such overflow happens. ### What changes are included in this PR? 1. Detect the offset overflow in appending data in `ExecBatchBuilder` and explicitly throw. 2. Change the offset type from `uint32_t` to `int32_t` in `ExecBatchBuilder` and respects the `BinaryBuilder::memory_limit()` which is `2GB - 2B` as the rest part of the codebase. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: #39332 Lead-authored-by: zanmato <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Rossi(Ruoxi) Sun <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

…ng var length data exceeds offset limit (int32 max) (apache#39383) ### Rationale for this change When appending var length data in `ExecBatchBuilder`, the offset is potentially to overflow if the batch contains 4GB data or more. This may further result in segmentation fault during the subsequent data content copying. For details, please refer to this comment: apache#39332 (comment). The solution is let user to use the "large" counterpart data type to avoid the overflow, but we may need explicit error information when such overflow happens. ### What changes are included in this PR? 1. Detect the offset overflow in appending data in `ExecBatchBuilder` and explicitly throw. 2. Change the offset type from `uint32_t` to `int32_t` in `ExecBatchBuilder` and respects the `BinaryBuilder::memory_limit()` which is `2GB - 2B` as the rest part of the codebase. ### Are these changes tested? UT included. ### Are there any user-facing changes? No. * Closes: apache#39332 Lead-authored-by: zanmato <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Rossi(Ruoxi) Sun <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Dec 28, 2023

zanmato1984 mentioned this pull request Dec 28, 2023

SIGSEGV during join on large table #39332

Closed

zanmato1984 changed the title ~~GH-39332: [C++] Give explicit error in ExecBatchBuilder when appending data exceeds offset limit (int32 max)~~ GH-39332: [C++] Explicit error in ExecBatchBuilder when appending data exceeds offset limit (int32 max) Dec 28, 2023

zanmato1984 changed the title ~~GH-39332: [C++] Explicit error in ExecBatchBuilder when appending data exceeds offset limit (int32 max)~~ GH-39332: [C++] Explicit error in ExecBatchBuilder when appending var length data exceeds offset limit (int32 max) Jan 15, 2024

zanmato1984 marked this pull request as ready for review January 15, 2024 08:50

zanmato1984 force-pushed the fix-39332 branch from 4acb497 to e972f3e Compare January 17, 2024 16:33

pitrou reviewed Jan 17, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 17, 2024

zanmato1984 commented Jan 18, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

zanmato1984 and others added 7 commits January 18, 2024 10:42

Give explicit error in ExecBatchBuilder when appending data exceeds o…

b88558b

…ffset limit (int32 max)

Add tests

9797e24

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

eb91fce

Co-authored-by: Antoine Pitrou <[email protected]>

Address comments

a421d8e

Address comment

f9b7ff8

Fix lint

c0db9bd

Fix compiler error

c1ae591

Skip test on 32-bit systems

34d2d41

pitrou force-pushed the fix-39332 branch from fbbcaa0 to 34d2d41 Compare January 18, 2024 09:54

pitrou merged commit a2aa1c4 into apache:main Jan 18, 2024

pitrou removed the awaiting committer review Awaiting committer review label Jan 18, 2024

		for (int i = 0; i < num_rows_to_append; ++i) {
		int32_t length = offsets[num_rows_before + i];

		offsets[num_rows_before + i] = static_cast<int32_t>(sum);
		if (ARROW_PREDICT_FALSE(sum + length > BinaryBuilder::memory_limit())) {

		@@ -19,7 +19,9 @@

		#include <type_traits>

		#include "arrow/array/builder_binary.h"

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

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

Conversation

zanmato1984 commented Dec 28, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Dec 28, 2023

pitrou commented Jan 17, 2024

zanmato1984 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jan 18, 2024

zanmato1984 commented Jan 18, 2024

pitrou commented Jan 18, 2024

This comment was marked as outdated.

zanmato1984 commented Jan 18, 2024

pitrou commented Jan 18, 2024

pitrou commented Jan 18, 2024

zanmato1984 commented Jan 18, 2024

github-actions bot commented Jan 18, 2024

pitrou commented Jan 18, 2024

pitrou commented Jan 18, 2024

conbench-apache-arrow bot commented Jan 18, 2024

zanmato1984 commented Dec 28, 2023 •

edited

Loading

zanmato1984 Jan 17, 2024 •

edited

Loading