martin-g commented on code in PR #20838:
URL: https://github.com/apache/datafusion/pull/20838#discussion_r2911210124
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -38,29 +39,38 @@ pub struct DataFusion {
relative_path: PathBuf,
pb: ProgressBar,
currently_executing_sql_tracker: CurrentlyExecutingSqlTracker,
+ default_config: HashMap<String, Option<String>>,
}
impl DataFusion {
pub fn new(ctx: SessionContext, relative_path: PathBuf, pb: ProgressBar)
-> Self {
+ let default_config = SessionContext::new()
Review Comment:
Why do you create a fresh SessionContext to collect the initial state ?
IMO you should use the passed `ctx`
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -135,6 +145,34 @@ impl sqllogictest::AsyncDB for DataFusion {
async fn shutdown(&mut self) {}
}
+impl Drop for DataFusion {
+ fn drop(&mut self) {
+ let mut changed = false;
+
+ for e in self.ctx.state().config().options().entries() {
+ if self.default_config.get(&e.key) != Some(&e.value) {
+ if !changed {
+ changed = true;
+ self.pb.println(format!(
+ "SLT file {} left modified configuration",
+ self.relative_path.display()
+ ));
+ }
+
+ let old = self
Review Comment:
```suggestion
let default = self
```
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -38,29 +39,38 @@ pub struct DataFusion {
relative_path: PathBuf,
pb: ProgressBar,
currently_executing_sql_tracker: CurrentlyExecutingSqlTracker,
+ default_config: HashMap<String, Option<String>>,
}
impl DataFusion {
pub fn new(ctx: SessionContext, relative_path: PathBuf, pb: ProgressBar)
-> Self {
+ let default_config = SessionContext::new()
+ .state()
+ .config()
+ .options()
+ .entries()
+ .iter()
+ .map(|e| (e.key.clone(), e.value.clone()))
+ .collect();
+
Self {
ctx,
relative_path,
pb,
currently_executing_sql_tracker:
CurrentlyExecutingSqlTracker::default(),
+ default_config,
}
}
/// Add a tracker that will track the currently executed SQL statement.
///
/// This is useful for logging and debugging purposes.
pub fn with_currently_executing_sql_tracker(
- self,
+ mut self,
Review Comment:
nit: This change looks unrelated to the PR goal.
Since the build passes either the callers already pass a mutable instance or
it is not used at all.
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -135,6 +145,34 @@ impl sqllogictest::AsyncDB for DataFusion {
async fn shutdown(&mut self) {}
}
+impl Drop for DataFusion {
+ fn drop(&mut self) {
+ let mut changed = false;
+
+ for e in self.ctx.state().config().options().entries() {
+ if self.default_config.get(&e.key) != Some(&e.value) {
Review Comment:
IMO it would be good to use `remove()` here instead of `get()`. This way at
the end you could check whether any old keys are still in the map. I.e. there
was an entry in the beginning but it is removed during the execution.
I doubt anyone removes config settings in .slt but it does not harm to check
it.
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -135,6 +145,34 @@ impl sqllogictest::AsyncDB for DataFusion {
async fn shutdown(&mut self) {}
}
+impl Drop for DataFusion {
+ fn drop(&mut self) {
+ let mut changed = false;
+
+ for e in self.ctx.state().config().options().entries() {
+ if self.default_config.get(&e.key) != Some(&e.value) {
+ if !changed {
+ changed = true;
+ self.pb.println(format!(
+ "SLT file {} left modified configuration",
+ self.relative_path.display()
+ ));
+ }
+
+ let old = self
+ .default_config
+ .get(&e.key)
+ .and_then(|v| v.as_deref())
+ .unwrap_or("NULL");
+
+ let new = e.value.as_deref().unwrap_or("NULL");
Review Comment:
```suggestion
let current = e.value.as_deref().unwrap_or("NULL");
```
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -135,6 +145,34 @@ impl sqllogictest::AsyncDB for DataFusion {
async fn shutdown(&mut self) {}
}
+impl Drop for DataFusion {
+ fn drop(&mut self) {
+ let mut changed = false;
+
+ for e in self.ctx.state().config().options().entries() {
+ if self.default_config.get(&e.key) != Some(&e.value) {
+ if !changed {
+ changed = true;
+ self.pb.println(format!(
+ "SLT file {} left modified configuration",
+ self.relative_path.display()
+ ));
+ }
+
+ let old = self
+ .default_config
+ .get(&e.key)
+ .and_then(|v| v.as_deref())
+ .unwrap_or("NULL");
+
+ let new = e.value.as_deref().unwrap_or("NULL");
+
+ self.pb.println(format!(" {}: {} -> {}", e.key, old, new));
Review Comment:
```suggestion
self.pb.println(format!(" {}: {} -> {}", e.key, default,
current));
```
--
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]
