-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
GH-15176: Fix various issues introduced in the asof-join benchmark by ARROW-17980 and ARROW-15732 #15190
GH-15176: Fix various issues introduced in the asof-join benchmark by ARROW-17980 and ARROW-15732 #15190
Conversation
westonpace
commented
Jan 4, 2023
•
edited by github-actions
bot
Loading
edited by github-actions
bot
- Closes: [C++][Benchmarking] AsOfJoinOverhead benchmarks started failing after https://github.com/apache/arrow/commit/498b645e1d09306bf5399a9a019a5caa99513815 was merged #15176
…ark by ARROW-17980 and ARROW-15732
|
|
AsofJoinNodeOptions options = | ||
GetRepeatedOptions(int(state.range(4)), kTimeCol, {kKeyCol}, tolerance); | ||
auto options = std::make_shared<AsofJoinNodeOptions>( | ||
GetRepeatedOptions(int(state.range(4) + 1), kTimeCol, {kKeyCol}, tolerance)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rtpsw can you verify this change? state.range(4)
is the # of right tables but it seems we need a Keys
element for each of the right tables as well as 1 for the left table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I somehow missed this. The fix seems right to me.
@ursabot please benchmark lang=C++ |
Benchmark runs are scheduled for baseline = ec9a8a3 and contender = 987a9ca. Results will be available as each benchmark for each run completes. |
Odd...ursabot scheduled the runs but gave me links to old buildkite builds. Here are the correct links: test-mac-arm: https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2134 @ElenaHenderson is this a known issue? |
Both benchmark builds passed so I think this addresses the issue. |
@westonpace Great! Thank you! ursabot gives links to baseline runs in addition to the contender runs: These are runs for the PR commit (contender): |
Ah, that makes sense. Just a matter of reading more closely. I'm going to go ahead and merge this so CI can be unblocked. @rtpsw , if you have any concerns let's address in a follow-up. |
…ark by ARROW-17980 and ARROW-15732 (apache#15190) * Closes: apache#15176 Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>
Benchmark runs are scheduled for baseline = 2410d36 and contender = 773b5d8. 773b5d8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
…ark by ARROW-17980 and ARROW-15732 (apache#15190) * Closes: apache#15176 Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>