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-10609: [Rust] Optimize min/max of non null strings #8673
Conversation
for i in 1..data.len() { | ||
let item = array.value(i); | ||
if !has_value || cmp(&n, item) { | ||
has_value = true; | ||
if cmp(&n, item) { | ||
n = item; | ||
} | ||
} |
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.
Feels like this code can use fold with your changes now.
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.
I translated the other min_max_helper
into using fold. couldn't figure out to convert min_max_string
to use fold here.
Liked it, can you address the comments, that will make some improvements on the benches indirectly. |
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.
LGTM! Cool improvement.
CI failure seems to be infrastructure related:
https://github.com/apache/arrow/pull/8673/checks?check_run_id=1408484265 I re-triggered the job in the github CI and will hope it will pass this time |
The TravisCI builds are not affected by Rust, so merging this one in |
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.
Looks good to me -- I read this carefully
Applies the same optimization as in ARROW-10595. Difference is smaller, but still there: ``` min string 512 time: [3.4096 us 3.4378 us 3.4683 us] change: [-13.563% -13.111% -12.628%] (p = 0.00 < 0.05) Performance has improved. ``` Closes apache#8673 from Dandandan/min_max_string Authored-by: Heres, Daniel <danielheres@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>
Applies the same optimization as in ARROW-10595. Difference is smaller, but still there: