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

MINOR: [C++] Fix StringFormatter type error in localfs_benchmark #13932

Merged
merged 5 commits into from
Aug 24, 2022

Conversation

drin
Copy link
Contributor

@drin drin commented Aug 19, 2022

since size_t is passed as the first argument to the StringFormatter, it
needs to be templated with Int64Type instead of Int32Type

since size_t is passed as the first argument to the StringFormatter, it
needs to be templated with DoubleType instead of Int32Type
drin added 3 commits August 23, 2022 12:06
had a moment of confusion and used the wrong type
the StringFormatter was used in benchmark setup code and only did a
single append. Since it is not part of the code benchmarked, this commit
simplifies by removing the formatter altogether.
since the StringFormatter was removed, formatting.h does not need to be
included anymore
@drin drin requested a review from pitrou August 23, 2022 19:16
@pitrou
Copy link
Member

pitrou commented Aug 23, 2022

@github-actions autotune

Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM, but needs reformatting

@drin
Copy link
Contributor Author

drin commented Aug 23, 2022

oops! hopefully that fixes it

@pitrou
Copy link
Member

pitrou commented Aug 24, 2022

AppVeyor failure is unrelated.

@pitrou pitrou merged commit f93ba4d into apache:master Aug 24, 2022
@ursabot
Copy link

ursabot commented Aug 24, 2022

Benchmark runs are scheduled for baseline = d53bce2 and contender = f93ba4d. f93ba4d 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
[Failed ⬇️0.0% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] f93ba4da ec2-t3-xlarge-us-east-2
[Failed] f93ba4da test-mac-arm
[Failed] f93ba4da ursa-i9-9960x
[Failed] f93ba4da ursa-thinkcentre-m75q
[Finished] d53bce20 ec2-t3-xlarge-us-east-2
[Failed] d53bce20 test-mac-arm
[Failed] d53bce20 ursa-i9-9960x
[Failed] d53bce20 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

anjakefala pushed a commit to anjakefala/arrow that referenced this pull request Aug 31, 2022
…che#13932)

since size_t is passed as the first argument to the StringFormatter, it
needs to be templated with Int64Type instead of Int32Type

Lead-authored-by: Aldrin M <[email protected]>
Co-authored-by: Aldrin Montana <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
@drin drin deleted the minor-fix-localfs-benchmark branch September 1, 2022 22:16
zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…che#13932)

since size_t is passed as the first argument to the StringFormatter, it
needs to be templated with Int64Type instead of Int32Type

Lead-authored-by: Aldrin M <[email protected]>
Co-authored-by: Aldrin Montana <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
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.

5 participants