Skip to content
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

Jump to bottom

GH-43040: [C++] Reduce the recursion of many-join test #43042

Merged
merged 3 commits into from
Jun 26, 2024
Merged

GH-43040: [C++] Reduce the recursion of many-join test #43042

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7251176
Reduce the recursion of many join test
zanmato1984 Jun 25, 2024
3e6acd8
Double check by increasing again and see if CI fails
zanmato1984 Jun 25, 2024
6ccaa6c
Reduce the recursion of many join test again
zanmato1984 Jun 25, 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
2 changes: 1 addition & 1 deletion cpp/src/arrow/acero/hash_join_node_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3220,7 +3220,7 @@ TEST(HashJoin, ManyJoins) {
// stack), which is essentially the recursive usage of the temp vector stack.

// A fair number of joins to guarantee temp vector stack overflow before GH-41335.
const int num_joins = 64;
const int num_joins = 16;
Copy link
Collaborator Author

@zanmato1984 zanmato1984 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To make sure this conservative value serves the same protection purpose, I've verified in my local that, by reverting commit 6c386da, the test failed (with "temp stack overflow") with 16 joins (actually the minimal number for joins to fail is 14).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you condition the reduction on the specific platforms that can't handle num_joins=64? To ensure possible bugs on a high number of joins are caught in regression tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah that's a nice idea. It's just that the condition could be very tricky to identify. So far I've experienced the following combinations on number of joins being 64:

  1. Ubuntu w/ or w/o ASAN (the CI jobs), all good.
  2. MacOS w/ ASAN, stack overflow; MacOS w/o ASAN, good.
  3. Alpine and Emscripten w/o ASAN (the CI jobs), segfault or memory out-of-bound (presumably to be caused by stack overflow as well).

And I don't find macros to differentiate Linux distributions such as Alpine and Ubuntu. To enable at least one build to run 64-join, it seems the only safe condition is to enable 64 joins on Linux w/ ASAN - but that's just because we have only sanitizer build on Ubuntu.

Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can stick with 16 if it's enough to reproduce the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can stick with 16 if it's enough to reproduce the issue.

What do you mean by "enough to repro the issue"? Reducing to 16 is making the issue "go away".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the "issue" here means this:

by reverting commit 6c386da, the test failed (with "temp stack overflow") with 16 joins

In other words, 16 joins serves the purpose that this test is originally designed to cover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh. Now I see it.


// `ExecBatchBuilder::num_rows_max()` is the number of rows for swiss join to accumulate
// before outputting.
Expand Down
Loading