chore: Remove references from SessionState from physical_plan #5455
Conversation
| @@ -280,7 +280,7 @@ impl FileOpener for CsvOpener { | |||
| } | |||
|
|
|||
| pub async fn plan_to_csv( | |||
| state: &SessionState, | |||
| task_ctx: Arc<TaskContext>, | |||
There was a problem hiding this comment.
The point of the pR is to remove the use of SessionState and hoist the creation of TaskContext into SessionContext
| @@ -300,8 +300,7 @@ pub async fn plan_to_csv( | |||
| let path = fs_path.join(filename); | |||
| let file = fs::File::create(path)?; | |||
| let mut writer = csv::Writer::new(file); | |||
| let task_ctx = Arc::new(TaskContext::from(state)); | |||
| let stream = plan.execute(i, task_ctx)?; | |||
| let stream = plan.execute(i, task_ctx.clone())?; | |||
There was a problem hiding this comment.
Note that TaskContext is simply a clone of the state on SessionState: https://github.com/apache/arrow-datafusion/blob/a95e0ec2fd929aae1c2f67148243eb4825d81a3b/datafusion/core/src/execution/context.rs#L2157-L2173
so making it once and cloning is probably better than making multiple TaskContexts (each that have a bunch of Arcs)
| @@ -803,6 +802,7 @@ mod tests { | |||
| use crate::datasource::file_format::test_util::scan_format; | |||
| use crate::datasource::listing::{FileRange, PartitionedFile}; | |||
| use crate::datasource::object_store::ObjectStoreUrl; | |||
| use crate::execution::context::SessionState; | |||
There was a problem hiding this comment.
This is for tests, which I think is ok to depend on datafusion core
|
@metesynnada / @mustafasrepo might you have time to review this change? I also think this may be be on code that @metesynnada is working on for INSERT / COPY TO. |
|
LGTM!. Thanks for this PR. |
Which 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; SessionState is in
datafusion-corewhich means physical_plan can't have references back there.See more details in #1754 (comment)
What changes are included in this PR?
Change some code to use
TaskContextrather thanSessionState(that is simply used to make aTaskContext)Are these changes tested?
Covered by existing tests
Are there any user-facing changes?
I don't think anyone uses these APIs (they are helpers from SessionContext) but I think they are technically public