Skip to content

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 28, 2024

Which issue does this PR close?

N/A

Rationale for this change

Small follow on to #12157 from @devanbenz

It is still somewhat awkward to make RuntimeEnv's, so let's make it slightly easier

What changes are included in this PR?

Adds a convenience method to RuntimeEnvBuilder to build an Arc<RuntimeEnv>

Are these changes tested?

Yes, by CI

Are there any user-facing changes?

A new method

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates core Core DataFusion crate execution Related to the execution crate labels Aug 28, 2024
.build()
.unwrap(),
);
let runtime = RuntimeEnvBuilder::new()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a pretty good example of how the new API makes the code simpler

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

I like the simplification.
Is this an established naming pattern in the ecosystem?

@alamb alamb marked this pull request as ready for review September 2, 2024 11:19
@alamb
Copy link
Contributor Author

alamb commented Sep 2, 2024

I like the simplification. Is this an established naming pattern in the ecosystem?

I am not aware of this being an established pattern (I think DataFusion uses Arc's a bit more than many Rust purists would prefer)

Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

@jayzhan211
Copy link
Contributor

I also have the same thought, maybe we don't really need Arc in some cases. We could gradually cleanup them later on

@jayzhan211 jayzhan211 merged commit ac74cd3 into apache:main Sep 2, 2024
24 checks passed
@jayzhan211
Copy link
Contributor

Thanks @alamb and @findepi

@alamb alamb deleted the alamb/build_arc branch September 5, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate execution Related to the execution crate physical-expr Changes to the physical-expr crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants