GH-14937: [C++] String Sort / Rank Benchmarks #34811
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felipecrv
force-pushed
the
string-sort-benchmark
branch
from
4cf771b to
6d85624
Compare
Contributor
Author
felipecrv
force-pushed
the
string-sort-benchmark
branch
from
7722db1 to
2caf530
Compare
Contributor
Author
|
Python issue in CI is unrelated. |
wgtmac
reviewed
Mar 31, 2023
Member
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Could you please provide some results? Just curious about the
Member
There was a problem hiding this comment.
Is n_chunks worth being a parameter instead of a constant value?
Contributor
Author
There was a problem hiding this comment.
The other benchmarks also chunk the arrays by 10, so to keeps things comparable, we should use 10 everywhere.
Member
There was a problem hiding this comment.
I am not familiar with the sort function here. Does it sort null values to the first or to the end? Does the null distribution affect the benchmark result? It would be good to provide some benchmark result as well.
Contributor
Author
There was a problem hiding this comment.
I pasted below. Null in variable-size binary arrays will reduce the amout of data in the data buffer, making CPU caching more effective.
github-actions
bot
added
awaiting committer review
westonpace
reviewed
Mar 31, 2023
Comment on lines
+226
to
+219
Member
There was a problem hiding this comment.
Given that you went to the trouble of making this configurable, should we have a ArraySortIndicesStringShort and ArraySortIndicesStringLong similar to how we have Narrow and Wide versions of the integer benchmarks?
Contributor
Author
github-actions
bot
added
awaiting changes
Contributor
Author
|
@wgtmac here it is. Number might contain noise as I run a lot of stuff on my dev machine: |
felipecrv
force-pushed
the
string-sort-benchmark
branch
from
2caf530 to
f02c06d
Compare
github-actions
bot
added
awaiting change review
zeroshade
approved these changes
Apr 4, 2023
Member
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
this all LGTM, i think the other comments already addressed question i would have asked.
github-actions
bot
added
awaiting merge
Contributor
Author
|
@zeroshade I added the Narrow and Wide versions based on @westonpace's request above. I picked 16 chars for narrow and 64 chars for the wide. The CI failure is unrelated, so feel free to merge if the last commit is OK. |
Contributor
|
Thanks @felipecrv for seeing this through |
|
Benchmark runs are scheduled for baseline = 8c91434 and contender = a09201b. a09201b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
liujiacheng777
pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
This is apache#15041 with the last review comments addressed. * Closes: apache#14937 Lead-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Co-authored-by: Will Ayd <william.ayd@icloud.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
ArgusLi
pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
This is apache#15041 with the last review comments addressed. * Closes: apache#14937 Lead-authored-by: Felipe Oliveira Carvalho <felipekde@gmail.com> Co-authored-by: Will Ayd <william.ayd@icloud.com> Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is #15041 with the last review comments addressed.