-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Split SessionState
into its own module
#10794
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name it as session_state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in 0c1dbbe
@@ -1280,7 +1253,7 @@ impl SessionContext { | |||
/// `query_execution_start_time` to the current time | |||
pub fn state(&self) -> SessionState { | |||
let mut state = self.state.read().clone(); | |||
state.execution_props.start_execution(); | |||
state.execution_props_mut().start_execution(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need mut here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed because the start execution method actually modifies the execution_props field -- when SessionContext was part of the same module, it could simply directly modify state.execution_props
(the field, not a method)
Now that they are in separate modules, SessionContext can't modify SessionState
internal fields directly.
So TLDR is that this code does the same as the previous version (modifies execution_props
) but is more explicit about it, which I think is an improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @alamb but this might require downstream user changes?
use url::Url; | ||
use uuid::Uuid; | ||
|
||
/// Execution context for registering data sources and executing queries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just moved from context/mod.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure!
}; | ||
use sqlparser::dialect::dialect_from_str; | ||
|
||
// backwards compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pub use
means there should be no downstream changes required
Sorry I should have pointed that out -- I left a |
Thanks again for the reviews |
* Split `SessionState` into its own module * fix docs * fix test * Rename state.rs to session_state.rs
Which issue does this PR close?
Part of #10782
Rationale for this change
the
datafusion/core/src/execution/context/mod.rs
file is quite large and hard to navigate.It would be better to have SessionContext and SessionState in their own modules
Also, if we ever are able to break out catalog from the core, e.g. #10782 we may have to break out
SessionState
as well, so having SessionState in its own module will help hereIt also turns out splitting them into their own files makes the boundaries a bit clearer
What changes are included in this PR?
SessionState
out ofdatafusion/core/src/execution/context/mod.rs
and intodatafusion/core/src/execution/state.rs
pub use
SessionState
to maintain the same behaviorAre these changes tested?
Exiting CI
Are there any user-facing changes?
No, this is an internal code reorganization only