gabotechs commented on code in PR #26353:
URL: https://github.com/apache/datafusion/pull/26353#discussion_r4312797931
##########
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:
Nice! Is there a chance we can remove the `Default` and `Clone` restrictions
here? I can imagine that a several of people's extensions are not `Clone`
because they are wrapped in an `Arc` here and are not `Default` as they might
contain references to connections or other non-defaultable stuff.
##########
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:
🤔 removing the `Clone` restriction might be complicated actually, as it'd be
hard to get an inner mutable reference given that extensions are hidden behind
an `Arc`...
But maybe we do can remove the `Default` requirement.
--
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]
