Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
[workspace]
members = [
"crates/types",
"crates/sql-function-catalog",
"crates/asap-aware-mapping",
"crates/frontend-promql",
"crates/frontend-sql",
Expand Down
5 changes: 4 additions & 1 deletion crates/frontend-sql/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ version = "0.1.0"
edition = "2021"

# SQL front end: L1 (parse + plan via DataFusion) → L2 relational, then the
# shared L2→L3 converter — both in asap-types. Pulls DataFusion only — never promql-parser.
# shared L2→L3 converter — both in asap-types. Pulls DataFusion, plus the
# independent SQL function-name catalog (asap-sql-function-catalog, issue
# #225) it consults when lowering an aggregate call — never promql-parser.
[dependencies]
asap-types = { path = "../types" }
asap-sql-function-catalog = { path = "../sql-function-catalog" }
datafusion = "43"

[dev-dependencies]
Expand Down
6 changes: 4 additions & 2 deletions crates/frontend-sql/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ pub async fn lower_sql(
/// Lower a single SQL query string under an explicit [`SqlDialect`].
///
/// `ClickhouseSQL` parses via sqlparser's vendored `ClickHouseDialect`
/// (array-lambda syntax, `arr[-1]` indexing) — it does not teach DataFusion's
/// planner any ClickHouse-only builtin functions, which still fail to plan.
/// (array-lambda syntax, `arr[-1]` indexing). It also teaches DataFusion's
/// planner the ClickHouse-only builtin functions listed in
/// `asap_sql_function_catalog::CLICKHOUSE_BUILTINS` (`uniqExact`, `countIf`)
/// — every other ClickHouse-only builtin still fails to plan.
/// `ElasticSQL` has no vendored parser and always returns `UnsupportedDialect`.
pub async fn lower_sql_dialect(
query: &str,
Expand Down
255 changes: 157 additions & 98 deletions crates/frontend-sql/src/sql/mod.rs

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -160,15 +160,20 @@ async fn corpus_lowering_matches_the_pinned_aggregate_tally() {
gap was added/removed: {tally:?}"
);
};
expect(Category::Lowered, 85);
expect(Category::Plan, 105);
// `countIf` support (issue #225 -- `ClickHouseBuiltinRewrite`, catalog-
// driven the same way `uniqExact` (#221) already was) clears the
// "unknown function: countif" `Plan` failure for 13 queries: 12 now
// lower end to end, and a 13th plans far enough to hit a second,
// pre-existing gap (map/array index access, `NotImplemented`).
expect(Category::Lowered, 97);
expect(Category::Plan, 92);
expect(Category::Schema, 0);
expect(Category::Parse, 0);
// One query that used to fail at `uniqExact` (`Plan`) now clears that
// hurdle -- `UniqExactRewrite` in `sql/mod.rs` rewrites it to
// hurdle -- `ClickHouseBuiltinRewrite` in `sql/mod.rs` rewrites it to
// `COUNT(DISTINCT ...)` before `lower_plan` runs -- and plans far enough
// to hit a second, pre-existing gap: map/array index access.
expect(Category::NotImplemented, 4);
expect(Category::NotImplemented, 5);
expect(Category::UnsupportedFeature, 6);
expect(Category::Other, 0);
}
76 changes: 75 additions & 1 deletion crates/frontend-sql/tests/sql_lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@
//! the shared `resolve_root` produces the positional, resolved canonical
//! tree (the same resolver the PromQL path uses).

use asap_frontend_sql::{lower_sql, SqlCatalog};
use asap_frontend_sql::{lower_sql, lower_sql_dialect, SqlCatalog};
use asap_types::pre_asap::schema::{Column, DataType, Schema};
use asap_types::pre_asap::{
AggIntent, CompareOp, GroupKeys, JoinKind, QueryExpr, Reduction, ScalarValue, Source,
WindowFuncKind,
};
use asap_types::types::AccuracyTarget;
use asap_types::workload::SqlDialect;

fn col(name: &str, dtype: DataType) -> Column {
Column::new(name, dtype, false)
Expand Down Expand Up @@ -1392,3 +1393,76 @@ async fn array_agg_is_deliberately_rejected() {
"expected a clean UnsupportedAggregate, got {err}"
);
}

// ── Issue #225: catalog-driven ClickHouse builtins (countIf, generalizing
// uniqExact from #221) ───────────────────────────────────────────────────

async fn lower_clickhouse(sql: &str) -> QueryExpr {
lower_sql_dialect(
sql,
&catalog(),
SqlDialect::ClickhouseSQL,
AccuracyTarget::Exact,
)
.await
.unwrap_or_else(|e| panic!("lower failed for {sql:?}: {e}"))
}

#[tokio::test]
async fn count_if_lowers_to_a_sum_over_a_derived_indicator_column() {
// ClickHouse's `countIf(cond)` has no DataFusion equivalent at all, so it
// goes through the same stub-UDAF + catalog-driven `FunctionRewrite`
// mechanism `uniqExact` (#221) does — rewritten, before `lower_agg_intent`
// ever runs, to `sum(CASE WHEN cond THEN 1 ELSE 0 END)`. Not a plain
// `count(...) FILTER (WHERE cond)`: `AggIntent::Count` never consults its
// argument (always a row count), so the filter would be silently dropped;
// summing a 0/1 indicator keeps `cond` observable through the ordinary
// `Sum` path instead.
let qe = lower_clickhouse("SELECT countIf(bytes > 100) AS big FROM metrics").await;
let (by, measures) = find_aggregate(&qe).expect("expected an Aggregate");
assert!(by.is_empty());
assert!(
matches!(measures.as_slice(), [AggIntent::Sum { col: Some(_) }]),
"expected a Sum bound to the derived indicator column, got {measures:?}"
);
let (_, materialized) = reducer_input_names(&qe);
assert!(
materialized,
"the indicator expression must be materialized in a Project beneath the Aggregate"
);
}

#[tokio::test]
async fn two_count_ifs_with_different_conditions_stay_distinct_reducers() {
// The corpus pattern (`countIf(operation = 'A'), countIf(operation = 'W')`
// in one GROUP BY) needs each call's own condition to survive as its own
// derived column, not collapse onto a shared one.
let qe = lower_clickhouse(
"SELECT service, countIf(bytes > 100) AS big, countIf(bytes <= 100) AS small \
FROM metrics GROUP BY service",
)
.await;
let (by, measures) = find_aggregate(&qe).expect("expected an Aggregate");
assert_eq!(*by, GroupKeys::by(vec![0]));
assert!(
matches!(
measures.as_slice(),
[
AggIntent::Sum { col: Some(a) },
AggIntent::Sum { col: Some(b) }
] if a != b
),
"expected two distinct Sum reducers, got {measures:?}"
);
}

#[tokio::test]
async fn count_if_composes_with_group_by() {
let qe = lower_clickhouse(
"SELECT service, countIf(bytes > 100) AS big FROM metrics GROUP BY service",
)
.await;
let (by, measures) = find_aggregate(&qe).expect("expected an Aggregate");
assert_eq!(*by, GroupKeys::by(vec![0]));
assert!(matches!(measures.as_slice(), [AggIntent::Sum { .. }]));
}
14 changes: 14 additions & 0 deletions crates/sql-function-catalog/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
[package]
name = "asap-sql-function-catalog"
version = "0.1.0"
edition = "2021"

# The (SQL function name, arity) -> canonical AggIntent-shaped semantic
# catalog `asap-frontend-sql` consults when lowering an aggregate call, plus
# the ClickHouse-only-builtin -> native-DataFusion-shape rewrite table
# (issue #225). No internal deps -- like `asap-types`, this is meant to be
# consultable on its own (a future extraction tool populating it, a test
# asserting a name is covered, ...) without pulling in DataFusion or the
# front end that uses it. `asap-frontend-sql` depends on this crate, never
# the other way around.
[dependencies]
Loading
Loading