Skip to content

Minor: avoid a few clones in sum accumulator setup - #24309

Merged
jayzhan211 merged 1 commit into
apache:mainfrom
alamb:alamb/avoid_some_clones
Aug 13, 2026
Merged

Minor: avoid a few clones in sum accumulator setup#24309
jayzhan211 merged 1 commit into
apache:mainfrom
alamb:alamb/avoid_some_clones

Conversation

@alamb

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

This is a minor thing I found while doing some performance profiling for #24035

What changes are included in this PR?

Avoid a few clone calls

Are these changes tested?

By CI

Are there any user-facing changes?

realistically nothing that someone will measure

@github-actionsgithub-actionsBot added the functions Changes to functions implementation label Aug 12, 2026
Self {
values: GenericDistinctBuffer::new(data_type.clone()),
data_type: data_type.clone(),
data_type,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

this is one clone removed

} else {
macro_rules! helper {
($t:ty, $dt:expr) => {
Ok(Box::new(SumAccumulator::<$t>::new($dt.clone())))

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

this is the second clone removed

@alamb
alamb marked this pull request as ready for review August 12, 2026 21:25
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.14%. Comparing base (bbb552f) to head (c2d13e7).

Additional details and impacted files
@@ Coverage Diff @@## main #24309 +/- ##
==========================================
- Coverage 81.14% 81.14% -0.01% 
==========================================
Files 1110 1110 Lines 386137 386137 Branches 386137 386137 ==========================================
- Hits 313347 313339 -8 - Misses 54324 54330 +6 - Partials 18466 18468 +2 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Rich-T-kidRich-T-kid 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.

make sense to me

@gabotechsgabotechs 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.

👍 pretty simple, looks good

@jayzhan211
jayzhan211 added this pull request to the merge queueAug 13, 2026
@jayzhan211

Copy link
Copy Markdown
Contributor

LGTM 👍🏻

Merged via the queue into apache:main with commit 1164d60Aug 13, 2026
37 checks passed
@alamb

Copy link
Copy Markdown
ContributorAuthor

Thanks @gabotechs and @jayzhan211

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functionsChanges to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@alamb@codecov-commenter@jayzhan211@gabotechs@Rich-T-kid