liurenjie1024 commented on code in PR #1953:
URL: https://github.com/apache/iceberg-rust/pull/1953#discussion_r2641653056
##########
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>,
Review Comment:
+1, see the suggestion above.
##########
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 {
Review Comment:
```suggestion
pub enum EngineConfig {
DataFusion {
catalog: DatafusionCatalogConfig
}
}
pub struct DatafusionCatalogConfig {
typ: String,
properties: HashMap<String, String>
}
```
##########
crates/sqllogictest/src/schedule.rs:
##########
@@ -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" }
Review Comment:
```suggestion
df = { type = "datafusion",
catalog = {
type = "rest",
props = {
// A hashmap of properties.
url = "http://localhost:9000";,
use_x = "true",
}
}
}
```
This is a complete example, and then we could use catalog loader to load a
catalog.
--
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]
