POC: Add ConfigOptions to ExecutionProps when execution is started
#16661
Conversation
github-actions
bot
added
logical-expr
| @@ -152,7 +157,7 @@ impl SessionConfig { | |||
| /// assert_eq!(config.options().execution.batch_size, 1024); | |||
| /// ``` | |||
| pub fn options_mut(&mut self) -> &mut ConfigOptions { | |||
| &mut self.options | |||
| Arc::make_mut(&mut self.options) | |||
There was a problem hiding this comment.
This is the key function call -- it basically does "copy on write" and if there are existing references to the options a new copy is created.
There was a problem hiding this comment.
Previously the cost of options_mut was small, now it's considerable.
Not sure it matters, but worth calling out.
There was a problem hiding this comment.
Previously the cost of options_mut was small, now it's considerable.
Not sure it matters, but worth calling out.
Technically I think it is considerable only on the first call -- subsequent calls will use the same copy (as I understand from the make_mut docs https://doc.rust-lang.org/std/sync/struct.Arc.html#method.make_mut)
| /// Configuration options | ||
| options: ConfigOptions, | ||
| /// Configuration options, copy on write | ||
| options: Arc<ConfigOptions>, |
There was a problem hiding this comment.
The idea is to pass this pointer down into the ExecutionProps
| @@ -152,7 +157,7 @@ impl SessionConfig { | |||
| /// assert_eq!(config.options().execution.batch_size, 1024); | |||
| /// ``` | |||
| pub fn options_mut(&mut self) -> &mut ConfigOptions { | |||
| &mut self.options | |||
| Arc::make_mut(&mut self.options) | |||
There was a problem hiding this comment.
Previously the cost of options_mut was small, now it's considerable.
Not sure it matters, but worth calling out.
| /// Returns the configuration properties for this execution | ||
| /// if the execution has started | ||
| pub fn config_options(&self) -> Option<&Arc<ConfigOptions>> { |
There was a problem hiding this comment.
the with_query_execution_start_time makes / used to make ExecutionProps "as started"
these call sites should be updated as well
|
I don't have time to push this along for the next few days, but maybe we can restart it if / when @Omega359 needs access to the config options |
Which issue does this PR close?
I think @Omega359 had other PRs as well
Relates to
SessionConfigreference toScalarFunctionArgs#13519datafusion.execution.time_zoneis not used for basic time zone inference #13212Rationale for this change
The need is to access configuration settings (such as the configured timezone) to scalar functions during planning
What changes are included in this PR?
Add an
Arc<ConfigOptions>to ExecutionProps when the execution is startedThings I am still not happy about:
Are these changes tested?
Are there any user-facing changes?