adriangb commented on code in PR #26353:
URL: https://github.com/apache/datafusion/pull/26353#discussion_r4330292048
##########
datafusion/execution/src/config.rs:
##########
@@ -681,6 +681,39 @@ impl SessionConfig {
{
self.extensions.get_arc::<T>()
}
+
+ /// Change the extension of type `T` in place, and return what `f` returns.
+ ///
+ /// If there is no extension of type `T`, `f` gets `T::default()`, which is
+ /// then attached. If the extension is shared with another config, it is
+ /// cloned first, so the other config does not change.
+ ///
+ /// Use this, not [`with_extension`](Self::with_extension), when more than
+ /// one part of an application adds to the same extension. `with_extension`
+ /// replaces the extension, so it discards what other parts added.
+ ///
+ /// # Example
+ /// ```
+ /// use datafusion_execution::config::SessionConfig;
+ ///
+ /// #[derive(Clone, Default)]
+ /// struct Names(Vec<&'static str>);
+ ///
+ /// let mut cfg = SessionConfig::default();
+ /// cfg.update_extension(|names: &mut Names| names.0.push("a"));
+ /// cfg.update_extension(|names: &mut Names| names.0.push("b"));
+ ///
+ /// assert_eq!(cfg.get_extension::<Names>().unwrap().0, ["a", "b"]);
+ /// ```
+ pub fn update_extension<T, R>(&mut self, f: impl FnOnce(&mut T) -> R) -> R
+ where
+ T: Clone + Default + Send + Sync + 'static,
Review Comment:
Thanks! `Clone` has to stay: the extension sits behind a shared `Arc`, and
`Arc::make_mut` needs `Clone` to change it without affecting other configs that
share it. A type that is not `Clone` can be wrapped, for example in
`Arc<Mutex<T>>`.
For `Default`, I see four options:
1. Keep `Default` (this PR as it is). The call is simple, and any library
can create the extension the first time it is used. The cost: types that can't
implement `Default` can't use it.
2 Take an `init` closure: `update_extension(init: impl FnOnce() -> T, f)`,
called as `cfg.update_extension(ProtoDecoderRegistry::new, |r| ...)`. It needs
no `Default` and is still one method, but every call passes an extra argument.
3. Fail when the extension is missing, returning `Option<R>` or `Result<R>`.
This has the fewest bounds, but someone must insert the extension first.
Libraries would then have to coordinate who inserts, which is what this PR
tries to avoid.
4. An entry-style API, like `HashMap::entry`:
`cfg.extension_entry::<T>().or_default()` or `.or_insert_with(init)`, which
return `&mut T`. This is the most idiomatic, and only `or_default` needs
`Default`. It is more API, and it needs changes to how `Extensions` stores
values, so I'd do it in a follow-up.
Do you have any preference?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
