chore: revert structured spill config (#19088)

* chore: drop structured spill config

* test: remove structured spill behavior it

* test: fix configs_table_basic formatting

* test: add trailing newlines for configs_table_basic

Author: BohuTANG

src/query/config/src/config.rs

@@ -784,22 +784,6 @@ pub struct FsStorageConfig {
         default_value = "_data"
     )]
     pub data_path: String,
-
-    /// Percentage of disk space to reserve (0.0-100.0) - for spill usage
-    #[clap(skip)]
-    #[serde(default = "FsStorageConfig::default_reserved_space_percentage")]
-    pub reserved_space_percentage: Option<OrderedFloat<f64>>,
-
-    /// Maximum bytes allowed for spill (0 = unlimited) - for spill usage  
-    #[clap(skip)]
-    #[serde(default)]
-    pub max_bytes: Option<u64>,
-}
-
-impl FsStorageConfig {
-    fn default_reserved_space_percentage() -> Option<OrderedFloat<f64>> {
-        None // Use None as default, will use system default (10.0) if not specified
-    }
 }
 
 impl Default for FsStorageConfig {
@@ -812,8 +796,6 @@ impl From<InnerStorageFsConfig> for FsStorageConfig {
     fn from(inner: InnerStorageFsConfig) -> Self {
         Self {
             data_path: inner.root,
-            reserved_space_percentage: None,
-            max_bytes: None,
         }
     }
 }
@@ -3567,20 +3549,6 @@ impl Default for DiskCacheConfig {
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Args)]
 #[serde(default)]
 pub struct SpillConfig {
-    /// Unified spill storage configuration
-    /// Auto-detects type based on storage.type field:
-    /// - "fs": Local filesystem spill
-    /// - "s3": S3 remote spill  
-    /// - "azblob": Azure blob remote spill
-    /// - etc.
-    ///   If not configured, uses main data storage with _spill prefix (default)
-    ///
-    /// Note: Spill operator created based on this config will only use Standard S3 storage class,
-    /// regardless of any other configured storage class
-    #[clap(skip)]
-    pub storage: Option<StorageConfig>,
-
-    // Legacy fields for backward compatibility
     /// Path of spill to local disk. disable if it's empty.
     #[clap(long, value_name = "VALUE", default_value = "")]
     pub spill_local_disk_path: String,
@@ -3593,6 +3561,10 @@ pub struct SpillConfig {
     /// Allow space in bytes to spill to local disk.
     pub spill_local_disk_max_bytes: u64,
 
+    // TODO: We need to fix StorageConfig so that it supports command line injections.
+    #[clap(skip)]
+    pub storage: Option<StorageConfig>,
+
     /// Maximum percentage of the global local spill quota that a single sort
     /// operator may use for one query.
     ///
@@ -3613,49 +3585,9 @@ pub struct SpillConfig {
     pub result_set_spilling_disk_quota_ratio: u64,
 }
 
-impl SpillConfig {
-    /// Get the spill type based on configuration
-    /// Auto-detects from storage configuration
-    pub fn get_spill_type(&self) -> String {
-        // Check new unified storage configuration first (higher priority)
-        if let Some(ref storage) = self.storage {
-            match storage.typ.as_str() {
-                "fs" => "fs".to_string(),
-                "s3" => "s3".to_string(),
-                "azblob" => "azblob".to_string(),
-                "gcs" => "gcs".to_string(),
-                "oss" => "oss".to_string(),
-                "obs" => "obs".to_string(),
-                "cos" => "cos".to_string(),
-                "webhdfs" => "webhdfs".to_string(),
-                _ => "remote".to_string(), // fallback for other storage types
-            }
-        } else if !self.spill_local_disk_path.is_empty() {
-            // Fall back to legacy configuration
-            "fs".to_string() // legacy local disk maps to fs
-        } else {
-            "default".to_string()
-        }
-    }
-
-    /// Check if this is using legacy local disk configuration
-    pub fn is_legacy_local_disk(&self) -> bool {
-        !self.spill_local_disk_path.is_empty()
-    }
-}
-
 impl Default for SpillConfig {
     fn default() -> Self {
-        Self {
-            storage: None,
-            spill_local_disk_path: String::new(),
-            spill_local_disk_reserved_space_percentage: OrderedFloat(10.0),
-            spill_local_disk_max_bytes: u64::MAX,
-            sort_spilling_disk_quota_ratio: 60,
-            window_partition_spilling_disk_quota_ratio: 60,
-            // TODO: keep 0 to avoid deleting local result-set spill dir before HTTP pagination finishes.
-            result_set_spilling_disk_quota_ratio: 0,
-        }
+        inner::SpillConfig::default().into()
     }
 }
 
@@ -3827,91 +3759,19 @@ mod cache_config_converters {
     }
 
     fn convert_local_spill_config(spill: SpillConfig) -> Result<inner::SpillConfig> {
-        // Determine configuration based on auto-detected spill type
-        let spill_type = spill.get_spill_type();
-        let (local_writeable_root, path, reserved_disk_ratio, global_bytes_limit, storage_params) =
-            match spill_type.as_str() {
-                "fs" => {
-                    if let Some(ref storage) = spill.storage {
-                        // Use new filesystem storage configuration (higher priority)
-                        let fs_path = storage.fs.data_path.clone();
-                        let reserved_ratio = storage
-                            .fs
-                            .reserved_space_percentage
-                            .unwrap_or(OrderedFloat(10.0))
-                            / 100.0;
-                        let max_bytes = storage.fs.max_bytes.unwrap_or(u64::MAX);
-
-                        // Validate fs configuration
-                        if fs_path.is_empty() {
-                            return Err(ErrorCode::InvalidConfig(
-                                "FS storage configured but data_path is empty. Either specify a path or remove the fs storage configuration to use default behavior."
-                            ));
-                        } else {
-                            (None, fs_path, reserved_ratio, max_bytes, None)
-                        }
-                    } else if spill.is_legacy_local_disk() {
-                        // Fall back to legacy local disk configuration
-                        (
-                            None,
-                            spill.spill_local_disk_path,
-                            spill.spill_local_disk_reserved_space_percentage / 100.0,
-                            spill.spill_local_disk_max_bytes,
-                            None,
-                        )
-                    } else {
-                        return Err(ErrorCode::InvalidConfig(
-                            "FS storage configuration not found",
-                        ));
-                    }
-                }
-                "s3" | "azblob" | "gcs" | "oss" | "obs" | "cos" | "webhdfs" | "remote" => {
-                    // Use remote storage configuration for all remote storage types
-                    let storage_params = spill
-                        .storage
-                        .map(|storage| {
-                            let storage: InnerStorageConfig = storage.try_into()?;
-                            Ok::<_, ErrorCode>(storage.params)
-                        })
-                        .transpose()?;
-
-                    (
-                        None,
-                        String::new(),
-                        OrderedFloat(0.3),
-                        u64::MAX,
-                        storage_params,
-                    )
-                }
-                _ => {
-                    // Default behavior for "default" type and any unrecognized types:
-                    // do NOT implicitly reuse the data cache disk. Local spill is
-                    // enabled only when explicitly configured via either
-                    //   - [spill.storage] with type = "fs", or
-                    //   - legacy spill_local_disk_path.
-                    let storage_params = spill
-                        .storage
-                        .map(|storage| {
-                            let storage: InnerStorageConfig = storage.try_into()?;
-                            Ok::<_, ErrorCode>(storage.params)
-                        })
-                        .transpose()?;
-
-                    (
-                        None,
-                        spill.spill_local_disk_path,
-                        spill.spill_local_disk_reserved_space_percentage / 100.0,
-                        spill.spill_local_disk_max_bytes,
-                        storage_params,
-                    )
-                }
-            };
+        let storage_params = spill
+            .storage
+            .map(|storage| {
+                let storage: InnerStorageConfig = storage.try_into()?;
+                Ok::<_, ErrorCode>(storage.params)
+            })
+            .transpose()?;
 
         Ok(inner::SpillConfig {
-            local_writeable_root,
-            path,
-            reserved_disk_ratio,
-            global_bytes_limit,
+            local_writeable_root: None,
+            path: spill.spill_local_disk_path,
+            reserved_disk_ratio: spill.spill_local_disk_reserved_space_percentage / 100.0,
+            global_bytes_limit: spill.spill_local_disk_max_bytes,
             storage_params,
             sort_spilling_disk_quota_ratio: spill.sort_spilling_disk_quota_ratio,
             window_partition_spilling_disk_quota_ratio: spill
@@ -3931,10 +3791,10 @@ mod cache_config_converters {
             });
 
             Self {
-                storage,
                 spill_local_disk_path: value.path,
                 spill_local_disk_reserved_space_percentage: value.reserved_disk_ratio * 100.0,
                 spill_local_disk_max_bytes: value.global_bytes_limit,
+                storage,
                 sort_spilling_disk_quota_ratio: value.sort_spilling_disk_quota_ratio,
                 window_partition_spilling_disk_quota_ratio: value
                     .window_partition_spilling_disk_quota_ratio,

src/query/config/src/mask.rs

@@ -211,20 +211,20 @@ impl StorageConfig {
 impl SpillConfig {
     fn mask_display(&self) -> Self {
         let Self {
-            ref storage,
             ref spill_local_disk_path,
             spill_local_disk_reserved_space_percentage,
             spill_local_disk_max_bytes,
+            ref storage,
             sort_spilling_disk_quota_ratio,
             window_partition_spilling_disk_quota_ratio,
             result_set_spilling_disk_quota_ratio,
         } = *self;
 
         Self {
-            storage: storage.as_ref().map(|storage| storage.mask_display()),
             spill_local_disk_path: spill_local_disk_path.clone(),
             spill_local_disk_reserved_space_percentage,
             spill_local_disk_max_bytes,
+            storage: storage.as_ref().map(|storage| storage.mask_display()),
             sort_spilling_disk_quota_ratio,
             window_partition_spilling_disk_quota_ratio,
             // TODO: keep 0 to avoid deleting local result-set spill dir before HTTP pagination finishes.

src/query/config/tests/it/config.rs

@@ -0,0 +1,131 @@
+// Copyright 2023 Datafuse Labs.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::ffi::OsString;
+use std::io::Write;
+
+use clap::Parser;
+use databend_common_config::Config;
+use databend_common_config::InnerConfig;
+use pretty_assertions::assert_eq;
+
+/// It's required to make sure setting's default value is the same with clap.
+#[test]
+fn test_config_default() {
+    let setting_default = InnerConfig::default();
+    let config_default: InnerConfig = Config::parse_from(Vec::<OsString>::new())
+        .try_into()
+        .expect("parse from args must succeed");
+
+    assert_eq!(
+        setting_default, config_default,
+        "default setting is different from default config, please check again"
+    )
+}
+
+#[test]
+fn test_load_config() {
+    // Create a comprehensive test configuration with multiple sections and data types
+    let conf = r#"
+# Query configuration section
+[query]
+max_active_sessions = 256
+shutdown_wait_timeout_ms = 5000
+flight_record_quota_size = 1048576
+unknown_query_field = "should be ignored"
+
+# Log configuration section
+[log]
+file.level = "INFO"
+file.dir = "/tmp/databend/logs"
+unknown_log_field = 123
+
+# Storage configuration section
+[storage]
+type = "fs"
+
+[storage.fs]
+data_path = "/tmp/databend/data"
+
+# Meta configuration section
+[meta]
+endpoint = "localhost:9191"
+username = "databend"
+password = "databend123"
+"#;
+
+    let mut temp_file = tempfile::NamedTempFile::new().unwrap();
+    temp_file.write_all(conf.as_bytes()).unwrap();
+    let temp_path = temp_file.path().to_str().unwrap().to_string();
+
+    // Save the original environment variable (if it exists)
+    let original_env = std::env::var("CONFIG_FILE").ok();
+
+    // Set the environment variable to our test config file
+    std::env::set_var("CONFIG_FILE", &temp_path);
+
+    // Use the original load function (without a config_file parameter)
+    let config = Config::load(false).unwrap();
+
+    // Restore the original environment variable
+    match original_env {
+        Some(val) => std::env::set_var("CONFIG_FILE", val),
+        None => std::env::remove_var("CONFIG_FILE"),
+    }
+
+    // Test query configuration values
+    assert_eq!(
+        config.query.max_active_sessions, 256,
+        "max_active_sessions should be 256"
+    );
+    assert_eq!(
+        config.query.shutdown_wait_timeout_ms, 5000,
+        "shutdown_wait_timeout_ms should be 5000"
+    );
+    assert_eq!(
+        config.query.flight_record_quota_size, 1048576,
+        "flight_record_quota_size should be 1048576"
+    );
+
+    // Test log configuration values
+    assert_eq!(
+        config.log.file.level, "INFO",
+        "log.file.level should be INFO"
+    );
+    assert_eq!(
+        config.log.file.dir,
+        "/tmp/databend/logs",
+        "log.file.dir should be /tmp/databend/logs"
+    );
+
+    // Test storage configuration values
+    assert_eq!(
+        config.storage.fs.data_path, "/tmp/databend/data",
+        "storage.fs.data_path should be /tmp/databend/data"
+    );
+
+    // Test meta configuration values
+    assert_eq!(
+        config.meta.endpoint, "localhost:9191",
+        "meta.endpoint should be localhost:9191"
+    );
+    assert_eq!(
+        config.meta.username, "databend",
+        "meta.username should be databend"
+    );
+    assert_eq!(
+        config.meta.password, "databend123",
+        "meta.password should be databend123"
+    );
+}