feat(sqllogictest): use serde derived structs for schedule parsing

Replace manual TOML parsing with serde-derived structs to improve
maintainability and separate parsing from engine instantiation.

Changes:
- Add ScheduleConfig, EngineConfig, and EngineType structs with serde derives
- Use #[serde(flatten)] for forward-compatibility with future config fields
- Refactor Schedule::from_file() to use toml::from_str() directly
- Add instantiate_engines() to separate parsing from engine creation
- Remove manual parse_engines() and parse_steps() functions
- Update tests to verify deserialization behavior

Closes #1952

Author: AndreaBozzo

crates/sqllogictest/src/engine/datafusion.rs

@@ -27,9 +27,8 @@ use iceberg::spec::{NestedField, PrimitiveType, Schema, Transform, Type, Unbound
 use iceberg::{Catalog, CatalogBuilder, NamespaceIdent, TableCreation};
 use iceberg_datafusion::IcebergCatalogProvider;
 use indicatif::ProgressBar;
-use toml::Table as TomlTable;
 
-use crate::engine::{EngineRunner, run_slt_with_runner};
+use crate::engine::{EngineConfig, EngineRunner, run_slt_with_runner};
 use crate::error::Result;
 
 pub struct DataFusionEngine {
@@ -59,20 +58,22 @@ impl EngineRunner for DataFusionEngine {
 }
 
 impl DataFusionEngine {
-    pub async fn new(config: TomlTable) -> Result<Self> {
+    pub async fn new(_name: &str, config: &EngineConfig) -> Result<Self> {
         let session_config = SessionConfig::new()
             .with_target_partitions(4)
             .with_information_schema(true);
         let ctx = SessionContext::new_with_config(session_config);
-        ctx.register_catalog("default", Self::create_catalog(&config).await?);
+        ctx.register_catalog("default", Self::create_catalog(&config.extra).await?);
 
         Ok(Self {
             test_data_path: PathBuf::from("testdata"),
             session_context: ctx,
         })
     }
 
-    async fn create_catalog(_: &TomlTable) -> anyhow::Result<Arc<dyn CatalogProvider>> {
+    async fn create_catalog(
+        _extra: &HashMap<String, toml::Value>,
+    ) -> anyhow::Result<Arc<dyn CatalogProvider>> {
         // TODO: support dynamic catalog configuration
         //  See: https://github.com/apache/iceberg-rust/issues/1780
         let catalog = MemoryCatalogBuilder::default()

crates/sqllogictest/src/engine/mod.rs

@@ -17,29 +17,44 @@
 
 mod datafusion;
 
+use std::collections::HashMap;
 use std::path::Path;
 
 use anyhow::anyhow;
+use serde::Deserialize;
 use sqllogictest::{AsyncDB, MakeConnection, Runner, parse_file};
-use toml::Table as TomlTable;
 
 use crate::engine::datafusion::DataFusionEngine;
 use crate::error::{Error, Result};
 
-const TYPE_DATAFUSION: &str = "datafusion";
+/// Supported engine types for sqllogictest
+#[derive(Debug, Clone, Deserialize, PartialEq, Eq)]
+#[serde(rename_all = "lowercase")]
+pub enum EngineType {
+    Datafusion,
+}
+
+/// Configuration for a single engine instance
+#[derive(Debug, Clone, Deserialize)]
+pub struct EngineConfig {
+    /// The type of engine
+    #[serde(rename = "type")]
+    pub engine_type: EngineType,
+
+    /// Additional configuration fields for extensibility
+    /// This allows forward-compatibility with future fields like catalog_type, catalog_properties
+    #[serde(flatten)]
+    pub extra: HashMap<String, toml::Value>,
+}
 
 #[async_trait::async_trait]
 pub trait EngineRunner: Send {
     async fn run_slt_file(&mut self, path: &Path) -> Result<()>;
 }
 
-pub async fn load_engine_runner(
-    engine_type: &str,
-    cfg: TomlTable,
-) -> Result<Box<dyn EngineRunner>> {
-    match engine_type {
-        TYPE_DATAFUSION => Ok(Box::new(DataFusionEngine::new(cfg).await?)),
-        _ => Err(anyhow::anyhow!("Unsupported engine type: {engine_type}").into()),
+pub async fn load_engine_runner(name: &str, config: EngineConfig) -> Result<Box<dyn EngineRunner>> {
+    match config.engine_type {
+        EngineType::Datafusion => Ok(Box::new(DataFusionEngine::new(name, &config).await?)),
     }
 }
 
@@ -65,29 +80,45 @@ where
 
 #[cfg(test)]
 mod tests {
-    use crate::engine::{TYPE_DATAFUSION, load_engine_runner};
+    use std::collections::HashMap;
 
-    #[tokio::test]
-    async fn test_engine_invalid_type() {
+    use crate::engine::{EngineConfig, EngineType, load_engine_runner};
+
+    #[test]
+    fn test_deserialize_engine_config() {
         let input = r#"
-            [engines]
-            random = { type = "random_engine", url = "http://localhost:8181" }
+            type = "datafusion"
         "#;
-        let tbl = toml::from_str(input).unwrap();
-        let result = load_engine_runner("random_engine", tbl).await;
 
-        assert!(result.is_err());
+        let config: EngineConfig = toml::from_str(input).unwrap();
+        assert_eq!(config.engine_type, EngineType::Datafusion);
+        assert!(config.extra.is_empty());
     }
 
-    #[tokio::test]
-    async fn test_load_datafusion() {
+    #[test]
+    fn test_deserialize_engine_config_with_extras() {
         let input = r#"
-            [engines]
-            df = { type = "datafusion" }
+            type = "datafusion"
+            catalog_type = "rest"
+
+            [catalog_properties]
+            uri = "http://localhost:8181"
         "#;
-        let tbl = toml::from_str(input).unwrap();
-        let result = load_engine_runner(TYPE_DATAFUSION, tbl).await;
 
+        let config: EngineConfig = toml::from_str(input).unwrap();
+        assert_eq!(config.engine_type, EngineType::Datafusion);
+        assert!(config.extra.contains_key("catalog_type"));
+        assert!(config.extra.contains_key("catalog_properties"));
+    }
+
+    #[tokio::test]
+    async fn test_load_datafusion() {
+        let config = EngineConfig {
+            engine_type: EngineType::Datafusion,
+            extra: HashMap::new(),
+        };
+
+        let result = load_engine_runner("df", config).await;
         assert!(result.is_ok());
     }
 }

crates/sqllogictest/src/schedule.rs

@@ -21,10 +21,18 @@ use std::path::{Path, PathBuf};
 
 use anyhow::{Context, anyhow};
 use serde::{Deserialize, Serialize};
-use toml::{Table as TomlTable, Value};
 use tracing::info;
 
-use crate::engine::{EngineRunner, load_engine_runner};
+use crate::engine::{EngineConfig, EngineRunner, load_engine_runner};
+
+/// Raw configuration parsed from the schedule TOML file
+#[derive(Debug, Clone, Deserialize)]
+pub struct ScheduleConfig {
+    /// Engine name to engine configuration
+    pub engines: HashMap<String, EngineConfig>,
+    /// List of test steps to run
+    pub steps: Vec<Step>,
+}
 
 pub struct Schedule {
     /// Engine names to engine instances
@@ -59,15 +67,27 @@ impl Schedule {
     pub async fn from_file<P: AsRef<Path>>(path: P) -> anyhow::Result<Self> {
         let path_str = path.as_ref().to_string_lossy().to_string();
         let content = read_to_string(path)?;
-        let toml_value = content.parse::<Value>()?;
-        let toml_table = toml_value
-            .as_table()
-            .ok_or_else(|| anyhow!("Schedule file must be a TOML table"))?;
 
-        let engines = Schedule::parse_engines(toml_table).await?;
-        let steps = Schedule::parse_steps(toml_table)?;
+        let config: ScheduleConfig = toml::from_str(&content)
+            .with_context(|| format!("Failed to parse schedule file: {path_str}"))?;
+
+        let engines = Self::instantiate_engines(config.engines).await?;
+
+        Ok(Self::new(engines, config.steps, path_str))
+    }
+
+    /// Instantiate engine runners from their configurations
+    async fn instantiate_engines(
+        configs: HashMap<String, EngineConfig>,
+    ) -> anyhow::Result<HashMap<String, Box<dyn EngineRunner>>> {
+        let mut engines = HashMap::new();
+
+        for (name, config) in configs {
+            let engine = load_engine_runner(&name, config).await?;
+            engines.insert(name, engine);
+        }
 
-        Ok(Self::new(engines, steps, path_str))
+        Ok(engines)
     }
 
     pub async fn run(mut self) -> anyhow::Result<()> {
@@ -105,103 +125,116 @@ impl Schedule {
         }
         Ok(())
     }
+}
 
-    async fn parse_engines(
-        table: &TomlTable,
-    ) -> anyhow::Result<HashMap<String, Box<dyn EngineRunner>>> {
-        let engines_tbl = table
-            .get("engines")
-            .with_context(|| "Schedule file must have an 'engines' table")?
-            .as_table()
-            .ok_or_else(|| anyhow!("'engines' must be a table"))?;
-
-        let mut engines = HashMap::new();
-
-        for (name, engine_val) in engines_tbl {
-            let cfg_tbl = engine_val
-                .as_table()
-                .ok_or_else(|| anyhow!("Config of engine '{name}' is not a table"))?
-                .clone();
-
-            let engine_type = cfg_tbl
-                .get("type")
-                .ok_or_else(|| anyhow::anyhow!("Engine {name} doesn't have a 'type' field"))?
-                .as_str()
-                .ok_or_else(|| anyhow::anyhow!("Engine {name} type must be a string"))?;
-
-            let engine = load_engine_runner(engine_type, cfg_tbl.clone()).await?;
-
-            if engines.insert(name.clone(), engine).is_some() {
-                return Err(anyhow!("Duplicate engine '{name}'"));
-            }
-        }
+#[cfg(test)]
+mod tests {
+    use crate::engine::EngineType;
+    use crate::schedule::ScheduleConfig;
 
-        Ok(engines)
-    }
+    #[test]
+    fn test_deserialize_schedule_config() {
+        let input = r#"
+            [engines]
+            df = { type = "datafusion" }
 
-    fn parse_steps(table: &TomlTable) -> anyhow::Result<Vec<Step>> {
-        let steps_val = table
-            .get("steps")
-            .with_context(|| "Schedule file must have a 'steps' array")?;
+            [[steps]]
+            engine = "df"
+            slt = "test.slt"
+        "#;
 
-        let steps: Vec<Step> = steps_val
-            .clone()
-            .try_into()
-            .with_context(|| "Failed to deserialize steps")?;
+        let config: ScheduleConfig = toml::from_str(input).unwrap();
 
-        Ok(steps)
+        assert_eq!(config.engines.len(), 1);
+        assert!(config.engines.contains_key("df"));
+        assert_eq!(config.engines["df"].engine_type, EngineType::Datafusion);
+        assert_eq!(config.steps.len(), 1);
+        assert_eq!(config.steps[0].engine, "df");
+        assert_eq!(config.steps[0].slt, "test.slt");
     }
-}
-
-#[cfg(test)]
-mod tests {
-    use toml::Table as TomlTable;
-
-    use crate::schedule::Schedule;
 
     #[test]
-    fn test_parse_steps() {
+    fn test_deserialize_multiple_steps() {
         let input = r#"
+            [engines]
+            datafusion = { type = "datafusion" }
+
             [[steps]]
             engine = "datafusion"
             slt = "test.slt"
 
             [[steps]]
-            engine = "spark"
+            engine = "datafusion"
             slt = "test2.slt"
         "#;
 
-        let tbl: TomlTable = toml::from_str(input).unwrap();
-        let steps = Schedule::parse_steps(&tbl).unwrap();
+        let config: ScheduleConfig = toml::from_str(input).unwrap();
 
-        assert_eq!(steps.len(), 2);
-        assert_eq!(steps[0].engine, "datafusion");
-        assert_eq!(steps[0].slt, "test.slt");
-        assert_eq!(steps[1].engine, "spark");
-        assert_eq!(steps[1].slt, "test2.slt");
+        assert_eq!(config.steps.len(), 2);
+        assert_eq!(config.steps[0].engine, "datafusion");
+        assert_eq!(config.steps[0].slt, "test.slt");
+        assert_eq!(config.steps[1].engine, "datafusion");
+        assert_eq!(config.steps[1].slt, "test2.slt");
     }
 
     #[test]
-    fn test_parse_steps_empty() {
+    fn test_deserialize_with_extra_fields() {
+        // Test forward-compatibility with extra fields (for PR #1943)
         let input = r#"
+            [engines]
+            df = { type = "datafusion", catalog_type = "rest", some_future_field = "value" }
+
             [[steps]]
+            engine = "df"
+            slt = "test.slt"
         "#;
 
-        let tbl: TomlTable = toml::from_str(input).unwrap();
-        let steps = Schedule::parse_steps(&tbl);
+        let config: ScheduleConfig = toml::from_str(input).unwrap();
 
-        assert!(steps.is_err());
+        assert!(config.engines["df"].extra.contains_key("catalog_type"));
+        assert!(config.engines["df"].extra.contains_key("some_future_field"));
     }
 
-    #[tokio::test]
-    async fn test_parse_engines_invalid_table() {
-        let toml_content = r#"
-            engines = "not_a_table"
+    #[test]
+    fn test_deserialize_missing_engine_type() {
+        let input = r#"
+            [engines]
+            df = { }
+
+            [[steps]]
+            engine = "df"
+            slt = "test.slt"
         "#;
 
-        let table: TomlTable = toml::from_str(toml_content).unwrap();
-        let result = Schedule::parse_engines(&table).await;
+        let result: Result<ScheduleConfig, _> = toml::from_str(input);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_deserialize_invalid_engine_type() {
+        let input = r#"
+            [engines]
+            df = { type = "unknown_engine" }
+
+            [[steps]]
+            engine = "df"
+            slt = "test.slt"
+        "#;
+
+        let result: Result<ScheduleConfig, _> = toml::from_str(input);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_deserialize_missing_step_fields() {
+        let input = r#"
+            [engines]
+            df = { type = "datafusion" }
+
+            [[steps]]
+        "#;
 
+        let result: Result<ScheduleConfig, _> = toml::from_str(input);
         assert!(result.is_err());
     }
 }