GH-54885: [C++][Compute] Use 64-bit offsets for basic row encoding by zanmato1984 · Pull Request #55170 · apache/arrow

zanmato1984 · 2026-10-24T06:29:40Z

Rationale for this change

Basic hash joins can crash when encoded build-side keys or payloads exceed 2 GiB because their cumulative byte offsets overflow int32_t.

What changes are included in this PR?

Use 64-bit byte offsets and length calculations in RowEncoder, shared KeyEncoder counters, and basic Grouper. Row/group IDs and per-column encoded length formats are unchanged.

Are these changes tested?

Added large-memory regressions for length calculation, appends, fixed-width decoding, grouping, and hash-join keys/payloads, plus tests for sliced inputs and encoder reuse.

Are there any user-facing changes?

Basic hash joins can handle more than 2 GiB of encoded large_string/large_binary build-side keys or payloads.

Offsets grow from 4 to 8 bytes. Extra element storage is 4 × (N + 1) bytes per RowEncoder storing N rows: approximately 3.8 MiB per million rows, separately for key and payload encoders. Basic Grouper also uses four additional bytes per unique-key offset and temporary batch offset. These figures exclude spare capacity and allocator overhead; total join memory does not double.

No public API or row/group-count limit changes.

This PR contains a "Critical Fix". It prevents a crash on valid input.

Was AI used for this PR?

The author conducted a thorough review with AI assistance.

PR code and description written by:

Human
AI

Reviewed before submission by:

Human
AI
Not reviewed

GitHub Issue: [C++][Python] pyarrow.Table.join() fatal crash for large_string keys near 2gb #54885

Signed-off-by: Rossi Sun <[email protected]>

github-actions · 2026-10-24T06:30:04Z

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

Copilot

🟢 Approval recommended

Static review found no blocking issues in the consistent widening or regression coverage; the opt-in large-memory tests were not executed.

No open issues 🎉

What changed in this PR

Fixes the 2 GiB encoded-buffer overflow behind GH-54885 by widening internal byte offsets and lengths to 64 bits in basic row encoding and grouping. Row IDs, group IDs, and per-column encoding formats remain unchanged.

Changes:

Widens RowEncoder offsets and shared KeyEncoder length counters.
Updates basic Grouper temporary and persistent buffer offsets consistently.
Adds six opt-in large-memory regressions covering encoders, grouping, and join keys and payloads.

File	Description
cpp/src/arrow/compute/row/row_encoder_internal.h	Widens encoder interfaces, row lengths, and stored offsets.
cpp/src/arrow/compute/row/row_encoder_internal.cc	Uses 64-bit length calculations and cumulative append sizes.
cpp/src/arrow/compute/row/row_encoder_internal_test.cc	Tests large value lengths and cumulative appends beyond 2 GiB.
cpp/src/arrow/compute/row/grouper.cc	Widens batch and persistent unique-key offsets.
cpp/src/arrow/compute/row/grouper_test.cc	Tests batch and unique-key buffers beyond 2 GiB.
cpp/src/arrow/acero/hash_join_node_test.cc	Tests oversized build-side key and payload buffers.

_{Review effort: Balanced}

Signed-off-by: Rossi Sun <[email protected]>

Copilot

🔵 Needs a closer look

The multi-gigabyte native buffer changes need opt-in regression execution and final human validation before approval.

No open issues 🎉

_{Review effort: Balanced}

dmitry-chirkov-dremio

LGTM but I'd love to see benchmarks for short-row workloads

Avoid redundant offset initialization and read variable payload lengths directly from array metadata while retaining 64-bit counters. Signed-off-by: Rossi Sun <[email protected]>

Copilot

🔵 Needs a closer look

The crash-critical buffer changes need human confirmation with the opt-in large-memory regressions before approval.

No open issues 🎉

_{Review effort: Balanced}

pitrou · 2026-10-26T10:15:49Z

+  ASSERT_OK_AND_ASSIGN(auto expected, MakeArrayFromScalar(UInt32Scalar{0}, kBatchLength));
+
+  // All rows must receive group 0, despite the large temporary batch encoding.
+  ASSERT_OK_AND_ASSIGN(auto groups, grouper->Consume(ExecSpan(batch)));


Also validate groups?

Added ValidateFull() before checking the group IDs.

pitrou · 2026-10-26T10:16:12Z

+  // Re-inserting the same key must not create more groups; Lookup must find it
+  // for every row, and GetUniques must decode just the one original value.
+  ASSERT_OK(grouper->Populate(ExecSpan(batch)));
+  ASSERT_OK_AND_ASSIGN(auto found, grouper->Lookup(ExecSpan(batch)));


Same here: validate found

Added ValidateFull() before comparing the lookup result with the expected IDs.

pitrou · 2026-10-26T10:17:47Z

+      ASSERT_OK(grouper->Populate(ExecSpan(batch)));
+    } else {
+      ASSERT_OK_AND_ASSIGN(auto groups, grouper->Consume(ExecSpan(batch)));
+      auto group_array = groups.make_array();


Same: validate groups

Added ValidateFull(), length and null-count checks before accessing the individual group IDs. The final lookup result is now validated as well.

pitrou · 2026-10-26T10:27:27Z

 }

 Status RowEncoder::EncodeAndAppend(const ExecSpan& batch) {
+  const int64_t batch_length = batch.length;


Is there a point in this? Casting to int64_t perhaps?

batch.length is already int64_t; this caches the loop bound rather than casting it. With Apple Clang on arm64, using the member directly caused the prefix-sum loop to reload it on each iteration. A local A/B benchmark showed about a 1% benefit from caching it for million-row fixed-width encoding batches. I kept the local variable for that small benefit; it is not required for correctness.

pitrou · 2026-10-26T10:33:27Z

+  LargeBinaryScalar scalar(bytes);
+  VarLengthKeyEncoder<LargeBinaryType> encoder(large_binary());
+  int64_t length = 0;
+  encoder.AddLength(ExecValue(&scalar), /*batch_length=*/1, &length);


Why not use a non-trivial batch length?

Changed the scalar case to broadcast over three rows and check every length counter, rather than only exercising a single row.

pitrou · 2026-10-26T10:33:39Z

+  auto offsets = Buffer::FromVector<int64_t>({0, kValueLength});
+  auto array = ArrayData::Make(large_binary(), /*length=*/1, {nullptr, offsets, bytes},
+                               /*null_count=*/0);
+  encoder.AddLength(ExecValue(*array), /*batch_length=*/1, &length);


Same question here.

Changed the array case to three rows: a value larger than 2 GiB, an empty value, and a null. It reuses the same value buffer and checks all three counters, including accumulation on top of the scalar lengths.

pitrou · 2026-10-26T10:35:54Z

+  // the special all-null row, without decoding the entire large buffer.
+  const int32_t row_ids[] = {0, kBatchLength - 1, kNumRows - 2, kNumRows - 1,
+                             RowEncoder::kRowIdForNulls()};
+  ASSERT_OK_AND_ASSIGN(auto decoded, encoder.Decode(kNumDecodedValues + 1, row_ids));


Validate decoded?

Added ValidateFull() on the complete decoded array before slicing it and comparing the values.

pitrou · 2026-10-26T10:42:35Z

+  constexpr int64_t kValueLength = 64 * 1024;
+  constexpr int32_t kBatchLength = 1024;
+  constexpr int32_t kNumBatches = 17;
+  // Keep join keys small so that it is the build-side payload encoder, not the


"Payload encoder" means the join result, or something else?

It is the build-side encoder storing r_payload1 and r_payload2, not the join result. I clarified the comment to distinguish that buffer from the key buffer.

pitrou · 2026-10-26T10:44:02Z

+  const auto zero_id = ArrayFromJSON(int32(), "[0]");
+  const auto zero_key = ArrayFromJSON(binary(), R"(["0"])");
+
+  for (const auto& type : {large_utf8(), large_binary()}) {


Do we actually need to test both types?

Agreed, both types use VarLengthKeyEncoder<LargeBinaryType> here. I kept only large_binary() for the payload regression, while retaining both small-key encodings and serial/parallel execution.

pitrou · 2026-10-26T10:46:19Z

+    // cover eight INNER join combinations without growing the expected output.
+    for (bool binary_keys : {false, true}) {
+      for (bool parallel : {false, true}) {
+        CheckRunOutput(JoinType::INNER, left, right, {binary_keys ? "l_key" : "l_id"},


Not testing LEFT_ANTI as well?

With the default true filter, LEFT_ANTI has no right-side output, so the large payload columns are projected away. Adding it to this test would therefore not exercise the payload buffer exceeding 2 GiB. A meaningful anti-join payload case would need a residual filter using those payload columns. I kept this test focused on INNER; the separate large-key regression still covers both INNER and LEFT_ANTI.

pitrou · 2026-10-26T10:46:58Z

@zanmato1984 AI-generated PR description is very verbose, can you trim it for human consumption?

Signed-off-by: Rossi Sun <[email protected]>

zanmato1984

Shortened the description to the problem, main changes, test coverage and memory overhead, while retaining the required disclosures.

zanmato1984 · 2026-10-27T20:55:04Z

+  ASSERT_OK_AND_ASSIGN(auto expected, MakeArrayFromScalar(UInt32Scalar{0}, kBatchLength));
+
+  // All rows must receive group 0, despite the large temporary batch encoding.
+  ASSERT_OK_AND_ASSIGN(auto groups, grouper->Consume(ExecSpan(batch)));


Added ValidateFull() before checking the group IDs.

zanmato1984 · 2026-10-27T20:55:05Z

+  // Re-inserting the same key must not create more groups; Lookup must find it
+  // for every row, and GetUniques must decode just the one original value.
+  ASSERT_OK(grouper->Populate(ExecSpan(batch)));
+  ASSERT_OK_AND_ASSIGN(auto found, grouper->Lookup(ExecSpan(batch)));


Added ValidateFull() before comparing the lookup result with the expected IDs.

zanmato1984 · 2026-10-27T20:55:07Z

+      ASSERT_OK(grouper->Populate(ExecSpan(batch)));
+    } else {
+      ASSERT_OK_AND_ASSIGN(auto groups, grouper->Consume(ExecSpan(batch)));
+      auto group_array = groups.make_array();


Added ValidateFull(), length and null-count checks before accessing the individual group IDs. The final lookup result is now validated as well.

zanmato1984 · 2026-10-27T20:55:08Z

 }

 Status RowEncoder::EncodeAndAppend(const ExecSpan& batch) {
+  const int64_t batch_length = batch.length;


batch.length is already int64_t; this caches the loop bound rather than casting it. With Apple Clang on arm64, using the member directly caused the prefix-sum loop to reload it on each iteration. A local A/B benchmark showed about a 1% benefit from caching it for million-row fixed-width encoding batches. I kept the local variable for that small benefit; it is not required for correctness.

zanmato1984 · 2026-10-27T20:55:10Z

+  LargeBinaryScalar scalar(bytes);
+  VarLengthKeyEncoder<LargeBinaryType> encoder(large_binary());
+  int64_t length = 0;
+  encoder.AddLength(ExecValue(&scalar), /*batch_length=*/1, &length);


Changed the scalar case to broadcast over three rows and check every length counter, rather than only exercising a single row.

zanmato1984 · 2026-10-27T20:55:11Z

+  auto offsets = Buffer::FromVector<int64_t>({0, kValueLength});
+  auto array = ArrayData::Make(large_binary(), /*length=*/1, {nullptr, offsets, bytes},
+                               /*null_count=*/0);
+  encoder.AddLength(ExecValue(*array), /*batch_length=*/1, &length);


Changed the array case to three rows: a value larger than 2 GiB, an empty value, and a null. It reuses the same value buffer and checks all three counters, including accumulation on top of the scalar lengths.

zanmato1984 · 2026-10-27T20:55:13Z

+  // the special all-null row, without decoding the entire large buffer.
+  const int32_t row_ids[] = {0, kBatchLength - 1, kNumRows - 2, kNumRows - 1,
+                             RowEncoder::kRowIdForNulls()};
+  ASSERT_OK_AND_ASSIGN(auto decoded, encoder.Decode(kNumDecodedValues + 1, row_ids));


Added ValidateFull() on the complete decoded array before slicing it and comparing the values.

zanmato1984 · 2026-10-27T20:55:14Z

+  constexpr int64_t kValueLength = 64 * 1024;
+  constexpr int32_t kBatchLength = 1024;
+  constexpr int32_t kNumBatches = 17;
+  // Keep join keys small so that it is the build-side payload encoder, not the


It is the build-side encoder storing r_payload1 and r_payload2, not the join result. I clarified the comment to distinguish that buffer from the key buffer.

zanmato1984 · 2026-10-27T20:55:16Z

+  const auto zero_id = ArrayFromJSON(int32(), "[0]");
+  const auto zero_key = ArrayFromJSON(binary(), R"(["0"])");
+
+  for (const auto& type : {large_utf8(), large_binary()}) {


Agreed, both types use VarLengthKeyEncoder<LargeBinaryType> here. I kept only large_binary() for the payload regression, while retaining both small-key encodings and serial/parallel execution.

zanmato1984 · 2026-10-27T20:55:17Z

+    // cover eight INNER join combinations without growing the expected output.
+    for (bool binary_keys : {false, true}) {
+      for (bool parallel : {false, true}) {
+        CheckRunOutput(JoinType::INNER, left, right, {binary_keys ? "l_key" : "l_id"},


With the default true filter, LEFT_ANTI has no right-side output, so the large payload columns are projected away. Adding it to this test would therefore not exercise the payload buffer exceeding 2 GiB. A meaningful anti-join payload case would need a residual filter using those payload columns. I kept this test focused on INNER; the separate large-key regression still covers both INNER and LEFT_ANTI.

Copilot

🟢 Approval recommended

No active review issues are reported for the changes to 64-bit row-encoding and grouping offsets.

No open issues 🎉

_{Review effort: Balanced}

[C++][Compute] Use 64-bit offsets for basic row encoding

11f0000

Signed-off-by: Rossi Sun <[email protected]>

Copilot AI balanced review requested due to automatic review settings October 24, 2026 06:29

github-actions Bot added awaiting review Awaiting review Component: C++ labels Oct 24, 2026

Copilot started reviewing on behalf of zanmato1984 October 24, 2026 06:30

View session

Copilot AI reviewed Oct 24, 2026

View reviewed changes

zanmato1984 added the Critical Fix Bugfixes for security vulnerabilities, crashes, or invalid data. label Oct 24, 2026

zanmato1984 requested a review from pitrou October 24, 2026 06:36

[C++][Compute] Widen fixed-width decoded buffer size

bd679a0

Signed-off-by: Rossi Sun <[email protected]>

Copilot AI balanced review requested due to automatic review settings October 24, 2026 20:04

Copilot started reviewing on behalf of zanmato1984 October 24, 2026 20:04

View session

Copilot AI reviewed Oct 24, 2026

View reviewed changes

zanmato1984 added the CI: Extra: C++ Run extra C++ CI label Oct 24, 2026

dmitry-chirkov-dremio approved these changes Oct 25, 2026

View reviewed changes

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 25, 2026

[C++][Compute] Reduce basic row encoding overhead

1bc6ba1

Avoid redundant offset initialization and read variable payload lengths directly from array metadata while retaining 64-bit counters. Signed-off-by: Rossi Sun <[email protected]>

Copilot AI balanced review requested due to automatic review settings October 25, 2026 08:40

Copilot started reviewing on behalf of zanmato1984 October 25, 2026 08:40

View session

Copilot AI reviewed Oct 25, 2026

View reviewed changes

pitrou reviewed Oct 26, 2026

View reviewed changes

[C++][Compute] Strengthen large-memory encoding tests

d8b2bb1

Signed-off-by: Rossi Sun <[email protected]>

Copilot AI balanced review requested due to automatic review settings October 27, 2026 20:54

zanmato1984 commented Oct 27, 2026

View reviewed changes

Copilot started reviewing on behalf of zanmato1984 October 27, 2026 20:56

View session

Copilot AI reviewed Oct 27, 2026

View reviewed changes

Conversation

zanmato1984 commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Was AI used for this PR?

Uh oh!

github-actions Bot commented Oct 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

dmitry-chirkov-dremio left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 26, 2026

Uh oh!

zanmato1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

zanmato1984 commented Oct 24, 2026 •

edited

Loading