Move SessionConfig to datafusion_execution
#5581
Conversation
github-actions
bot
added
core
alamb
force-pushed
the
alamb/move_session_config
branch
from
2d2acb7
to
5bdcb64
Compare
|
|
||
| /// Configuration options for Execution context | ||
| #[derive(Clone)] | ||
| pub struct SessionConfig { |
There was a problem hiding this comment.
At first it seemed strange to have something called "SessionConfig" in an execution crate and I considered renaming it to ExecutionConfig or TaskConfig, but since both SessionState and TaskContext refer to this (and it is properly session scoped) I decided the SessionConfig name was OK
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| use std::{ |
There was a problem hiding this comment.
The code in this module was simply moved from datafusion/core/src/execution/context.rs
| @@ -1161,344 +1159,6 @@ impl QueryPlanner for DefaultQueryPlanner { | |||
| } | |||
| } | |||
|
|
|||
| /// Map that holds opaque objects indexed by their type. | |||
There was a problem hiding this comment.
Code moved to datafusion-execution
alamb
force-pushed
the
alamb/move_session_config
branch
from
5bdcb64
to
768cb38
Compare
| /// Return a handle to the configuration options. | ||
| #[deprecated(since = "21.0.0", note = "use options() instead")] | ||
| pub fn config_options(&self) -> &ConfigOptions { | ||
| &self.options | ||
| } | ||
|
|
||
| /// Return a mutable handle to the configuration options. | ||
| #[deprecated(since = "21.0.0", note = "use options_mut() instead")] | ||
| pub fn config_options_mut(&mut self) -> &mut ConfigOptions { | ||
| &mut self.options | ||
| } |
There was a problem hiding this comment.
My rationale was to keep the naming consistent (the field is called options but the method was called config_options()`)
Also, when working with the various config often you would see
ctx.session_config().config_options(),which seemed redundant. With this change it looks like
ctx.session_config().options(),
There was a problem hiding this comment.
I can avoid the rename if people prefer that
Draft as builds on #5580Which issue does this PR close?
Part of #1754
Rationale for this change
I am trying to extract the physical_plan code into its own crate; and to do so I need to remove the circular dependencies between core --> datasource --> execution --> datasource
The
TaskContextis the part of theSessionStatethat is used at runtime, so I want it it into thedatafusion_executioncrate.SessionConfigis used inTaskContextso it needs to go firstSee more details in #1754 (comment)
What changes are included in this PR?
SessionConfigtodatafusion_executionSessionConfig::options()/SessionConfig::options_mut()to mirror the field name and reduce redundancySessionConfig::config_options()/SessionConfig::config_options_mut()deprecatedAre these changes tested?
Covered by existing tests
Are there any user-facing changes?
I added a
pub useso this code movement should not affect users other than the deprecated APINote I also explored leaving
SessionConfigin datafusion-core (see #5582) but I felt this was too disruptive and preferred to minimize API churn for users