GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in `CompareColumnsToRows` #43065

zanmato1984 · 2024-06-26T14:30:10Z

Rationale for this change

See #43046.

What changes are included in this PR?

Use unsigned offset safe gather introduced in #42188 which is to fix similar issues.

Are these changes tested?

Yes.

Are there any user-facing changes?

None.

GitHub Issue: [C++] Potential issue in CompareColumnsToRows on AVX2 platforms when row table contains more than 2^31 rows #43046

zanmato1984 · 2024-06-26T14:34:18Z

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

+
+// Compare columns to rows of row ids larger than 2^31 within a row table.
+// Certain AVX2 instructions may behave unexpectedly causing troubles like GH-43046.
+TEST(KeyCompare, CompareColumnsToRowsMany) {


This case takes about 3min to run - it has to construct a row table with more than 2^31 rows which is necessary to reproduce the issue. Is it proper to add it here? Or somewhere else that is more suitable for long-running tests? @pitrou please help?

Or somewhere else that is more suitable for long-running tests?

For example some compiler flags that will be only turned on for time consuming tests? Like ARROW_BUILD_DETAILED_BENCHMARKS for benchmarks.

We don't have such a flag, no. Hopefully we can shrink the test execution time?

Also, how much RAM does this require?

Also, how much RAM does this require?

It takes 24+GB virtual memory.

We don't have such a flag, no. Hopefully we can shrink the test execution time?

I can try to reduce the execution time by using more memory, not sure how good it can do though. And we don't want to introduce more sophisticated speedup method like parallel, do we?

24+GB memory is unpractical for most CI configurations (which are typically based on small VMs), and also many local dev environments. So, I think we'll unfortunately have to disable this test, perhaps using the LARGE_MEMORY_TEST annotation (you can grep through the sources to look for it).

OK, that's unfortunate but seems to be the only option. Shall I now stop my (hopeless) attempts to speedup the test then?

If speeding up the test requires consuming even more memory, then it doesn't sound like a good idea indeed.

zanmato1984 · 2024-06-27T03:11:49Z

Some CI failure seems related. Looking. And turn to draft before figure that out.

zanmato1984 · 2024-06-27T06:01:34Z

Some CI failure seems related. Looking. And turn to draft before figure that out.

No big deal, changed one more line by mistake:
c1eb593 (#43065)

Fixed. Re-ready for review.

zanmato1984 · 2024-06-27T12:05:49Z

My investigation on how much RAM this test uses disclosed something new. The test was a fake pass. There are offset overflows (of uint32_t) happening - I'll open up an issue for it later. But since the number of rows and the size of the row_table_right is multiple of columns_left, the overflowed offsets slide to the beginning of the row table, making the comparison actually happens between the first num_rows_batch rows in row table and the batch. Simply reducing the number of rows of the batch by 1 will make the offset overflow bad enough to fail the test:

-  constexpr int64_t num_rows_batch = std::numeric_limits<uint16_t>::max() + 1ll;
+  constexpr int64_t num_rows_batch = std::numeric_limits<uint16_t>::max();

The further impact to this PR is that, it is technically impossible to construct a valid test case to reproduce the issue that this PR is aiming to solve. This issue requires the following conditions to reproduce:

A fixed length column exists;
A var length column exists;
The number of rows is greater than 2^31.

No matter how small the fixed length column and var length column's content are, there is an 32b "offset" (of the var length column) within the row. And more than 2^31 rows effectively take 2^31 * 4B > 4GB data, then the offset within the row must overflow. Or in other words, one could not simply construct a row table of more than 2^31 rows WITHOUT overflowing the row offset.

We can say that the issue that this PR is for won't actually happen, because something even worse (the overflow) must have happened way earlier. But I still want to have this fix in (I'll have to remove the test though), because the fix is simple, safe, without overhead, and can prevent bad things from happening in the future (maybe when someday we improve the row table to allow valid bigger-than-2^31 rows?).

What's your opinion? @pitrou Thanks.

pitrou · 2024-06-27T12:08:47Z

Two things:

Yes, I agree we should fix the bug even though we can't write a unit test to avoid regressions
Are you saying the current fix is not enough, or am I misunderstanding you?

zanmato1984 · 2024-06-27T12:20:44Z

Are you saying the current fix is not enough, or am I misunderstanding you?

No, the current fix is perfectly enough to solely solve the "gather data from offsets for rows[>=2^31]" issue as long as it actually happens.

What I was talking about is an "overflow" issue will always happen way before the gather issue could actually happen. And once we fixed the overflow issue (I'm going to introduce explicit error), the gather issue won't happen too because the run will be interrupted by my presumable explicit error. But, quoting myself:

and can prevent bad things from happening in the future (maybe when someday we improve the row table to allow valid bigger-than-2^31 rows?).

zanmato1984 · 2024-06-28T05:03:38Z

The test has been removed from this PR. But it will come back, with minor modification and slightly different coverage intention, when this TODO is being mitigated:
https://github.com/apache/arrow/pull/43065/files#diff-26ce8f4fb451b09705a8cadde87fbcc588d0cfe05a91b5677fbda95af215218cR328

zanmato1984 · 2024-06-28T05:35:44Z

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

@@ -325,6 +325,7 @@ Status RowTableImpl::AppendSelectionFrom(const RowTableImpl& from,
    // Varying-length rows
    auto from_offsets = reinterpret_cast<const uint32_t*>(from.offsets_->data());
    auto to_offsets = reinterpret_cast<uint32_t*>(offsets_->mutable_data());
+    // TODO: The following two variables are possibly overflowing.


This is where the "overflow" mentioned in my other comments happens. I'll file another issue/PR for it.

Did you file the other issue? If so, can you update the TODO here to the proper GH issue number? Thanks!

No. I was planning to file it after and refer to this TODO in it.

zanmato1984 · 2024-07-03T07:57:36Z

@pitrou Shall we move on with this? Thanks.

pitrou

LGTM except one change to make

pitrou · 2024-07-09T15:06:09Z

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

@@ -325,6 +325,7 @@ Status RowTableImpl::AppendSelectionFrom(const RowTableImpl& from,
    // Varying-length rows
    auto from_offsets = reinterpret_cast<const uint32_t*>(from.offsets_->data());
    auto to_offsets = reinterpret_cast<uint32_t*>(offsets_->mutable_data());
+    // TODO: The following two variables are possibly overflowing.


Did you file the other issue? If so, can you update the TODO here to the proper GH issue number? Thanks!

pitrou · 2024-07-09T15:06:34Z

@zanmato1984 Sorry for the delay, I was away sick

pitrou · 2024-07-09T15:07:26Z

@zanmato1984 Also, can you rebase?

zanmato1984 · 2024-07-09T15:32:32Z

@zanmato1984 Sorry for the delay, I was away sick

You have my best wishes for you to take care and get better my friend.

zanmato1984 · 2024-07-09T15:42:34Z

@zanmato1984 Also, can you rebase?

Rebase done.

Also, if you think it's better to file the other issue ahead of this PR and have the TODO pointing to it, I'll do that. Thank you!

pitrou · 2024-07-09T15:58:40Z

Also, if you think it's better to file the other issue ahead of this PR and have the TODO pointing to it, I'll do that. Thank you!

Yes, that would be nice. Thanks!

zanmato1984 · 2024-07-09T17:17:48Z

Also, if you think it's better to file the other issue ahead of this PR and have the TODO pointing to it, I'll do that. Thank you!

Yes, that would be nice. Thanks!

#43202 filed and assigned to the TODO in the PR.

conbench-apache-arrow · 2024-07-10T18:51:00Z

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

There were no benchmark performance regressions. 🎉

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

github-actions bot added Component: C++ awaiting review Awaiting review labels Jun 26, 2024

zanmato1984 commented Jun 26, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 26, 2024

zanmato1984 marked this pull request as draft June 27, 2024 03:12

zanmato1984 marked this pull request as ready for review June 27, 2024 06:01

zanmato1984 commented Jun 28, 2024

View reviewed changes

pitrou requested changes Jul 9, 2024

View reviewed changes

zanmato1984 requested review from assignUser, kou, raulcd, paleolimbot, thisisnic, domoritz, trxcllnt, lidavidm, zeroshade, CurtHagenlocher and wgtmac as code owners July 9, 2024 15:32

github-actions bot removed Component: Python Component: JavaScript Component: C# Component: FlightRPC Component: Documentation Component: Swift labels Jul 9, 2024

zanmato1984 removed request for kou, trxcllnt, lidavidm, zeroshade, domoritz, raulcd, CurtHagenlocher, westonpace, wgtmac, paleolimbot, thisisnic and assignUser July 9, 2024 15:40

Fix build

4c7106c

Assign github issue for TODO

cba9a4c

zanmato1984 mentioned this pull request Jul 9, 2024

[C++][Compute] Possible overflow of row table offsets #43202

Closed

pitrou approved these changes Jul 10, 2024

View reviewed changes

pitrou merged commit 47b2fbd into apache:main Jul 10, 2024
39 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Jul 10, 2024

pitrou mentioned this pull request Jul 10, 2024

[C++] Potential issue in CompareColumnsToRows on AVX2 platforms when row table contains more than 2^31 rows #43046

Closed

github-actions bot added the awaiting committer review Awaiting committer review label Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in `CompareColumnsToRows` #43065

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in `CompareColumnsToRows` #43065

zanmato1984 commented Jun 26, 2024 •

edited by github-actions bot

Loading

zanmato1984 Jun 26, 2024

zanmato1984 Jun 27, 2024

pitrou Jun 27, 2024

pitrou Jun 27, 2024

zanmato1984 Jun 27, 2024

zanmato1984 Jun 27, 2024

pitrou Jun 27, 2024

zanmato1984 Jun 27, 2024

pitrou Jun 27, 2024

zanmato1984 commented Jun 27, 2024

zanmato1984 commented Jun 27, 2024

zanmato1984 commented Jun 27, 2024 •

edited

Loading

pitrou commented Jun 27, 2024

zanmato1984 commented Jun 27, 2024

zanmato1984 commented Jun 28, 2024 •

edited

Loading

zanmato1984 Jun 28, 2024 •

edited

Loading

pitrou Jul 9, 2024

zanmato1984 Jul 9, 2024

zanmato1984 commented Jul 3, 2024

pitrou left a comment

pitrou Jul 9, 2024

pitrou commented Jul 9, 2024

pitrou commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

pitrou commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

conbench-apache-arrow bot commented Jul 10, 2024

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in CompareColumnsToRows #43065

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in CompareColumnsToRows #43065

Conversation

zanmato1984 commented Jun 26, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

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 Jun 27, 2024

zanmato1984 commented Jun 27, 2024

zanmato1984 commented Jun 27, 2024 • edited Loading

pitrou commented Jun 27, 2024

zanmato1984 commented Jun 27, 2024

zanmato1984 commented Jun 28, 2024 • edited Loading

zanmato1984 Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanmato1984 commented Jul 3, 2024

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jul 9, 2024

pitrou commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

pitrou commented Jul 9, 2024

zanmato1984 commented Jul 9, 2024

conbench-apache-arrow bot commented Jul 10, 2024

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in `CompareColumnsToRows` #43065

GH-43046: [C++] Fix avx2 gather rows more than 2^31 issue in `CompareColumnsToRows` #43065

zanmato1984 commented Jun 26, 2024 •

edited by github-actions bot

Loading

zanmato1984 commented Jun 27, 2024 •

edited

Loading

zanmato1984 commented Jun 28, 2024 •

edited

Loading

zanmato1984 Jun 28, 2024 •

edited

Loading