Config Cleanup: Remove TaskProperties and KV structure, keep key=value serialization #4382
Conversation
There was a problem hiding this comment.
cc @liukun4515 and @yahoNanJing as I believe you follow ballista closely
| /// Task Context Properties | ||
| pub enum TaskProperties { | ||
| ///SessionConfig | ||
| SessionConfig(SessionConfig), |
There was a problem hiding this comment.
The core of the change is to just use SessionConfig everywhere, creating it in the constructor of TaskContext if needed
| aggregate_functions, | ||
| runtime, | ||
| } | ||
| TaskContext::from(&*session.state.read()) |
There was a problem hiding this comment.
this is a drive by clean up as the code was replicated in impl From<&SessionState> for TaskContext {
| scalar_functions, | ||
| aggregate_functions, | ||
| runtime, | ||
| } | ||
| } | ||
|
|
||
| /// Return the SessionConfig associated with the Task | ||
| pub fn session_config(&self) -> SessionConfig { |
There was a problem hiding this comment.
this conversion from string/value properties to SessionConfig is moved to TaskContext::new
|
I will take a closer look at the configuration related issues today. |
|
I'm OK with the change. |
|
I plan to merge this tomorrow unless I hear otherwise and then will likely work on chipping away at the random parquet settings that are sprinkled around to make them visible via |
|
Benchmark runs are scheduled for baseline = 02da32e and contender = 1438bc4. 1438bc4 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?
re #4349
Rationale for this change
Step 1 of N in unraveling the gordian knot of datafusion configuration
While there is a need to serialize/deserialize session state as key=value pairs in SessionContext, there is no reason these key / value pairs need to be kept in the actual TaskContext after it is created and having the different representation makes the eventual unification of configuration that much harder.
What changes are included in this PR?
FromimplsTaskPropertiesand just useSessionConfigeverywhereAre these changes tested?
covered by existing tests
Are there any user-facing changes?
If you use TaskProperties directly you'll be impacted, but I don't think anyone does. It is not used by ballista https://github.com/search?q=repo%3Aapache%2Farrow-ballista%20KVPairs&type=code
@mingmwang this code was originally added by you in #1987 -- can you take a look ?