Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: get mutable ref to SessionConfig in SessionState #10050

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

MichaelScofield
Copy link
Contributor

@MichaelScofield MichaelScofield commented Apr 12, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

The SessionConfig has an "extensions: AnyMap", can be used "... to attach extra data to the session config -- e.g. tracing information or caches."(from with_extension's docs). However, there's no place to add an extension from the SessionState site, 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_mut in SessionState to get the mutable ref to SessionConfig to 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.

@github-actions github-actions bot added the core Core datafusion crate label Apr 12, 2024
@MichaelScofield
Copy link
Contributor Author

@alamb can you take a look?

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.

Thanks @MichaelScofield -- the basic idea looks great. I have some comments on the specific API

@@ -500,7 +500,7 @@ impl SessionConfig {
/// ```
///
/// [^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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelScofield can you change the PR to not change the public API (keep with_extensions and add set_extensions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alamb I've changed the PR according to your suggestions, PTAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@milenkovicm we are using the SessionState a little differently in that it's not created everytime.

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.

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.

@alamb alamb merged commit 8604d0a into apache:main Apr 15, 2024
25 checks passed
@MichaelScofield MichaelScofield deleted the chore/mut-config-in-session-state branch April 15, 2024 10:58
@milenkovicm
Copy link
Contributor

@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 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants