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.

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

ARROW-17071: [C++][Compute] Fixing off-by-one error in hash join node #13616

Merged

Conversation

michalursa
Copy link
Contributor

@michalursa michalursa commented Jul 15, 2022

Fixing off-by-one error in hash join node.
Zeroing allocated bit vectors in hash join node to fix another valgrind error.

@michalursa
Copy link
Contributor Author

@github-actions crossbow submit -g cpp

@github-actions
Copy link

@github-actions
Copy link

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@github-actions
Copy link

Revision: 53ad1c2

Submitted crossbow builds: ursacomputing/crossbow @ actions-1d8d74c193

Task Status
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-14 Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions

@michalursa michalursa force-pushed the ARROW-17071-hash-join2-valgrind branch from 53ad1c2 to 147aadb Compare July 15, 2022 02:51
@michalursa
Copy link
Contributor Author

@github-actions crossbow submit -g cpp

@github-actions
Copy link

Revision: 147aadb

Submitted crossbow builds: ursacomputing/crossbow @ actions-206b6227c4

Task Status
test-build-cpp-fuzz Github Actions
test-conda-cpp Github Actions
test-conda-cpp-valgrind Azure
test-debian-10-cpp-amd64 Github Actions
test-debian-10-cpp-i386 Github Actions
test-debian-11-cpp-amd64 Github Actions
test-debian-11-cpp-i386 Github Actions
test-fedora-35-cpp Github Actions
test-ubuntu-18.04-cpp Github Actions
test-ubuntu-18.04-cpp-release Github Actions
test-ubuntu-18.04-cpp-static Github Actions
test-ubuntu-20.04-cpp Github Actions
test-ubuntu-20.04-cpp-14 Github Actions
test-ubuntu-20.04-cpp-17 Github Actions
test-ubuntu-20.04-cpp-bundled Github Actions
test-ubuntu-20.04-cpp-thread-sanitizer Github Actions
test-ubuntu-22.04-cpp Github Actions

@michalursa michalursa marked this pull request as ready for review July 15, 2022 04:40
@raulcd
Copy link
Member

raulcd commented Jul 18, 2022

could the test-ubuntu-20.04-cpp-thread-sanitizer be related to the change? Asking because it is on the asof-join instead of the hash-join : arrow-compute-asof-join-node-test

 SUMMARY: ThreadSanitizer: data race /arrow/cpp/src/arrow/compute/exec/asof_join_node.cc:256:7 in arrow::compute::InputState::Push(std::shared_ptr<arrow::RecordBatch> const&)

@westonpace
Copy link
Member

could the test-ubuntu-20.04-cpp-thread-sanitizer be related to the change?

I don't think so. The asof join node has been failing TSAN since before the hash-join code merged. See https://issues.apache.org/jira/browse/ARROW-16964

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

@westonpace westonpace merged commit abcfbd7 into apache:master Jul 18, 2022
@ursabot
Copy link

ursabot commented Jul 18, 2022

Benchmark runs are scheduled for baseline = ffd31d8 and contender = abcfbd7. abcfbd7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️2.27% ⬆️2.27%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.7% ⬆️0.21%] test-mac-arm
[Failed ⬇️0.33% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.61% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Failed] abcfbd78 ec2-t3-xlarge-us-east-2
[Failed] abcfbd78 test-mac-arm
[Failed] abcfbd78 ursa-i9-9960x
[Finished] abcfbd78 ursa-thinkcentre-m75q
[Failed] ffd31d8c ec2-t3-xlarge-us-east-2
[Failed] ffd31d8c test-mac-arm
[Failed] ffd31d8c ursa-i9-9960x
[Finished] ffd31d8c ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants