Repository navigation
Minor: get mutable ref to SessionConfig in SessionState - #10050
Conversation
|
@alamb can you take a look? |
alamb
left a comment
There was a problem hiding this comment.
Thanks @MichaelScofield -- the basic idea looks great. I have some comments on the specific API
| /// | ||
| /// [^1]: Compare that to [`ConfigOptions`] which only supports [`ScalarValue`] payloads. | ||
| pub fn with_extension<T>(mut self, ext: Arc<T>) -> Self | ||
| pub fn with_extension<T>(&mut self, ext: Arc<T>) -> &mut Self |
There was a problem hiding this comment.
This is an API change and some people might like to be able to create a SessionConfig with a builder style
I think we should add a new function for updating an existing SessionConfig like this
pub fn set_extension<T>(&mut self, ext: Arc<T>) {
...
}
(ideally also with an example)
cc @milenkovicm as we also discussed various ways to make adding extensions easier
There was a problem hiding this comment.
we talked about the other extensions
self.options_mut().extensions ...which i always mix with this extensions 😀
I wonder if it make sense to register extension with internal mutability at the session creation and mutate that extension from your optimizer/planner? 🤔
There was a problem hiding this comment.
@MichaelScofield can you change the PR to not change the public API (keep with_extensions and add set_extensions)?
There was a problem hiding this comment.
@alamb I've changed the PR according to your suggestions, PTAL.
There was a problem hiding this comment.
@milenkovicm we are using the SessionState a little differently in that it's not created everytime.
alamb
left a comment
There was a problem hiding this comment.
Awesome -- thank you so much @MichaelScofield 🙏
@milenkovicm as you have been using the extension points, any thing you can do or suggest to make using them easier would be awesome as well. Thank you for your help so far.
I think they work ok, I cant think anything to suggest. Thanks for asking A situation we had, we could not retrieve registered extension (regular extension, not the config extension), at the end it was build issue as there were two different versions of the same crate used, with the extension struct in it, (our fault). Peace of code which registered extension used first version, part of code which retrieved extension used second version (so different typeid) ... eventually we figured it out but it was a head scratcher 😀 |
Which issue does this PR close?
Closes #.
Rationale for this change
The
SessionConfighas an "extensions: AnyMap", can be used "... to attach extra data to the session config -- e.g. tracing information or caches."(fromwith_extension's docs). However, there's no place to add an extension from theSessionStatesite, which limits its functionality, especially when the attached extra data is wanted in optimizing physical plan stage.What changes are included in this PR?
This PR adds a
config_mutinSessionStateto get the mutable ref toSessionConfigto resolve the issue above.Are these changes tested?
Covered, and in doc-test.
Are there any user-facing changes?
Two new methods are added, no breaking.