Push SessionState into FileFormat (#4349) #4699
Conversation
| @@ -254,7 +258,7 @@ mod tests { | |||
| projection: Some(vec![0, 1, 2]), | |||
| limit: None, | |||
| table_partition_cols: vec![], | |||
| config_options: ConfigOptions::new().into_shareable(), | |||
| config_options: ctx.config_options(), | |||
There was a problem hiding this comment.
I fully intend to remove this in a future PR, this should be retrieved from the TaskContext not baked into FileScanConfig
datafusion/core/tests/row.rs
Outdated
| @@ -84,7 +76,7 @@ async fn test_with_parquet_word_aligned() -> Result<()> { | |||
| } | |||
|
|
|||
| async fn get_exec( | |||
| ctx: &SessionContext, | |||
| ctx: &SessionState, | |||
There was a problem hiding this comment.
Taking SessionState here is more correct as it allows the query to be executed against the same state
There was a problem hiding this comment.
Thanks -- looks good to me, thank you @tustvold
I have only a minor quibble about the name used for variables of SessionState type (ctx) being confusing. Otherwise 👍
| @@ -47,6 +48,7 @@ impl FileFormat for AvroFormat { | |||
|
|
|||
| async fn infer_schema( | |||
| &self, | |||
| _ctx: &SessionState, | |||
There was a problem hiding this comment.
I agree this makes much more sense 👍
| @@ -124,9 +129,10 @@ mod tests { | |||
| #[tokio::test] | |||
| async fn read_limit() -> Result<()> { | |||
| let session_ctx = SessionContext::new(); | |||
| let task_ctx = session_ctx.task_ctx(); | |||
| let ctx = session_ctx.state(); | |||
There was a problem hiding this comment.
maybe state would be a better name than ctx as there is already session_ctx which is a Context 🤔
| @@ -1257,13 +1265,13 @@ mod tests { | |||
| } | |||
|
|
|||
| async fn get_exec( | |||
| ctx: &SessionContext, | |||
| ctx: &SessionState, | |||
There was a problem hiding this comment.
I find the ctx name confusing
There was a problem hiding this comment.
Yeah, I was just copying the naming used in things like TableProvider - I don't disagree that it is confusing. Renamed
|
I plan to merge this once CI turns green, so that I can base some further changes off it |
|
Benchmark runs are scheduled for baseline = 4917235 and contender = c9d6118. c9d6118 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #4349
Relates to #4617
Relates to #3909 #3886
Rationale for this change
#4427 pushed
ConfigOptionsintoParquetFormat, however, the resultingArc<Mutex<ConfigOptions>>is tricky to work with (#3886) and has unclear mutation semantics (#4617).What changes are included in this PR?
Pushes
SessionStateintoFileFormatwhich in turn will allow removingConfigOptionsfromParquetFormatand instead just fetching the configuration from the passed inSessionStateAre these changes tested?
Are there any user-facing changes?