Allow setting config extensions for TaskContext #5497
Conversation
There was a problem hiding this comment.
Thank you @mpurins-coralogix
I had a suggestion on the API design, but I consider it minor and I think this PR could be merged as is
| @@ -2097,27 +2097,28 @@ pub struct TaskContext { | |||
|
|
|||
| impl TaskContext { | |||
| /// Create a new task context instance | |||
| pub fn new( | |||
| pub fn try_new( | |||
There was a problem hiding this comment.
👍 for the change to make this fallible
datafusion/common/src/config.rs
Outdated
| @@ -397,6 +397,13 @@ impl ConfigOptions { | |||
| Self::default() | |||
| } | |||
|
|
|||
| /// Creates a new [`ConfigOptions`] with extensions set to provided value | |||
| pub fn with_extensions(extensions: Extensions) -> Self { | |||
There was a problem hiding this comment.
What would you think about making this a builder style API?
Like
| pub fn with_extensions(extensions: Extensions) -> Self { | |
| pub fn with_extensions(mut self, extensions: Extensions) -> Self { |
So one could modify an existing ConfigOptions like:
let options = ConfigOptions::new()
.with_extensions(extensions)There was a problem hiding this comment.
The benefit of this style is that it naturally extends to setting other fields (rather than having to have a specialize constructor for each field that we want to set)
There was a problem hiding this comment.
Would with builder api it would initially create one redundant instance of Extensions?
Anyway it likely doesn't really matter and I agree that it would be nicer to use. Done.
|
Mac Ci failure seemed unrelated to code changes in this PR https://github.com/apache/arrow-datafusion/actions/runs/4355096606/jobs/7612772747 so I retriggered it |
|
Benchmark runs are scheduled for baseline = 0ead640 and contender = 8a1b133. 8a1b133 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?
Closes #5496.
Rationale for this change
What changes are included in this PR?
ConfigOptions::with_extensionsconstructor to create new config with some default extensions.TaskContext::newto accept config extensions to create new config using that.TaskContext::newtotry_newand added return type so that any error is not ignored when setting config optionAre these changes tested?
One test added.
Are there any user-facing changes?
TaskContext::newchanged toTaskContext::try_newwith new argument added and which now returns result