Skip to content

Optimize char expression#16076

Merged
andygrove merged 16 commits into
apache:mainfrom
ajita-asthana:aa/string_builder_char
Aug 7, 2025
Merged

Optimize char expression#16076
andygrove merged 16 commits into
apache:mainfrom
ajita-asthana:aa/string_builder_char

Conversation

@ajita-asthana

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

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@andygrove

Copy link
Copy Markdown
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.

@andygrove andygrove left a comment

Copy link
Copy Markdown
Member

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

3 participants