Skip to content

fix(actions): use type-appropriate Column Profiler query for Array/Map/Tuple columns #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ZhuchkaTriplesix merged 1 commit into from
Sep 29, 2026
Merged

fix(actions): use type-appropriate Column Profiler query for Array/Map/Tuple columns #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 76 additions & 2 deletions src/rpc/handlers/schema.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ pub async fn handle_context_actions(params: Option<Value>) -> Result<Value, Driv
})?;

// Verify connection exists in pool
let _client = ConnectionPool::global()
let client = ConnectionPool::global()
.get(p.connection_id)
.ok_or_else(|| DriverError::ConnectionNotFound(p.connection_id))?;

Expand All @@ -255,7 +255,46 @@ pub async fn handle_context_actions(params: Option<Value>) -> Result<Value, Driv
p.connection_id, p.node_type, p.node_id
);

let actions = crate::sdui::actions::get_context_actions_for_node(&p.node_type, &p.node_id)?;
// For a column node, look up its ClickHouse type so the Column Profiler
// (`column.stats`/`column.top_10`) can pick a query that's valid for that
// type instead of unconditionally emitting min()/max()/topK() and GROUP BY,
// which ClickHouse rejects for Array/Map/Tuple columns (issue #60).
let column_type = if p.node_type == "column" {
let parts = split_node_id(&p.node_id);
if parts.len() >= 4 {
let is_mock =
client.base_url.starts_with("mock://") || client.base_url.starts_with("test://");
if is_mock {
None
} else {
let db_name = &parts[1];
let table_name = &parts[2];
let col_name = &parts[3];
let sql = format!(
"SELECT type FROM system.columns WHERE database = '{}' AND table = '{}' AND name = '{}' LIMIT 1 FORMAT JSONCompactEachRowWithNamesAndTypes",
crate::utils::sql_escape::escape_sql_string_literal(db_name),
crate::utils::sql_escape::escape_sql_string_literal(table_name),
crate::utils::sql_escape::escape_sql_string_literal(col_name)
);
let text = run_introspection_query(p.connection_id, &sql).await?;
crate::mapper::row_compact::parse_compact_output(&text, 0)
.ok()
.and_then(|result| result.rows.into_iter().next())
.and_then(|row| row.into_iter().next())
.and_then(|v| v.as_str().map(|s| s.to_string()))
}
} else {
None
}
} else {
None
};

let actions = crate::sdui::actions::get_context_actions_for_node(
&p.node_type,
&p.node_id,
column_type.as_deref(),
)?;
Ok(json!({ "actions": actions }))
}

Expand Down Expand Up @@ -637,6 +676,41 @@ mod tests {
ConnectionPool::global().remove(403);
}

#[tokio::test]
async fn test_handle_context_actions_for_column_on_mock_connection() {
// A mock connection has no real system.columns to look up, so the type
// lookup is skipped and column_type stays None, falling back to the
// scalar-oriented profiling query (issue #60).
let _guard = crate::utils::test_lock::GLOBAL_TEST_LOCK.lock().await;
let client = ClickHouseClient::from_params(ConnectParams {
connection_id: 404,
connection_string: Some("mock://localhost:8123/default".to_string()),
..Default::default()
})
.unwrap();
ConnectionPool::global().insert(client);

let params = json!({
"connectionId": 404,
"nodeType": "column",
"nodeId": "col.analytics.events.user_id"
});

let res = handle_context_actions(Some(params)).await.unwrap();
let actions = res["actions"].as_array().unwrap();
assert_eq!(actions.len(), 2);
assert_eq!(actions[0]["id"], "column.stats");
assert!(
actions[0]["sql"]
.as_str()
.unwrap()
.contains("min(`user_id`)")
);
assert_eq!(actions[1]["id"], "column.top_10");

ConnectionPool::global().remove(404);
}

#[tokio::test]
async fn test_handle_get_capabilities() {
let _guard = crate::utils::test_lock::GLOBAL_TEST_LOCK.lock().await;
Expand Down
159 changes: 133 additions & 26 deletions src/sdui/actions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,27 @@ impl SduiContextAction {
}
}

/// Returns `true` if a ClickHouse column type is a complex type (`Array`, `Map`,
/// `Tuple`) that the `min`/`max`/`topK` aggregate functions cannot operate on
/// directly, so the Column Profiler must fall back to type-appropriate queries.
fn is_complex_clickhouse_type(column_type: &str) -> bool {
let mut t = column_type.trim();
if let Some(inner) = t.strip_prefix("Nullable(") {
t = inner.trim_end_matches(')');
}
t.starts_with("Array(") || t.starts_with("Map(") || t.starts_with("Tuple(")
}

/// Generates SDUI context menu actions based on `nodeType` and `nodeId`.
///
/// `column_type` is the ClickHouse type of the target column (e.g. from
/// `system.columns`), used only for `nodeType == "column"` to choose a
/// profiling query that's valid for the column's type. Pass `None` when the
/// type is unknown; the scalar-oriented query is used as a safe default.
pub fn get_context_actions_for_node(
node_type: &str,
node_id: &str,
column_type: Option<&str>,
) -> Result<Vec<SduiContextAction>, DriverError> {
let parts: Vec<String> = split_node_id(node_id);

Expand Down Expand Up @@ -372,20 +389,35 @@ pub fn get_context_actions_for_node(
let q_tbl = quote_identifier(table_name);
let q_col = quote_identifier(col_name);

Ok(vec![
SduiContextAction::new(
"column.stats",
"📈 Column Statistics (Быстрый профайлер)",
Some("bar-chart"),
"query",
Some(format!(
// `min`/`max`/`topK` cannot operate on Array/Map/Tuple columns in
// ClickHouse (ILLEGAL_TYPE_OF_ARGUMENT), and GROUP BY on a Map column
// is rejected outright, so complex-typed columns get a profiling
// query built from length()/uniqueness instead, and no "Top 10
// Frequent Values" action (see issue #60).
let is_complex = column_type.is_some_and(is_complex_clickhouse_type);

let mut actions = vec![SduiContextAction::new(
"column.stats",
"📈 Column Statistics (Быстрый профайлер)",
Some("bar-chart"),
"query",
Some(if is_complex {
format!(
"SELECT count() as total_rows, countIf(isNotNull({0})) as not_nulls, uniqExact({0}) as unique_exact, min(length({0})) as min_length, max(length({0})) as max_length FROM {1}.{2}",
q_col, q_db, q_tbl
)
} else {
format!(
"SELECT count() as total_rows, countIf(isNotNull({0})) as not_nulls, uniqExact({0}) as unique_exact, min({0}) as min_val, max({0}) as max_val, topK(5)({0}) as top_5_values FROM {1}.{2}",
q_col, q_db, q_tbl
)),
false,
false,
),
SduiContextAction::new(
)
}),
false,
false,
)];

if !is_complex {
actions.push(SduiContextAction::new(
"column.top_10",
"🔝 Top 10 Frequent Values",
Some("list"),
Expand All @@ -396,8 +428,10 @@ pub fn get_context_actions_for_node(
)),
false,
false,
),
])
));
}

Ok(actions)
}
_ => Ok(Vec::new()),
}
Expand All @@ -409,7 +443,8 @@ mod tests {

#[test]
fn test_table_context_actions() {
let actions = get_context_actions_for_node("table", "table.analytics.events").unwrap();
let actions =
get_context_actions_for_node("table", "table.analytics.events", None).unwrap();
assert_eq!(actions.len(), 8);
assert_eq!(actions[0].id, "table.top_100");
assert_eq!(
Expand All @@ -432,7 +467,8 @@ mod tests {
#[test]
fn test_partition_context_actions() {
let actions =
get_context_actions_for_node("partition", "part.analytics.events.202607").unwrap();
get_context_actions_for_node("partition", "part.analytics.events.202607", None)
.unwrap();
assert_eq!(actions.len(), 6);
assert_eq!(actions[0].id, "partition.drop");
assert_eq!(
Expand Down Expand Up @@ -483,15 +519,15 @@ mod tests {

#[test]
fn test_database_and_view_actions() {
let db_actions = get_context_actions_for_node("database", "db.analytics").unwrap();
let db_actions = get_context_actions_for_node("database", "db.analytics", None).unwrap();
assert_eq!(db_actions.len(), 4);
assert_eq!(db_actions[0].id, "db.active_mutations");
assert_eq!(db_actions[1].id, "db.active_queries");
assert_eq!(db_actions[2].id, "db.kill_mutations");
assert_eq!(db_actions[3].id, "db.kill_queries");

let view_actions =
get_context_actions_for_node("view", "view.analytics.mv_summary").unwrap();
get_context_actions_for_node("view", "view.analytics.mv_summary", None).unwrap();
assert_eq!(view_actions.len(), 2);
assert_eq!(view_actions[0].id, "view.top_100");
assert_eq!(
Expand All @@ -502,7 +538,8 @@ mod tests {

#[test]
fn test_server_and_process_monitoring_actions() {
let server_actions = get_context_actions_for_node("server", "server.cluster").unwrap();
let server_actions =
get_context_actions_for_node("server", "server.cluster", None).unwrap();
assert_eq!(server_actions.len(), 3);
assert_eq!(server_actions[0].id, "server.active_mutations");
assert_eq!(server_actions[1].id, "server.active_queries");
Expand All @@ -513,7 +550,7 @@ mod tests {
#[test]
fn test_column_context_actions() {
let actions =
get_context_actions_for_node("column", "col.analytics.events.user_id").unwrap();
get_context_actions_for_node("column", "col.analytics.events.user_id", None).unwrap();
assert_eq!(actions.len(), 2);
assert_eq!(actions[0].id, "column.stats");
assert_eq!(
Expand All @@ -533,10 +570,80 @@ mod tests {
);
}

#[test]
fn test_column_context_actions_for_complex_types() {
// Regression for issue #60: min()/max()/topK() abort with
// ILLEGAL_TYPE_OF_ARGUMENT on Array/Map/Tuple columns, and GROUP BY
// on a raw Map column is rejected outright, so complex-typed columns
// must get a length()-based profiling query and no top_10 action.
for complex_type in [
"Array(String)",
"Map(String, UInt64)",
"Tuple(Int32, String)",
"Nullable(Array(String))",
] {
let actions = get_context_actions_for_node(
"column",
"col.analytics.events.tags",
Some(complex_type),
)
.unwrap();
assert_eq!(
actions.len(),
1,
"expected only column.stats for type {}",
complex_type
);
assert_eq!(actions[0].id, "column.stats");
let sql = actions[0].sql.as_deref().unwrap();
assert!(
!sql.contains("min(`tags`)"),
"type {}: {}",
complex_type,
sql
);
assert!(
!sql.contains("max(`tags`)"),
"type {}: {}",
complex_type,
sql
);
assert!(!sql.contains("topK"), "type {}: {}", complex_type, sql);
assert!(
sql.contains("min(length(`tags`))"),
"type {}: {}",
complex_type,
sql
);
assert!(
sql.contains("max(length(`tags`))"),
"type {}: {}",
complex_type,
sql
);
}

// A plain scalar type keeps the original min/max/topK query and the top_10 action.
let scalar_actions = get_context_actions_for_node(
"column",
"col.analytics.events.tags",
Some("LowCardinality(String)"),
)
.unwrap();
assert_eq!(scalar_actions.len(), 2);
assert!(
scalar_actions[0]
.sql
.as_deref()
.unwrap()
.contains("min(`tags`)")
);
}

#[test]
fn test_context_actions_sql_injection_protection() {
let malicious_part = "part.db`test.tbl'test.2026'; DROP TABLE secret; --";
let actions = get_context_actions_for_node("partition", malicious_part).unwrap();
let actions = get_context_actions_for_node("partition", malicious_part, None).unwrap();
assert_eq!(
actions[0].sql.as_deref(),
Some(
Expand All @@ -545,7 +652,7 @@ mod tests {
);

let malicious_col = "col.db.tbl.user_id`; DROP TABLE users; --";
let col_actions = get_context_actions_for_node("column", malicious_col).unwrap();
let col_actions = get_context_actions_for_node("column", malicious_col, None).unwrap();
assert!(
col_actions[1]
.sql
Expand All @@ -566,7 +673,7 @@ mod tests {
encode_id_segment("analytics"),
encode_id_segment("weird.table.name")
);
let actions = get_context_actions_for_node("table", &node_id).unwrap();
let actions = get_context_actions_for_node("table", &node_id, None).unwrap();
assert_eq!(
actions[0].sql.as_deref(),
Some("SELECT * FROM `analytics`.`weird.table.name` LIMIT 100")
Expand All @@ -575,8 +682,8 @@ mod tests {

#[test]
fn test_invalid_node_id() {
assert!(get_context_actions_for_node("table", "table.only").is_err());
assert!(get_context_actions_for_node("partition", "part.only.two").is_err());
assert!(get_context_actions_for_node("column", "col.only.two").is_err());
assert!(get_context_actions_for_node("table", "table.only", None).is_err());
assert!(get_context_actions_for_node("partition", "part.only.two", None).is_err());
assert!(get_context_actions_for_node("column", "col.only.two", None).is_err());
}
}
Loading