Skip to content

Conversation

@ajita-asthana
Copy link
Contributor

@ajita-asthana ajita-asthana commented May 17, 2025

Which issue does this PR close?

#16009

Rationale for this change

What changes are included in this PR?

Implemented char.rs function to use GenericStringBuilder based on the chr.rs implementation.

Are these changes tested?

In-progress of implementing a benchmark

Are there any user-facing changes?

No

@ajita-asthana
Copy link
Contributor Author

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@logan-keede
Copy link
Contributor

Hi @ajita-asthana, are you still working on this issue?

consider implementing a benchmark like this first(in a separate PR) to measure the performance of current chr function against the one you have implemented.

You should also mark this as a draft PR till then.

@ajita-asthana
Copy link
Contributor Author

Hey @logan-keede, I am. Will do the benchmarking.

@github-actions github-actions bot added the spark label May 24, 2025
@ajita-asthana ajita-asthana marked this pull request as draft May 25, 2025 12:34
@ajita-asthana
Copy link
Contributor Author

Hey @logan-keede I am running into import errors unlinked crate. I tried extern crate module_name and it did not work.
The benchmarks are failing because the rust file is not compiled.
Could you please let me how I can resolve the linking errors.

@logan-keede
Copy link
Contributor

Dependencies in rust needs to be mentioned in Cargo.toml of the crate. Have you done that?

@ajita-asthana ajita-asthana force-pushed the aa/string_builder_char branch from 6f66cf4 to 7a2e084 Compare July 8, 2025 11:32
@ajita-asthana ajita-asthana marked this pull request as ready for review July 11, 2025 20:07
@ajita-asthana
Copy link
Contributor Author

@logan-keede could you please review this when you have time.

@andygrove
Copy link
Member

I do see a small performance improvement with these changes:

char                    time:   [9.9009 µs 9.9636 µs 10.036 µs]
                        change: [-3.3878% -2.8089% -2.2840%] (p = 0.00 < 0.05)
                        Performance has improved.

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ajita-asthana.

@andygrove andygrove merged commit df153c2 into apache:main Aug 7, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants