Return &Arc reference to inner trait object
#11103
Conversation
github-actions
bot
added
logical-expr
|
Looks like the CI is failing -- I think running |
|
Thank you @alamb ! I've fixed the code formatting issue. |
| @@ -236,13 +236,14 @@ mod tests { | |||
| fn setup_context() -> (SessionContext, Arc<dyn SchemaProvider>) { | |||
| let mut ctx = SessionContext::new(); | |||
| ctx.register_catalog_list(Arc::new(DynamicFileCatalog::new( | |||
| ctx.state().catalog_list(), | |||
| ctx.state().catalog_list().clone(), | |||
There was a problem hiding this comment.
when I see .clone() Im now thinking what if we comment it the similar way as .unwrap() back in the day. Like say clone is cheap here because it is Arc::clone so only reference gets cloned.... thats just an idea, its too cumbersome to make it happen
There was a problem hiding this comment.
One thing we do in InfluxDB is use this pattern to make it clear
| ctx.state().catalog_list().clone(), | |
| Arc::clone(ctx.state().catalog_list()) |
to make it explicit.
There is a clippy lint https://rust-lang.github.io/rust-clippy/v0.0.212/#clone_on_ref_ptr we could turn on in datafusion to enable this
Here is how it is done in influxdb:
There was a problem hiding this comment.
This is a good idea, let's do it
* Return `&Arc` reference to inner trait object * Format code
Which issue does this PR close?
Closes #11100.
Rationale for this change
It's better to return
&Arcreference to inner trait object for&selfmethods. This makes it easier to work with Rust lifetime rules when we need to cast trait object to concrete reference types. Please refer to the corresponding issue for an example.What changes are included in this PR?
This PR updates the return type for few methods in
ScalarUDF,AggregateUDF,WindowUDF, andSessionState.Are these changes tested?
The changes are covered by existing tests.
Are there any user-facing changes?
Yes, these are breaking changes to the public API. The changes should be small, since the user can simply call
.clone()on&Arcif needed.