Copilot commented on code in PR #21108:
URL: https://github.com/apache/datafusion/pull/21108#discussion_r2973001592
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -88,6 +98,43 @@ impl DataFusion {
self.pb
.set_message(format!("{} - {} took > 500 ms", split[0],
current_count));
}
+
+ pub fn validate_config_unchanged(&mut self) -> Result<()> {
+ let mut changed = false;
+ let mut message = format!(
+ "SLT file {} left modified configuration",
+ self.relative_path.display()
+ );
+
+ for entry in self.ctx.state().config().options().entries() {
+ let default_entry = self.default_config.remove(&entry.key);
+
Review Comment:
`validate_config_unchanged` currently mutates `self.default_config` via
`remove`, which makes the method destructive and forces it to take `&mut self`.
This is harder to reason about and can lead to incorrect behavior if the
validation ever runs more than once for the same `DataFusion` instance (e.g.
multiple shutdown calls). Consider making the comparison non-destructive (use
`get` and track seen keys separately, or clone the defaults into a local map)
so the method can take `&self` and be safely re-invoked.
##########
datafusion/sqllogictest/src/engines/datafusion_engine/runner.rs:
##########
@@ -142,48 +189,12 @@ impl sqllogictest::AsyncDB for DataFusion {
tokio::time::sleep(dur).await;
}
- 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() {
- let default_entry = self.default_config.remove(&e.key);
-
- if let Some(default_entry) = default_entry
- && default_entry.as_ref() != e.value.as_ref()
- {
- if !changed {
- changed = true;
- self.pb.println(format!(
- "SLT file {} left modified configuration",
- self.relative_path.display()
- ));
- }
-
- let default = default_entry.as_deref().unwrap_or("NULL");
- let current = e.value.as_deref().unwrap_or("NULL");
-
- self.pb
- .println(format!(" {}: {} -> {}", e.key, default,
current));
- }
- }
-
- // Any remaining entries were present initially but removed during
execution
- for (key, value) in &self.default_config {
- if !changed {
- changed = true;
- self.pb.println(format!(
- "SLT file {} left modified configuration",
- self.relative_path.display()
- ));
- }
-
- let default = value.as_deref().unwrap_or("NULL");
-
- self.pb.println(format!(" {key}: {default} -> NULL"));
+ /// Shutdown and check no DataFusion configuration has changed during test
+ async fn shutdown(&mut self) {
+ if let Some(config_change_errors) = self.config_change_errors.clone()
+ && let Err(error) = self.validate_config_unchanged()
+ {
+ config_change_errors.lock().unwrap().push(error.to_string());
Review Comment:
The config-drift error is being stored via `error.to_string()`, but
`DFSqlLogicTestError::Other` formats as `"Other Error: ..."`. Since the caller
later wraps these strings into `DataFusionError::External`, the final output
becomes nested/duplicated (e.g. `External error: Other Error: ...`). Consider
pushing just the underlying message (or returning a `DataFusionError` directly)
so the failure output is cleaner and easier to read in CI logs.
```suggestion
let msg = match &error {
DFSqlLogicTestError::Other(inner) => inner.clone(),
_ => error.to_string(),
};
config_change_errors.lock().unwrap().push(msg);
```
--
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]
