Repository navigation
add extension system to SessionConfig - #2940
Conversation
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @crepererum -- I like the code 👍 I have some documentation suggestions but otherwise looks good to go to me
| map | ||
| } | ||
|
|
||
| /// Add extensions. |
There was a problem hiding this 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).
| self | ||
| } | ||
|
|
||
| /// Get extension. |
There was a problem hiding this comment.
| /// Get extension. | |
| /// Get extension, if any, for the specific Id |
There was a problem hiding this comment.
"..for specific type". The ID is invisible to the user. I've heavily extended the docs to make that clearer.
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. This is loosely inspired by: https://github.com/hyperium/http/blob/34a9d6bdab027948d6dea3b36d994f9cbaf96f75/src/extensions.rs#L6-L28 Closes apache#2939.
2bc9267 to
3e7073a
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
Benchmark runs are scheduled for baseline = 944ef3d and contender = 9aa38c3. 9aa38c3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
I'm not sure whether this extension points should be added to RuntimeEnv or RuntimeConfig or SessionConfig. |
|
@mingmwang we can for sure have extension for |
|
I agree that a similar system for |
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
SessionConfigto 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.