GH-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks #45336

zanmato1984 · 2025-01-23T03:16:28Z

Rationale for this change

#45334

What changes are included in this PR?

An all-mighty test case that can effectively reveal all the bugs mentioned in the issue;
Other than directly fixing the bugs (actually simply casting to 64-bit somewhere in the multiplication will do), I did some refinement to the buffer accessors of the row table, in order to eliminate more potential similar issues (which I believe do exist):
1. null_masks() -> null_masks(row_id) which does overflow-safe indexing inside;
2. is_null(row_id, col_pos) which does overflow-safe indexing and directly gets the bit of the column;
3. data(1) -> fixed_length_rows(row_id) which first asserts the row table being fixed-length, then does overflow-safe indexing inside;
4. data(2) -> var_length_rows() which only asserts the row table being var-length. It is supposed to be paired by the offsets() (which is already 64-bit by GH-43495: [C++][Compute] Widen the row offset of the row table to 64-bit #43389 );
5. The data(0/1/2) members are made private.
The AVX2 specializations are fixed individually by using 64-bit multiplication and indexing.

Are these changes tested?

Yes.

Are there any user-facing changes?

None.

GitHub Issue: [C++][Acero] Overflow issues in swiss join when the number of build side rows is big enough #45334

…rflow

github-actions · 2025-01-23T03:16:51Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2025-01-23T03:18:37Z

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

zanmato1984 · 2025-01-23T04:05:17Z

Hi @pitrou , mind to take a look? Thanks.

cpp/src/arrow/acero/hash_join_node_test.cc

pitrou

The new helpers increase readability a bit, thank you.

I haven't looked at the test, but otherwise looks good in principle. See comments below.

pitrou · 2025-01-23T09:38:55Z

cpp/src/arrow/compute/row/row_internal.h

-    }
-    return NULLPTR;
+
+  inline const uint8_t* null_masks(uint32_t row_id) const {


Note that inline is implied when the method definition is inside the class declaration, there's no need to put it explicitly.

pitrou · 2025-01-23T09:39:54Z

cpp/src/arrow/compute/row/row_internal.h

-      return buffers_[i]->mutable_data();
-    }
-    return NULLPTR;
+  inline uint8_t* null_masks(uint32_t row_id) {


Should it be called mutable_null_masks, for consistency with other helper methods below?

pitrou · 2025-01-23T09:54:47Z

cpp/src/arrow/compute/row/encode_internal.cc

      case 1:
        for (uint32_t i = 0; i < num_rows; ++i) {
-          col_base[i] = row_base[i * row_size];
+          col_base[i] = *rows.fixed_length_rows(start_row + i);


Hmm... we're not adding offset_within_row anymore? Is it deliberate?

Sorry this was a mistake. Addressed. Thank you for spotting this!

pitrou · 2025-01-23T10:05:33Z

cpp/src/arrow/acero/swiss_join.cc

+    memcpy(target->mutable_fixed_length_rows(static_cast<uint32_t>(first_target_row_id)),
+           source.fixed_length_rows(/*row_id=*/0), fixed_length * num_source_rows);
  } else {
    // Row length must be a multiple of 64-bits due to enforced alignment.


Unrelated, but that's an interesting piece of info. Doesn't it blow up memory use if there is a small number of very small columns?

Yes this is true. But IMHO this is acceptable because we also have other auxiliary data structures to aim the hash join so I wouldn't say this is very bad.

pitrou · 2025-01-23T10:07:40Z

cpp/src/arrow/acero/swiss_join.cc

  if (!source_rows_permutation) {
-    memcpy(target->mutable_data(1) + fixed_length * first_target_row_id, source.data(1),
-           fixed_length * num_source_rows);
+    DCHECK_LE(first_target_row_id, std::numeric_limits<uint32_t>::max());


Were these size constraints already implied but not tested for?

We assume row id to be uint32_t (that means no more than 2^32 rows are allowed) almost everywhere, so this is implied. But there are still some places weirdly and unnecessarily using int64_t as row id, here included.

pitrou · 2025-01-23T10:09:25Z

cpp/src/arrow/acero/swiss_join.cc

    int64_t num_words_per_row = fixed_length / sizeof(uint64_t);
    for (int64_t i = 0; i < num_source_rows; ++i) {
      int64_t source_row_id = source_rows_permutation[i];
+      DCHECK_LE(source_row_id, std::numeric_limits<uint32_t>::max());


If that's always the case then are we wasting memory and CPU cache by having a 64-bit permutation array?

We are. In fact I have another WIP branch trying to clean them up.

pitrou · 2025-01-23T10:27:22Z

cpp/src/arrow/compute/row/compare_internal_avx2.cc

+      __m256i bit_id_hi =
+          _mm256_mul_epi32(irow_right_hi, _mm256_set1_epi64x(null_mask_num_bytes * 8));
+      bit_id_lo = _mm256_add_epi64(bit_id_lo, pos_after_encoding);
+      bit_id_hi = _mm256_add_epi64(bit_id_hi, pos_after_encoding);


I seem to have already seen this kind of code in other PRs, perhaps we can add helpers to make it more readable (perhaps in a avx2_internal.h header?):

inline std::pair<__m256i, __m256i> WidenInt32to64(__m256 x) { __m256i x_lo = _mm256_cvtepi32_epi64(_mm256_castsi256_si128(x)); __m256i x_hi = _mm256_cvtepi32_epi64(_mm256_extracti128_si256(x, 1)); return {x_lo, x_hi}; } // Compute `x * factor + addend` in the 64-bit domain inline __m256i MulAddInt64(__m256 x, __m256 factor, __m256 addend) { return _mm256_add_epi64(addend, _mm256_mul_epi32(x, factor)); } inline __m256i MulAddInt64(__m256 x, int64_t factor, int64_t addend) { return MulAddInt64(x, _mm256_set1_epi64x(factor), _mm256_set1_epi64x(addend)); }

then the code above could look like:

auto {irow_right_lo, irow_right_hi} = WidenInt32to64(irow_right); auto bit_id_lo = MulAddInt64(irow_right_lo, null_mask_num_bytes * 8, pos_after_encoding); auto bit_id_hi = MulAddInt64(irow_right_hi, null_mask_num_bytes * 8, pos_after_encoding);

(and in an ideal future, this would be migrated to xsimd :))

Sure, will do.

I found a lot of legacy code can take advantage of these helpers. But they might be too much to put into this PR. Can I do it in another PR?

Yes, definitely.

I made the helper in the other comment though, which is independent of the ones suggested in this comment. (I think these ones are more "basic" than that one which is more row table specific.)

pitrou · 2025-01-23T10:54:01Z

cpp/src/arrow/compute/row/compare_internal_avx2.cc

+      __m128i right_hi = _mm256_i64gather_epi32(reinterpret_cast<const int*>(null_masks),
+                                                _mm256_srli_epi64(bit_id_hi, 3), 1);
+      __m256i right = _mm256_set_m128i(right_hi, right_lo);
+      right = _mm256_and_si256(right, bit_in_right);


Also perhaps a more general helper:

// Get null bits at `null_bit_id` as a vector of 32-bit ints __m256i GetRowNullBitsInt32(const RowTableImpl& rows, uint32_t null_bit_id, __m256 row_index32);

Sure, will do.

Done. I made the helper you suggested and put it in a common header. Other than this file, there is one more piece of code in swiss_join_avx2.cc can reuse it. Pretty nice.

I also made two helper functions Cmp32/64To8 local in compare_internal_avx2.cc that also save some LOC.

zanmato1984 · 2025-01-23T11:14:41Z

Thank you @pitrou . Please hold a bit. The CI failure seems related, meanwhile I'm still working on enhancing the added test to cover not only payload columns.

Will update and address your comment soon.

This reverts commit c3b0ee7.

zanmato1984 · 2025-01-23T12:34:15Z

Hi @pitrou . Hopefully the CI should be fixed now. And the test has been enhanced to the extent that can also discover the overflow in key columns (the position of the key in row table depends on the hashing algorithm so it took me a while to tune a matching key value that will locate on the higher address in the row table - note the 289339070 in the test). I tested it by reverting several places of the fix in my local and the overflow did happen.

I'm addressing your comments now. Will update soon.

…result to 8b

…rflow

zanmato1984 · 2025-01-25T15:07:08Z

The R failures seem related. Will take a look.

zanmato1984 · 2025-01-27T03:30:27Z

The R failures seem related. Will take a look.

Fixed.

pitrou

LGTM except for one minor suggestion

pitrou · 2025-01-27T14:06:24Z

cpp/src/arrow/acero/swiss_join_avx2.cc

+    uint32_t null32_lo =
+        _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_castsi256_si128(null32)));
+    uint32_t null32_hi =
+        _mm256_movemask_epi8(_mm256_cvtepi32_epi64(_mm256_extracti128_si256(null32, 1)));


Can probably reuse Cmp32to8 here?

Indeed. Moved Cmp32/64To8 to common header and used it here. Thank you.

zanmato1984 · 2025-01-27T15:41:43Z

@github-actions crossbow submit -g cpp

github-actions · 2025-01-27T15:44:22Z

Revision: f7df7a4

Submitted crossbow builds: ursacomputing/crossbow @ actions-6bfabdccd3

Task	Status
example-cpp-minimal-build-static
example-cpp-minimal-build-static-system-dependency
example-cpp-tutorial
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp-ubuntu-20.04-cuda-11.2.2
test-cuda-cpp-ubuntu-22.04-cuda-11.7.1
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-bundled-offline
test-ubuntu-24.04-cpp-gcc-13-bundled
test-ubuntu-24.04-cpp-gcc-14
test-ubuntu-24.04-cpp-minimal-with-formats
test-ubuntu-24.04-cpp-thread-sanitizer

conbench-apache-arrow · 2025-01-27T23:08:29Z

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

There were 8 benchmark results with an error:

Commit Run on test-mac-arm at 2025-01-27 19:20:18Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1
- tpch (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-03, scale_factor=1
and 6 more (see the report linked below)

There were no benchmark performance regressions. 🎉

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

…table (#45473) ### Rationale for this change The failure reported in #45393 seems to be caused by a careless parentheses typo introduced in #45336: https://github.com/apache/arrow/blob/e32f56b478171fc4b53dc2042c4cf5d37c97e351/cpp/src/arrow/compute/row/encode_internal.cc#L281-L282 And unfortunately our `Grouper` UT doesn't have cases covering this particular code path (the questioning code path is only triggered in `Grouper` with very restrictive conditions: the row table is fixed-length, a 32-bit key is encoded after some other keys). ### What changes are included in this PR? An UT to reproduce the issue and the fix. ### Are these changes tested? UT included. ### Are there any user-facing changes? None. * GitHub Issue: #45393 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com>

…w-compute-row-test (#46635) ### Rationale for this change In #45336 we refined the row table buffer accessors and enforced the validation on who can call the `var_length_rows()` method. However a legacy test `CompareColumnsToRowsOver4GBFixedLength` is leveraging this accessor to assert this buffer being null. ### What changes are included in this PR? We can just check if the row table is fixed length. ### Are these changes tested? Yes. ### Are there any user-facing changes? None. * GitHub Issue: #46623 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>

zanmato1984 added 9 commits January 15, 2025 01:05

Reproduce the payload overflow issue

e15d80b

Merge remote-tracking branch 'apache/main' into reproduce-payload-ove…

b752669

…rflow

Refine tests

06bcc5e

Replace overflow-prone null mask access

2cdd4c2

Private buffer accessor and use dedicated interfaces

7f0ea14

Refine and fix

f2f3535

Fix avx2 visit null overflow

9b1e908

Remove useless assertion

c004237

Remove col_pos from null_masks() arguments

18d8188

zanmato1984 requested a review from westonpace as a code owner January 23, 2025 03:16

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 23, 2025

zanmato1984 changed the title ~~GH45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks~~ GH-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks Jan 23, 2025

zanmato1984 marked this pull request as draft January 23, 2025 03:18

Fix compare avx2 using null masks

ba24a03

zanmato1984 marked this pull request as ready for review January 23, 2025 04:04

Refine tests

22d6b1e

zanmato1984 commented Jan 23, 2025

View reviewed changes

cpp/src/arrow/acero/hash_join_node_test.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 23, 2025

Refine test

8897753

pitrou reviewed Jan 23, 2025

View reviewed changes

zanmato1984 added 4 commits January 23, 2025 19:40

Enhance test

ff4202b

Fix

5e7f863

Fix

c3b0ee7

Revert "Fix"

b93af5b

This reverts commit c3b0ee7.

zanmato1984 added 3 commits January 23, 2025 20:17

Fix

5c0c857

Remove already implied inline keywords

efe3c98

null_masks -> mutable_null_masks

53dc951

zanmato1984 added 4 commits January 24, 2025 00:09

Helper functions for get null bits from row table and 32/64b compare …

8e97d6b

…result to 8b

Revert mis-commented gtest skip

c4d3959

Move GetNullBitInt32 to common header and reuse it in swiss join code

d4c2af3

Merge remote-tracking branch 'apache/main' into reproduce-payload-ove…

65c4003

…rflow

Fix CI

8355c68

pitrou approved these changes Jan 27, 2025

View reviewed changes

Move Cmp32/64To8 to common header and reuse it in swiss join avx2

f7df7a4

zanmato1984 merged commit b818560 into apache:main Jan 27, 2025
37 checks passed

zanmato1984 removed the awaiting committer review Awaiting committer review label Jan 27, 2025

zanmato1984 mentioned this pull request Jan 27, 2025

[C++][Acero] Overflow issues in swiss join when the number of build side rows is big enough #45334

Closed

raulcd mentioned this pull request Jan 30, 2025

[Benchmarking] Conbench reports regressions and errors on latest PRs #45393

Closed

zanmato1984 mentioned this pull request Feb 10, 2025

GH-45393: [C++][Compute] Fix wrong decoding for 32-bit column in row table #45473

Merged

zanmato1984 mentioned this pull request Feb 12, 2025

GH-45506: [C++][Acero] More overflow-safe Swiss table #45515

Merged

zanmato1984 mentioned this pull request Mar 21, 2025

[C++][Acero] Cleanup 64-bit temp states of Swiss join by using 32-bit #45877

Closed

zanmato1984 mentioned this pull request May 29, 2025

GH-46623: [C++][Compute] Fix the failure of large memory test in arrow-compute-row-test #46635

Merged

GH-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks #45336

GH-45334: [C++][Acero] Fix swiss join overflow issues in row offset calculation for fixed length and null masks #45336

Uh oh!

Conversation

zanmato1984 commented Jan 23, 2025 • 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?

Uh oh!

github-actions bot commented Jan 23, 2025

Uh oh!

github-actions bot commented Jan 23, 2025

Uh oh!

zanmato1984 commented Jan 23, 2025

Uh oh!

Uh oh!

pitrou 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

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!

zanmato1984 commented Jan 23, 2025

Uh oh!

zanmato1984 commented Jan 23, 2025

Uh oh!

zanmato1984 commented Jan 25, 2025

Uh oh!

zanmato1984 commented Jan 27, 2025

Uh oh!

pitrou 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!

zanmato1984 commented Jan 27, 2025

Uh oh!

github-actions bot commented Jan 27, 2025

Uh oh!

zanmato1984 commented Jan 23, 2025 •

edited

Loading