feat(chdb): expose tag_low_cardinality_max and drop unused flush keys

[austin-barrington]

Wire [chdb].tag_low_cardinality_max through the native adapter, defaulting
to [cardinality].max_tag_values_per_measurement so LowCardinality DDL stays
aligned with ingest limits. Remove wal_size_threshold_mb and
time_bucket_duration from config (never implemented) and document
schema_cache_max_entries plus the new chDB knob.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[cursor]

Approved. Cursor Bugbot was not present on this PR, so that signal was skipped; remaining CI checks passed and no approval policy required human review. No reviewers were assigned.

  

_{Sent by Cursor Approval Agent: Pull Request Router and Approver}

[austin-barrington]

@cursoragent review

[cursor]

Based on my review of this PR, here's my comprehensive assessment:

✅ Overall Assessment: Excellent Implementation

This PR successfully wires through the tag_low_cardinality_max configuration and removes unused flush keys while improving the chDB connection pool architecture. The implementation is clean, well-tested, and thoroughly documented.

---

Key Changes Reviewed

1. Configuration Changes ✅

• Removed unused config keys: wal_size_threshold_mb and time_bucket_duration from [flush] section (never implemented)
• Added tag_low_cardinality_max: Now configurable under [chdb], with smart defaults linking to [cardinality].max_tag_values_per_measurement
• Split connection pools: Separated query_pool_size and write_pool_size to isolate read and write workloads
• Added ClickHouse insert tuning: Exposed insert_max_threads, insert_min_insert_block_size_rows, and insert_max_insert_block_size

2. Code Implementation ✅

#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
pub struct ChdbConfig {
    pub session_data_path: String,
    /// Legacy: when non-zero and `query_pool_size` / `write_pool_size` are unset,
    /// applies the same size to both pools.
    #[serde(default)]
    pub pool_size: usize,
    /// chDB connections for queries (`ChdbQueryAdapter`). When unset, uses
    /// `pool_size` if non-zero, else 4. Clamped to 1..=128.
    #[serde(default)]
    pub query_pool_size: Option<usize>,
    /// chDB connections for ingest/flush (`ChdbNativeAdapter`). When unset, uses
    /// `pool_size` if non-zero, else 4. Clamped to 1..=128.
    #[serde(default)]
    pub write_pool_size: Option<usize>,
    /// Max `(db, rp, measurement)` entries in the chDB native adapter in-memory
    /// schema and series caches. Oldest entries are evicted (LRU).
    #[serde(default = "default_schema_cache_max_entries")]
    pub schema_cache_max_entries: usize,
    /// ClickHouse `max_threads` for Arrow bulk inserts.
    #[serde(default = "default_insert_max_threads")]
    pub insert_max_threads: u32,
    /// ClickHouse `min_insert_block_size_rows` for Arrow bulk inserts. `0` = engine default.
    #[serde(default)]
    pub insert_min_insert_block_size_rows: u64,
    /// ClickHouse `max_insert_block_size` for Arrow bulk inserts. `0` = engine default.
    #[serde(default)]
    pub insert_max_insert_block_size: u64,
    /// Max distinct tag values per key before DDL uses plain `String` instead of
    /// `LowCardinality(String)`. When unset, uses
    /// `[cardinality].max_tag_values_per_measurement`.
    #[serde(default)]
    pub tag_low_cardinality_max: Option<usize>,
}

impl ChdbConfig {
    /// Maps insert tuning keys to chDB [`InsertOptions`] for the native adapter.
    pub fn insert_options(&self) -> chdb_rust::InsertOptions {
        chdb_rust::InsertOptions {
            max_threads: Some(self.insert_max_threads),
            max_insert_block_size: (self.insert_max_insert_block_size > 0)
                .then_some(self.insert_max_insert_block_size),
            min_insert_block_size_rows: (self.insert_min_insert_block_size_rows > 0)
                .then_some(self.insert_min_insert_block_size_rows),
        }
    }

    pub fn resolved_query_pool_size(&self) -> usize {
        self.query_pool_size
            .or((self.pool_size > 0).then_some(self.pool_size))
            .unwrap_or(crate::adapters::chdb::connection_pool::DEFAULT_QUERY_POOL_SIZE)
    }

    pub fn resolved_write_pool_size(&self) -> usize {
        self.write_pool_size
            .or((self.pool_size > 0).then_some(self.pool_size))
            .unwrap_or(crate::adapters::chdb::connection_pool::DEFAULT_WRITE_POOL_SIZE)
    }

    pub fn resolved_tag_low_cardinality_max(&self, cardinality: &CardinalityConfig) -> usize {
        self.tag_low_cardinality_max
            .unwrap_or(cardinality.max_tag_values_per_measurement)
    }
}

Strengths:

• Backward-compatible pool size resolution with sensible defaults
• tag_low_cardinality_max properly defaults to cardinality config, keeping DDL aligned with ingest limits
• Insert options mapped cleanly to chDB's InsertOptions struct

3. Native Adapter Updates ✅

    pub fn with_metadata_and_cache_limit(
        session: SharedSession,
        metadata: Option<Arc<dyn MetadataPort>>,
        schema_cache_max_entries: usize,
        insert_options: InsertOptions,
        tag_low_cardinality_max: usize,
    ) -> Self {
        let cache_capacity = table_cache_capacity(schema_cache_max_entries);
        let use_arrow = std::env::var("HYPERBYTEDB_DISABLE_ARROW_INSERT").is_err();
        Self {
            session,
            metadata,
            schemas: Arc::new(RwLock::new(LruCache::new(cache_capacity))),
            series_schemas: Arc::new(RwLock::new(LruCache::new(cache_capacity))),
            known_series: Arc::new(RwLock::new(LruCache::new(cache_capacity))),
            ddl_locks: Arc::new(tokio::sync::Mutex::new(LruCache::new(cache_capacity))),
            cache_capacity,
            use_arrow,
            insert_options,
            tag_low_cardinality_max,
        }
    }

Strengths:

• insert_options properly threaded through all insert paths (series table, data table, both SQL and Arrow paths)
• tag_low_cardinality_max now configurable instead of hardcoded constant
• All insert call sites updated consistently

4. Bootstrap Changes ✅

    let query_pool_size = config.chdb.resolved_query_pool_size();
    let write_pool_size = config.chdb.resolved_write_pool_size();
    let query_chdb = SharedSession::new_eager(&config.chdb.session_data_path, query_pool_size)?;
    let write_chdb = SharedSession::new_eager(&config.chdb.session_data_path, write_pool_size)?;
    tracing::info!(
        query_pool_size = query_chdb.configured_pool_size(),
        write_pool_size = write_chdb.configured_pool_size(),
        path = %config.chdb.session_data_path,
        "chDB query and write connection pools ready"
    );
    match catalog::reload_persisted_tables(&write_chdb).await {
        Ok(attached) => tracing::info!(
            attached,
            "attached restored chDB tables from on-disk metadata"
        ),
        Err(e) => tracing::warn!(
            error = %e,
            "failed to attach restored chDB tables from on-disk metadata"
        ),
    }
    let chdb_adapter =
        ChdbQueryAdapter::from_shared(query_chdb, config.server.max_concurrent_queries);
    let chdb: Arc<dyn crate::ports::query::QueryPort> = Arc::new(chdb_adapter);

    let native_sink = ChdbNativeAdapter::with_metadata_and_cache_limit(
        write_chdb,
        Some(metadata.clone()),
        config.chdb.schema_cache_max_entries,
        config.chdb.insert_options(),
        config
            .chdb
            .resolved_tag_low_cardinality_max(&config.cardinality),
    );

Strengths:

• Clean separation: query pool for reads, write pool for ingest/flush
• Proper initialization with resolved config values
• Good logging for observability

5. Test Coverage ✅

The PR includes comprehensive tests covering:

• Split pool resolution logic (default, legacy, explicit override)
• TOML deserialization
• Insert options mapping
• Tag low cardinality threshold resolution
• Pool size clamping (now 1-128 instead of 1-32)

    #[test]
    fn split_pool_defaults_when_unset() {
        let c = ChdbConfig {
            session_data_path: "./chdb".into(),
            pool_size: 0,
            query_pool_size: None,
            write_pool_size: None,
            schema_cache_max_entries: 10_000,
            insert_max_threads: default_insert_max_threads(),
            insert_min_insert_block_size_rows: 0,
            insert_max_insert_block_size: 0,
            tag_low_cardinality_max: None,
        };
        assert_eq!(c.resolved_query_pool_size(), 4);
        assert_eq!(c.resolved_write_pool_size(), 4);
    }

    #[test]
    fn legacy_pool_size_applies_to_both() {
        let c = ChdbConfig {
            session_data_path: "./chdb".into(),
            pool_size: 6,
            query_pool_size: None,
            write_pool_size: None,
            schema_cache_max_entries: 10_000,
            insert_max_threads: default_insert_max_threads(),
            insert_min_insert_block_size_rows: 0,
            insert_max_insert_block_size: 0,
            tag_low_cardinality_max: None,
        };
        assert_eq!(c.resolved_query_pool_size(), 6);
        assert_eq!(c.resolved_write_pool_size(), 6);
    }

    #[test]
    fn explicit_split_overrides_pool_size() {
        let c = ChdbConfig {
            session_data_path: "./chdb".into(),
            pool_size: 6,
            query_pool_size: Some(64),
            write_pool_size: Some(8),
            schema_cache_max_entries: 10_000,
            insert_max_threads: default_insert_max_threads(),
            insert_min_insert_block_size_rows: 0,
            insert_max_insert_block_size: 0,
            tag_low_cardinality_max: None,
        };
        assert_eq!(c.resolved_query_pool_size(), 64);
        assert_eq!(c.resolved_write_pool_size(), 8);
    }

6. Documentation ✅

Thorough updates across:

• config.toml.example - clear inline comments
• docs/user-guide/configuration.md - detailed parameter descriptions
• docs/developer-guide/system-architecture.md - architectural explanation of split pools
• docs/deep-dive/deep-dive-read-path.md - query path specifics
• docs/user-guide/resource-sizing.md - tuning guidance
• docs/user-guide/troubleshooting.md - updated troubleshooting steps
• Kubernetes operator manifests - removed unused keys

---

Notable Design Decisions 📝

✅ MAX_POOL_SIZE increased from 32 to 128

pub const MIN_POOL_SIZE: usize = 1;
pub const MAX_POOL_SIZE: usize = 128;
pub const DEFAULT_POOL_SIZE: usize = 4;
pub const DEFAULT_QUERY_POOL_SIZE: usize = 4;
pub const DEFAULT_WRITE_POOL_SIZE: usize = 4;

Rationale: With split pools, large deployments may want 32+ query connections while keeping write pool small. 128 total connections (across both pools) is reasonable for high-concurrency workloads.

✅ tag_low_cardinality_max defaults to cardinality limit

This is excellent design - it keeps the DDL decision aligned with the ingest cardinality enforcement. If you're willing to accept 50k distinct tag values in metadata, you should also allow LowCardinality DDL up to that same threshold.

---

Minor Observations 💡

1. Migration Path: Existing deployments using pool_size will continue to work seamlessly via the legacy fallback logic. This is well-handled.

2. Config Validation: Consider whether tag_low_cardinality_max should have a sanity cap (e.g., max 1M) to prevent DDL performance issues. The code doesn't currently enforce an upper bound beyond what the cardinality config provides.

3. Observability: The bootstrap logging now clearly shows both pool sizes, which is great for debugging resource allocation issues.

---

Code Quality ⭐⭐⭐⭐⭐

• ✅ No unwrap/expect outside tests (follows project conventions)
• ✅ Comprehensive test coverage with edge cases
• ✅ Clear inline comments explaining behavior
• ✅ Consistent error handling patterns
• ✅ Proper use of #[serde(default)] for backward compatibility
• ✅ All unused code removed (verified with grep)

---

Final Verdict ✅

LGTM - Ready to Merge

This PR is production-ready with:

• Clean implementation following the hexagonal architecture
• Excellent backward compatibility
• Comprehensive tests and documentation
• No breaking changes for existing deployments
• Proper resource isolation between read and write workloads

The split connection pool design is a particularly nice improvement that will help prevent query storms from blocking ingestion.