Skip to content

Improve performance of REPEAT functions - #12015

Merged
alamb merged 4 commits into
apache:mainfrom
tlm365:repeat-optimize
Aug 16, 2024
Merged

Improve performance of REPEAT functions#12015
alamb merged 4 commits into
apache:mainfrom
tlm365:repeat-optimize

Conversation

@tlm365

@tlm365tlm365 commented Aug 15, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes#11990.

Rationale for this change

Refactor code and improve performance.

What changes are included in this PR?

repeat function and benchmark.

Are these changes tested?

Yes

Are there any user-facing changes?

No

Benchmark stats on my local machine:

  • AMD Ryzen 5 PRO 5650U (6 cores 12 threads) - 24Gb RAM

2024-08-16_11-31-57

@tlm365
Improve performance of REPEAT functions
21278a8
Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
@github-actionsgithub-actionsBot added the functions Changes to functions implementation label Aug 15, 2024
comphead
comphead reviewed Aug 15, 2024
View reviewed changes

@compheadcomphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @tlm365 this PR looks promising. Would you mind adding bench results to the PR description?

@tlm365

tlm365 commented Aug 15, 2024

Copy link
Copy Markdown
ContributorAuthor

Thanks @tlm365 this PR looks promising. Would you mind adding bench results to the PR description?

@comphead Thanks for reviewing, I have updated it.

Omega359
Omega359 reviewed Aug 16, 2024
View reviewed changes
Comment threaddatafusion/functions/benches/repeat.rs Outdated
Comment threaddatafusion/functions/src/string/repeat.rs Outdated
@tlm365
Improve performance of REPEAT functions
1ea189e
Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
@tlm365
Fix cargo fmt
230281c
Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
alamb
alamb reviewed Aug 16, 2024
View reviewed changes
Comment threaddatafusion/functions/src/string/repeat.rs
(Some(_), Some(_)) => Some("".to_string()),
_ => None,
}
.for_each(|(string, number)| match (string, number) {

@alambalambAug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👏 -- very nice

alamb
alamb reviewed Aug 16, 2024
View reviewed changes
Comment threaddatafusion/functions/src/string/common.rs
}
}

pub trait StringArrayType<'a>: ArrayAccessor<Item = &'a str> + Sized {

@alambalambAug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This makes sense -- thank you @tlm365 and @Omega359 -- I'll make a PR to add some additional comments to this trait as well

@alambalambAug 16, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#12027 is a PR to document this trait and how to use it

@alamb
alamb merged commit bb92181 into apache:mainAug 16, 2024
@alamb

alamb commented Aug 16, 2024

Copy link
Copy Markdown
Contributor

Thank you for the reviews @tlm365@Omega359 and @comphead

This was referenced Aug 16, 2024
Closed
Closed
@tlm365
tlm365 deleted the repeat-optimize branch August 21, 2024 07:28
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb approved these changes
@compheadcompheadcomphead left review comments
+1 more reviewer
@Omega359Omega359Omega359 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

functionsChanges to functions implementation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve performance of REPEAT functions

4 participants

@tlm365@alamb@Omega359@comphead