Skip to content

Commit ad2e81a

Browse files
abbccddaclaude
andcommitted
fix(clickhouse): address PR #157 review findings
- Catalog mode is now best-effort (mirrors DynamoDB): a table whose schema fetch fails (broken view, permissions gap) is skipped with a warning instead of failing server startup. Stream-like engine tables (Kafka/RabbitMQ/NATS/FileLog) and materialized-view inner tables (.inner* names) are filtered out up front. - Introspection is one batched system.tables query instead of one query per database, so many databases no longer blow the single 5s retry window. Adds a direct dep on the same `clickhouse` client crate the upstream provider already uses. - URL-embedded credentials are a hard config error instead of a warning: the pool ignores them (auth would fail confusingly late) and the connection string is logged and exposed by the data-sources API. Connection strings are validated before they are logged. - `database` joins `table`/`schema` in the catalog-mode conflicting options list; added ClickHouse config-validation tests (read_write rejection, database-in-catalog rejection). - count(*) empty-projection scans stream the narrowest fixed-width column instead of column 0; the no-aggregate-pushdown limitation is documented in the README. - Per-table schema inference is wrapped in retry_with_timeout so a hanging endpoint can't stall startup. - Docs: schema inference is a SELECT ... LIMIT 0 probe, not DESCRIBE TABLE; federated_stock_value rounds stock_value to match the README. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 26e43d4 commit ad2e81a

6 files changed

Lines changed: 285 additions & 64 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ arrow-json = { version = "57.0.1" }
2626
async-trait = "0.1.88"
2727
axum = "0.7"
2828
cookie = "0.18"
29+
clickhouse = "0.14"
2930
ctor = "0.4.1"
3031
datafusion = { version = "52.1.0", default-features = false, features = [
3132
"nested_expressions",

crates/server/src/config.rs

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -761,10 +761,11 @@ fn validate_data_sources(data_sources: &[DataSource]) -> Result<()> {
761761
}
762762

763763
// Catalog mode must not mix with per-table / per-schema options
764+
// ("database" is ClickHouse's schema-analog spelling)
764765
if CATALOG_SUPPORTED_SOURCES.contains(&source.source_type)
765766
&& source.hierarchy_level == HierarchyLevel::Catalog
766767
{
767-
for conflicting in &["table", "schema"] {
768+
for conflicting in &["table", "schema", "database"] {
768769
if source
769770
.options
770771
.as_ref()
@@ -2374,6 +2375,72 @@ options:
23742375
);
23752376
}
23762377

2378+
fn clickhouse_source(
2379+
name: &str,
2380+
options: Option<HashMap<String, String>>,
2381+
access_mode: AccessMode,
2382+
) -> DataSource {
2383+
DataSource {
2384+
name: name.to_string(),
2385+
source_type: DataSourceType::Clickhouse,
2386+
path: PathBuf::new(),
2387+
connection_string: Some("http://localhost:8123".to_string()),
2388+
schema: None,
2389+
options,
2390+
hierarchy_level: HierarchyLevel::default(),
2391+
access_mode,
2392+
enable_cache: false,
2393+
description: None,
2394+
}
2395+
}
2396+
2397+
#[test]
2398+
fn validate_rejects_clickhouse_read_write() {
2399+
let mut options = HashMap::new();
2400+
options.insert("table".to_string(), "events".to_string());
2401+
let source = clickhouse_source("events", Some(options), AccessMode::ReadWrite);
2402+
let err = validate_data_sources(&[source]).unwrap_err();
2403+
let config_err = err.downcast_ref::<ConfigError>().unwrap();
2404+
assert!(
2405+
matches!(
2406+
config_err,
2407+
ConfigError::UnsupportedWriteMode { name, source_type }
2408+
if name == "events" && *source_type == DataSourceType::Clickhouse
2409+
),
2410+
"got {config_err}"
2411+
);
2412+
}
2413+
2414+
#[test]
2415+
fn validate_accepts_clickhouse_table_mode_with_database_option() {
2416+
let mut options = HashMap::new();
2417+
options.insert("table".to_string(), "events".to_string());
2418+
options.insert("database".to_string(), "analytics".to_string());
2419+
let source = clickhouse_source("events", Some(options), AccessMode::ReadOnly);
2420+
validate_data_sources(&[source]).expect("table mode accepts a database option");
2421+
}
2422+
2423+
#[test]
2424+
fn validate_rejects_clickhouse_catalog_database_option() {
2425+
// "database" is ClickHouse's schema-analog option; letting it through
2426+
// in catalog mode would silently change the pool's default database.
2427+
let mut options = HashMap::new();
2428+
options.insert("database".to_string(), "analytics".to_string());
2429+
let mut source = clickhouse_source("ch", Some(options), AccessMode::ReadOnly);
2430+
source.hierarchy_level = HierarchyLevel::Catalog;
2431+
2432+
let err = validate_data_sources(&[source]).unwrap_err();
2433+
let config_err = err.downcast_ref::<ConfigError>().unwrap();
2434+
assert!(
2435+
matches!(
2436+
config_err,
2437+
ConfigError::CatalogModeConflictingOptions { name, option }
2438+
if name == "ch" && option == "database"
2439+
),
2440+
"got {config_err}"
2441+
);
2442+
}
2443+
23772444
#[test]
23782445
fn unsupported_write_mode_error_lists_dynamodb() {
23792446
let err = ConfigError::UnsupportedWriteMode {

crates/skardi/Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ text-splitter = { version = "0.30", features = ["markdown"], optional = true }
5959
# sources
6060
aws-config = { version = "1", features = ["behavior-version-latest"] }
6161
aws-sdk-dynamodb = "1"
62+
# Direct dep on the same client `datafusion-table-providers` uses, so catalog
63+
# introspection can run one batched `system.tables` query instead of N+1.
64+
clickhouse = { workspace = true }
6265
datafusion-federation = { workspace = true }
6366
datafusion-table-providers = { workspace = true }
6467
derivative = "2.2.0"

0 commit comments

Comments
 (0)