You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/18 16:13:22 UTC
[GitHub] [arrow-datafusion] crepererum opened a new pull request, #2940: add extension system to `SessionConfig`
crepererum opened a new pull request, #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940
# Which issue does this PR close?
Closes #2939.
# Rationale for this change
This is helpful for application built on top of DataFusion that want to
pass certain query-scoped data structures around, e.g. for tracing or
caching.
# What changes are included in this PR?
Allow `SessionConfig` to carry opaque extensions around.
This is loosely inspired by:
https://github.com/hyperium/http/blob/34a9d6bdab027948d6dea3b36d994f9cbaf96f75/src/extensions.rs#L6-L28
# Are there any user-facing changes?
`SessionConfig::{with,get}_extension`. No breaking change.
--
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] codecov-commenter commented on pull request #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#issuecomment-1188784198
# [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2940](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (39df2d6) into [master](https://codecov.io/gh/apache/arrow-datafusion/commit/944ef3d6eeb5e3b31c5561df9b7b8cbdef7367f5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (944ef3d) will **decrease** coverage by `0.02%`.
> The diff coverage is `7.69%`.
> :exclamation: Current head 39df2d6 differs from pull request most recent head 3e7073a. Consider uploading reports for the commit 3e7073a to get more accurate results
```diff
@@ Coverage Diff @@
## master #2940 +/- ##
==========================================
- Coverage 85.44% 85.42% -0.03%
==========================================
Files 275 275
Lines 49690 49703 +13
==========================================
+ Hits 42458 42459 +1
- Misses 7232 7244 +12
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [datafusion/core/src/execution/context.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2940/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `77.95% <7.69%> (-1.12%)` | :arrow_down: |
| [datafusion/expr/src/window\_frame.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2940/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy93aW5kb3dfZnJhbWUucnM=) | `92.43% <0.00%> (-0.85%)` | :arrow_down: |
| [datafusion/expr/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow-datafusion/pull/2940/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZGF0YWZ1c2lvbi9leHByL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `77.14% <0.00%> (+0.17%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [944ef3d...3e7073a](https://codecov.io/gh/apache/arrow-datafusion/pull/2940?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] mingmwang commented on pull request #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
mingmwang commented on PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#issuecomment-1204724203
I'm not sure whether this extension points should be added to RuntimeEnv or RuntimeConfig or SessionConfig.
My original understanding is SessionConfig just hold the configuration entries to control the planer(logical/physical) and runtime behavior. Those configuration settings are session specific and can be changed on the fly in a distributed multi tenant system(Ballista). And RuntimeEnv and RuntimeConfig hold the the static configurations which are shared among multiple users and can not changed on the fly.
--
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 #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
alamb commented on code in PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#discussion_r923635322
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1173,6 +1215,29 @@ impl SessionConfig {
);
map
}
+
+ /// Add extensions.
+ pub fn with_extension<T>(mut self, ext: Arc<T>) -> Self
+ where
+ T: Send + Sync + 'static,
+ {
+ let ext = ext as Arc<dyn Any + Send + Sync + 'static>;
+ let id = TypeId::of::<T>();
+ self.extensions.insert(id, ext);
+ self
+ }
+
+ /// Get extension.
Review Comment:
```suggestion
/// Get extension, if any, for the specific Id
```
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1173,6 +1215,29 @@ impl SessionConfig {
);
map
}
+
+ /// Add extensions.
Review Comment:
Can you possibly add some documentation on what one might do with extensions (e.g. attach arbitrary data to nodes) and mention the system specific metric usecase?
Perhaps moving the test into a docstring comment would be the most helpful for future readers to discover this functionality
It is probably also good to document that only a single entry for at type is allowed (e.g. what happens if you register two extensions with the same type).
--
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] crepererum commented on a diff in pull request #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
crepererum commented on code in PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#discussion_r924206887
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1173,6 +1215,29 @@ impl SessionConfig {
);
map
}
+
+ /// Add extensions.
+ pub fn with_extension<T>(mut self, ext: Arc<T>) -> Self
+ where
+ T: Send + Sync + 'static,
+ {
+ let ext = ext as Arc<dyn Any + Send + Sync + 'static>;
+ let id = TypeId::of::<T>();
+ self.extensions.insert(id, ext);
+ self
+ }
+
+ /// Get extension.
Review Comment:
"..for specific **type**". The ID is invisible to the user. I've heavily extended the docs to make that clearer.
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1173,6 +1215,29 @@ impl SessionConfig {
);
map
}
+
+ /// Add extensions.
Review Comment:
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 #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#issuecomment-1188921979
Benchmark runs are scheduled for baseline = 944ef3d6eeb5e3b31c5561df9b7b8cbdef7367f5 and contender = 9aa38c3c73856fe7e93d67b29cc2c9ce892db3f8. 9aa38c3c73856fe7e93d67b29cc2c9ce892db3f8 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/507a146d2ae84e3c8009e0ab3ffae188...d69f263b99ef4d3683c57af9f2ff756e/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/7071456bd3b841f4a726221a63fffab8...b72ade7d1a424c06908e76d5cee1414f/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3e239b1202d243ae83d593e419a71cd7...49507037dc3b41e29712db1bb273e34d/)
[Skipped :warning: Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/560bbabf683d4c249a60372509fb6706...85723c4244ee4fc99f6bffa901011d0f/)
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
[GitHub] [arrow-datafusion] alamb merged pull request #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
alamb merged PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940
--
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] crepererum commented on pull request #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
crepererum commented on PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#issuecomment-1204906199
@mingmwang we can for sure have extension for `RuntimeEnv` and `RuntimeConfig` as well.
--
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 #2940: add extension system to `SessionConfig`
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #2940:
URL: https://github.com/apache/arrow-datafusion/pull/2940#issuecomment-1205090790
I agree that a similar system for `RuntimeEnv` would be a reasonable idea
--
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