Skip to content

Conversation

@js8544
Copy link
Contributor

@js8544 js8544 commented Feb 2, 2023

Rationale for this change

HashJoinImpl::Init expects output_batch_callback and finished_callback to return Status, while the currently passed arguments return void. It caused compilation errors on my machine.

error: cannot convert ‘arrow::compute::JoinBenchmark::JoinBenchmark(arrow::compute::BenchmarkSettings&)::<lambda(int64_t, arrow::compute::ExecBatch)>’ to ‘arrow::compute::HashJoinImpl::OutputBatchCallback’ {aka ‘std::function<arrow::Status(long int, arrow::compute::ExecBatch)>’}

What changes are included in this PR?

Fix the return types.

Are these changes tested?

Yes. It fixes the compilation error on my machine.

Are there any user-facing changes?

No.

@js8544
Copy link
Contributor Author

js8544 commented Feb 2, 2023

The expected return type was changed from void to Status in #15253 last week. Not sure why our CI process didn't catch this compile error.

@kou kou changed the title MINOR: Fix compilation error in hash_join_benchmark MINOR: [C++] Fix compilation error in hash_join_benchmark Feb 2, 2023
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

We need additional -DARROW_BUILD_OPENMP_BENCHMARKS=ON option to build this benchmark. Our CI jobs don't cover it.

@kou kou merged commit 899146b into apache:master Feb 2, 2023
@ursabot
Copy link

ursabot commented Feb 3, 2023

Benchmark runs are scheduled for baseline = b0e1037 and contender = 899146b. 899146b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.15% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.63% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 899146bb ec2-t3-xlarge-us-east-2
[Finished] 899146bb test-mac-arm
[Finished] 899146bb ursa-i9-9960x
[Finished] 899146bb ursa-thinkcentre-m75q
[Finished] b0e10379 ec2-t3-xlarge-us-east-2
[Finished] b0e10379 test-mac-arm
[Finished] b0e10379 ursa-i9-9960x
[Finished] b0e10379 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.

3 participants