You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mpurins-coralogix (via GitHub)" <gi...@apache.org> on 2023/03/07 12:57:53 UTC
[GitHub] [arrow-datafusion] mpurins-coralogix opened a new pull request, #5497: Allow setting config extensions for TaskContext
mpurins-coralogix opened a new pull request, #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497
# Which issue does this PR close?
Closes #5496.
# Rationale for this change
# What changes are included in this PR?
- Adds `ConfigOptions::with_extensions` constructor to create new config with some default extensions.
- Chages `TaskContext::new` to accept config extensions to create new config using that.
- Additionaly I renamed `TaskContext::new` to `try_new` and added return type so that any error is not ignored when setting config option
# Are these changes tested?
One test added.
# Are there any user-facing changes?
`TaskContext::new` changed to `TaskContext::try_new` with new argument added and which now returns result
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5497: Allow setting config extensions for TaskContext
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497#issuecomment-1458882389
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5497: Allow setting config extensions for TaskContext
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497#discussion_r1127907690
##########
datafusion/core/src/execution/context.rs:
##########
@@ -2097,27 +2097,28 @@ pub struct TaskContext {
impl TaskContext {
/// Create a new task context instance
- pub fn new(
+ pub fn try_new(
Review Comment:
👍 for the change to make this fallible
##########
datafusion/common/src/config.rs:
##########
@@ -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 {
Review Comment:
What would you think about making this a builder style API?
Like
```suggestion
pub fn with_extensions(mut self, extensions: Extensions) -> Self {
```
So one could modify an existing ConfigOptions like:
```rust
let options = ConfigOptions::new()
.with_extensions(extensions)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5497: Allow setting config extensions for TaskContext
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497#discussion_r1127909977
##########
datafusion/common/src/config.rs:
##########
@@ -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 {
Review Comment:
The benefit of this style is that he naturally extends to setting other fields (rather than having to have a specialize constructor for each field that we want to set)
##########
datafusion/common/src/config.rs:
##########
@@ -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 {
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #5497: Allow setting config extensions for TaskContext
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] mpurins-coralogix commented on a diff in pull request #5497: Allow setting config extensions for TaskContext
Posted by "mpurins-coralogix (via GitHub)" <gi...@apache.org>.
mpurins-coralogix commented on code in PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497#discussion_r1127937243
##########
datafusion/common/src/config.rs:
##########
@@ -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 {
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] ursabot commented on pull request #5497: Allow setting config extensions for TaskContext
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #5497:
URL: https://github.com/apache/arrow-datafusion/pull/5497#issuecomment-1460840336
Benchmark runs are scheduled for baseline = 0ead640e887c19607a371c22a183dc91a55baf45 and contender = 8a1b1339052b57ac01d730a57e2f3bb321b103ba. 8a1b1339052b57ac01d730a57e2f3bb321b103ba is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/d3a4b0f5726940cabd26d9b7f7d230ca...d625af0213cc4147b67520bcd9470ed5/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/32f904b863fc4e3e8329168d33ca99fd...318bede8a38042408eff340989c2e76e/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ad7aee88ec9d4e63964a23f1211338a9...fdbbeac14dd5430fbe74dce1e9d3ef00/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/7a955df7ffe74ef5b3967f67ebb17860...7eebae0a83c541b69eadd243c426f1f9/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org