Skip to content

Comments

Remove unnecessary Arc<ArrayRef>#9316

Merged
scovich merged 5 commits intoapache:mainfrom
scovich:no-arc-arc-array
Feb 3, 2026
Merged

Remove unnecessary Arc<ArrayRef>#9316
scovich merged 5 commits intoapache:mainfrom
scovich:no-arc-arc-array

Conversation

@scovich
Copy link
Contributor

@scovich scovich commented Jan 31, 2026

Which issue does this PR close?

Rationale for this change

Because Arc<dyn Array> (ArrayRef) implements Array, it's easy to create redundant Arc<Arc<dyn Array>>. The extra indirection is pure overhead (both cognitive and runtime) with no benefit.

What changes are included in this PR?

Remove the redundant wrapping.

Are these changes tested?

Compilation suffices -- failing to wrap a concrete array type in Arc would cause compilation failures.

Are there any user-facing changes?

No.

@scovich scovich requested review from alamb and tustvold January 31, 2026 03:32
@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate parquet-variant parquet-variant* crates labels Jan 31, 2026
@scovich scovich mentioned this pull request Jan 31, 2026
Open
alamb
alamb approved these changes Feb 2, 2026
View reviewed changes
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.

Looks great to me

arrow-arith/src/temporal.rs
DataType::Dictionary(_, _) => {
let array = array.as_any_dictionary();
let values = date_part(array.values(), part)?;
let values = Arc::new(values) as ArrayRef;
Copy link
Contributor

@alamb alamb Feb 2, 2026

Choose a reason for hiding this comment

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

🤦

@scovich
Copy link
Contributor Author

scovich commented Feb 2, 2026

Note: This PR is really just fixing the examples I found so far. I suspect I found them all (by removing the impl Array for Arc<dyn Array> and seeing what broke), but nothing stops new bad examples from showing up in the future. I even found a bad example in my own code (variant builder), so I know it's easy to miss!

@scovich
Copy link
Contributor Author

scovich commented Feb 2, 2026

@alamb -- something seems to have gone wrong with CI -- one job failed to acquire an instance to run on, and after 15 minutes everything canceled. Is that normal?

@alamb
Copy link
Contributor

alamb commented Feb 2, 2026

Close/reopen to retrigger CI

@alamb alamb closed this Feb 2, 2026
@alamb alamb reopened this Feb 2, 2026
@scovich
Copy link
Contributor Author

scovich commented Feb 2, 2026 •
edited
Loading

Same thing again...

arrow_flight / Clippy (pull_request) Canceled after 76m
The job was not acquired by Runner of type hosted even after multiple attempts

@alamb
Copy link
Contributor

alamb commented Feb 2, 2026

https://www.githubstatus.com/

Screenshot 2026-02-02 at 6 13 41 PM

Appears to be github related

@Jefffrey Jefffrey closed this Feb 3, 2026
@Jefffrey Jefffrey reopened this Feb 3, 2026
@scovich scovich merged commit 53a1614 into apache:main Feb 3, 2026
34 of 96 checks passed
@alamb
Copy link
Contributor

alamb commented Feb 3, 2026

Woohoo!

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

Reviewers

@alamb alamb alamb approved these changes

@Jefffrey Jefffrey Jefffrey approved these changes

@tustvold tustvold Awaiting requested review from tustvold

Assignees

No one assigned

Labels

arrow Changes to the arrow crate parquet Changes to the parquet crate parquet-variant parquet-variant* crates

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@scovich @alamb @Jefffrey