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

Improve benchmark for ltrim #12513

Merged
merged 9 commits into from
Sep 18, 2024
Merged

Improve benchmark for ltrim #12513

merged 9 commits into from
Sep 18, 2024

Conversation

Rachelint
Copy link
Contributor

@Rachelint Rachelint commented Sep 17, 2024

Which issue does this PR close?

Part of #12387

Rationale for this change

We need a benchmark to see the improvements of the new string view trim introduced in #12395 .

What changes are included in this PR?

Improve the ltrim benchmark, so that we can use it to see the improvements of the new string view trim.

Are these changes tested?

Test manually.

Are there any user-facing changes?

No.

@Rachelint Rachelint marked this pull request as ready for review September 18, 2024 04:57
@Rachelint
Copy link
Contributor Author

@alamb @Kev1n8 The benchmark is ready, and the result in my local can see in #12395 (comment)

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @Rachelint and @Kev1n8 -- looks good to me; I had one question, but I don't think there is anything that would prevent this PR from merging.

Comment on lines 61 to 62
let lens = vec![remaining_len; size];
let string_iter = lens.into_iter().map(|len| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the use of vec! here - isn't this the same as creating size strings of lenght up to remaining_len?

Maybe this could be somehthing like

let string_iter = (0..size).map(|len|{ 

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I want to create size strings, and each one has remaining_len lenght.
🤔 Maybe it is a bit confused, I have improved it and added some comments.

}
}

/// Create args for the ltrim benchmark
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

datafusion/functions/benches/ltrim.rs Show resolved Hide resolved
@alamb
Copy link
Contributor

alamb commented Sep 18, 2024

Thank you for the comments and benchmarks @Rachelint -- this PR now looks even better 🚀

@alamb alamb merged commit 5abef41 into apache:main Sep 18, 2024
24 checks passed
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.

2 participants