diff --git a/nodedb-sql/src/engine_rules/timeseries.rs b/nodedb-sql/src/engine_rules/timeseries.rs index f5ea41f9a..dc7b547a2 100644 --- a/nodedb-sql/src/engine_rules/timeseries.rs +++ b/nodedb-sql/src/engine_rules/timeseries.rs @@ -111,6 +111,21 @@ impl EngineRules for TimeseriesRules { ), }); } + // A native `TimeseriesScan` carries no `having` slot, so a predicate + // reaching here would be dropped and the statement would answer groups + // the user filtered out. Refuse it instead. Every other engine forwards + // `having`; this one cannot, and a refusal tells the user that where a + // silent widening of the result set does not. + if !p.having.is_empty() { + return Err(SqlError::Unsupported { + detail: format!( + "HAVING is not supported on timeseries collection '{}'; \ + the native time-series aggregate cannot express a predicate \ + over the aggregate result. Filter the input with WHERE instead", + p.collection + ), + }); + } Ok(SqlPlan::TimeseriesScan { collection: p.collection, time_range: default_time_range(), diff --git a/nodedb-sql/src/planner/select/select_stmt.rs b/nodedb-sql/src/planner/select/select_stmt.rs index 16b9faece..fc5c41ee7 100644 --- a/nodedb-sql/src/planner/select/select_stmt.rs +++ b/nodedb-sql/src/planner/select/select_stmt.rs @@ -378,11 +378,20 @@ fn has_column_comparison(expr: &SqlExpr) -> bool { } } -/// Check if a SELECT has aggregation (GROUP BY or aggregate functions in projection). +/// Check if a SELECT has aggregation (GROUP BY, aggregate functions in +/// projection, or a HAVING clause). pub(in crate::planner::select) fn has_aggregation( select: &Select, functions: &FunctionRegistry, ) -> bool { + // A HAVING clause is a statement about group results, so it needs the + // aggregate path even when the projection happens to name no aggregate: + // `SELECT host FROM ts HAVING COUNT(*) > 1` is an aggregate statement whose + // predicate the non-aggregate path never reads. Leaving it out dropped the + // predicate and answered every row. + if select.having.is_some() { + return true; + } let group_by_non_empty = match &select.group_by { ast::GroupByExpr::All(_) => true, ast::GroupByExpr::Expressions(exprs, _) => !exprs.is_empty(), diff --git a/nodedb/tests/wire/cases/having_matrix_all_engines.rs b/nodedb/tests/wire/cases/having_matrix_all_engines.rs new file mode 100644 index 000000000..bfaf66e4f --- /dev/null +++ b/nodedb/tests/wire/cases/having_matrix_all_engines.rs @@ -0,0 +1,319 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! The exhaustive `HAVING` matrix: every engine, every clause shape. +//! +//! `HAVING` filters the result of an aggregation. The planner reaches the +//! aggregate path through several routes, and an engine rule that cannot apply +//! the predicate must refuse it. What no engine may do is accept the statement +//! and silently widen the result set, because the user then sees rows they +//! explicitly excluded and no error to explain it. +//! +//! This file walks the whole space rather than the one shape an issue happened +//! to report. A guard that closes one route and leaves another open is not a +//! guard, and the only way to say which routes are closed is to exercise each +//! engine against each clause shape. +//! +//! Engines covered: `document_schemaless`, `document_strict`, `kv`, `columnar`, +//! `spatial`, `timeseries`, plus the default engine a bare `CREATE COLLECTION` +//! selects. + +use crate::harness::TestServer; + +/// One engine's create statement and seed values. +/// +/// Each engine states its whole DDL rather than sharing a column prefix: the +/// engines do not agree on the shape. `kv` demands a `PRIMARY KEY`, `spatial` +/// only accepts `COLUMNS (...)` with a `GEOMETRY` column, and `timeseries` +/// needs a `TIME_KEY`. A shared prefix could express none of those, and a +/// fixture that fails on CREATE tests the fixture, not the clause under test. +struct EngineCase { + /// The `CREATE COLLECTION` statement, complete. + create: &'static str, + /// Columns for the `INSERT`, matching `create`. + columns: &'static str, + /// One row's values, without the surrounding parentheses. + row: &'static str, +} + +/// Three groups with counts 3, 2 and 1, so `HAVING COUNT(*) > 1` has something +/// to exclude. `{n}` is the row ordinal. +const GROUPS: &[(&str, usize)] = &[("g_a", 3), ("g_b", 2), ("g_c", 1)]; + +const CASES: &[(&str, EngineCase)] = &[ + ( + "default", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT)", + columns: "id, grp", + row: "'id{n}', '{grp}'", + }, + ), + ( + "document_schemaless", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT) WITH (engine='document_schemaless')", + columns: "id, grp", + row: "'id{n}', '{grp}'", + }, + ), + ( + "document_strict", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT) WITH (engine='document_strict')", + columns: "id, grp", + row: "'id{n}', '{grp}'", + }, + ), + ( + "kv", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT) WITH (engine='kv')", + columns: "id, grp", + row: "'id{n}', '{grp}'", + }, + ), + ( + "columnar", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT) WITH (engine='columnar')", + columns: "id, grp", + row: "'id{n}', '{grp}'", + }, + ), + ( + "spatial", + EngineCase { + create: "CREATE COLLECTION {c} COLUMNS (id TEXT PRIMARY KEY, grp TEXT, location GEOMETRY) WITH (engine='spatial')", + columns: "id, grp, location", + row: "'id{n}', '{grp}', ST_Point(0.0, 0.0)", + }, + ), + ( + "timeseries", + EngineCase { + create: "CREATE COLLECTION {c} (id TEXT PRIMARY KEY, grp TEXT, ts TIMESTAMP TIME_KEY) WITH (engine='timeseries')", + columns: "id, grp, ts", + row: "'id{n}', '{grp}', '2020-03-05 10:00:00'", + }, + ), +]; + +/// Create `collection` for `case` and seed three groups with counts 3, 2, 1. +async fn seed(server: &TestServer, collection: &str, case: &EngineCase) { + let create = case.create.replace("{c}", collection); + server + .exec(&create) + .await + .unwrap_or_else(|e| panic!("create {collection}: {e}\n ddl: {create}")); + + let mut rows = Vec::new(); + let mut n = 0; + for (grp, count) in GROUPS { + for _ in 0..*count { + rows.push(format!( + "({})", + case.row + .replace("{n}", &n.to_string()) + .replace("{grp}", grp) + )); + n += 1; + } + } + let insert = format!( + "INSERT INTO {collection} ({}) VALUES {}", + case.columns, + rows.join(", ") + ); + server + .exec(&insert) + .await + .unwrap_or_else(|e| panic!("seed {collection}: {e}")); +} + +/// The outcome a `HAVING` statement is allowed to have. +enum Outcome { + /// The predicate applied: `g_c` (count 1) is absent. + Filtered, + /// The engine refused the clause, naming why. + Refused, +} + +/// Classify what `sql` did, failing when the clause was silently dropped. +async fn classify(server: &TestServer, sql: &str, label: &str) -> Outcome { + match server.query_named_rows(sql).await { + Err(error) => { + let message = error.to_string(); + assert!( + !message.is_empty(), + "{label}: a refusal must name its reason, got an empty error" + ); + Outcome::Refused + } + Ok(rows) => { + let groups: Vec = rows.iter().filter_map(|r| r.get("grp").cloned()).collect(); + // An empty result would satisfy the exclusion assertion below + // vacuously, so the surviving groups are required here first: the + // statement answered, and it answered the groups the predicate keeps. + assert_eq!( + groups.len(), + 2, + "{label}: `HAVING COUNT(*) > 1` keeps g_a (3) and g_b (2) and \ + excludes g_c (1); an empty or short result would pass the \ + exclusion check without testing it: {rows:?}" + ); + assert!( + groups.iter().any(|g| g == "g_a") && groups.iter().any(|g| g == "g_b"), + "{label}: the kept groups must both be present: {rows:?}" + ); + assert!( + !groups.iter().any(|g| g == "g_c"), + "{label}: `HAVING COUNT(*) > 1` excludes g_c (one row), but the \ + statement returned it — the clause was silently dropped: {rows:?}" + ); + Outcome::Filtered + } + } +} + +/// Every engine, the canonical shape: `GROUP BY` plus `HAVING` over `COUNT(*)`. +#[tokio::test] +async fn matrix_group_by_with_having() { + let server = TestServer::start().await; + for (name, case) in CASES { + let collection = format!("mx_gb_{name}"); + seed(&server, &collection, case).await; + let outcome = classify( + &server, + &format!("SELECT grp, COUNT(*) FROM {collection} GROUP BY grp HAVING COUNT(*) > 1"), + name, + ) + .await; + // Either outcome is acceptable; the assertion inside `classify` is the + // contract. Recorded so a future change that flips an engine from + // refusing to filtering is visible rather than silent. + match outcome { + Outcome::Filtered => {} + Outcome::Refused => {} + } + } +} + +/// `HAVING` with no `GROUP BY` and no aggregate in the projection. +/// +/// The statement is still an aggregate statement, so the clause must reach a +/// rule that can judge it. Its result shape differs from the `GROUP BY` case: +/// with no grouping it can only produce one grand-total row, or refuse. Two +/// grouped rows would mean the engine invented a grouping the statement did not +/// ask for, and `g_c` coming back means the clause was dropped. +#[tokio::test] +async fn matrix_having_without_group_by() { + let server = TestServer::start().await; + for (name, case) in CASES { + let collection = format!("mx_bare_{name}"); + seed(&server, &collection, case).await; + + match server + .query_named_rows(&format!("SELECT grp FROM {collection} HAVING COUNT(*) > 1")) + .await + { + Err(error) => { + let message = error.to_string(); + assert!( + !message.is_empty(), + "{name}: a refusal must name its reason, got an empty error" + ); + } + Ok(rows) => { + assert!( + rows.len() <= 1, + "{name}: a HAVING with no GROUP BY aggregates the whole \ + collection into at most one row; {} rows means the \ + grouping was invented: {rows:?}", + rows.len() + ); + let groups: Vec = + rows.iter().filter_map(|r| r.get("grp").cloned()).collect(); + assert!( + !groups.iter().any(|g| g == "g_c"), + "{name}: `HAVING COUNT(*) > 1` excludes g_c, but the \ + statement returned it — the clause was dropped: {rows:?}" + ); + } + } + } +} + +/// `HAVING` over a join. The join planner builds its own aggregate node rather +/// than consulting the engine rule, so this route is the one most likely to +/// keep dropping the clause. +/// +/// The join route is a *separate* defect from the one this PR fixes: the join +/// does not merely ignore `HAVING`, it answers input rows under an aggregate +/// heading (`count(*)` comes back empty). That is its own issue, with its own +/// reproduction, and it affects every engine including the default — not just +/// timeseries. This test therefore records the current behaviour rather than +/// asserting the fixed one, so the PR does not claim a fix it does not make. +#[tokio::test] +async fn matrix_having_over_a_join_is_a_separate_defect() { + let server = TestServer::start().await; + for (name, case) in CASES { + let collection = format!("mx_join_{name}"); + let counterpart = format!("mx_join_side_{name}"); + seed(&server, &collection, case).await; + server + .exec(&format!( + "CREATE COLLECTION {counterpart} (grp TEXT PRIMARY KEY, label TEXT)" + )) + .await + .unwrap_or_else(|e| panic!("create join side for {name}: {e}")); + server + .exec(&format!( + "INSERT INTO {counterpart} (grp, label) VALUES ('g_a', 'x'), ('g_b', 'y'), ('g_c', 'z')" + )) + .await + .unwrap_or_else(|e| panic!("seed join side for {name}: {e}")); + + // Either the clause applies (g_c, count 1, is excluded and the aggregate + // is computed) or the statement refuses. Anything else is the join bug. + let result = server + .query_named_rows(&format!( + "SELECT l.grp, COUNT(*) FROM {collection} l \ + JOIN {counterpart} r ON l.grp = r.grp \ + GROUP BY l.grp HAVING COUNT(*) > 1" + )) + .await; + + let Ok(rows) = result else { continue }; + let groups: Vec = rows.iter().filter_map(|r| r.get("grp").cloned()).collect(); + let counts_empty = rows + .iter() + .all(|r| r.get("count(*)").is_some_and(|v| v.is_empty())); + if groups.iter().any(|g| g == "g_c") || counts_empty { + // The known join defect. Recorded, not asserted as correct; the + // issue carrying its reproduction is filed separately. + eprintln!("known join defect on {name}: HAVING dropped or aggregate empty — {rows:?}"); + } + } +} + +/// `WHERE` is not affected by any of this: it filters input rows and must keep +/// working on every engine, including the ones that refuse `HAVING`. +#[tokio::test] +async fn matrix_where_still_filters() { + let server = TestServer::start().await; + for (name, case) in CASES { + let collection = format!("mx_where_{name}"); + seed(&server, &collection, case).await; + let rows = server + .query_named_rows(&format!( + "SELECT grp, COUNT(*) FROM {collection} WHERE grp <> 'g_c' GROUP BY grp" + )) + .await + .unwrap_or_else(|e| panic!("{name}: WHERE must still work: {e}")); + let groups: Vec = rows.iter().filter_map(|r| r.get("grp").cloned()).collect(); + assert!( + !groups.iter().any(|g| g == "g_c"), + "{name}: WHERE grp <> 'g_c' must exclude it: {rows:?}" + ); + } +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index f58a3ffcf..440e0105d 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -94,6 +94,7 @@ mod graph_vector_write_row_level_security; mod group_by_computed_key; mod group_by_typed_columns; mod group_by_unaliased_aggregate; +mod having_matrix_all_engines; mod healthz_authorization_lease; mod healthz_calvin_readiness; mod http_result_projection; @@ -308,6 +309,7 @@ mod strict_bitemporal_select_star; mod strict_schema_restart; mod strict_typed_column_rendering; mod timeseries_declared_time_key; +mod timeseries_having_predicate; mod timeseries_join_time_rendering; mod timeseries_read_row_level_security; mod timeseries_write_row_level_security; diff --git a/nodedb/tests/wire/cases/timeseries_having_predicate.rs b/nodedb/tests/wire/cases/timeseries_having_predicate.rs new file mode 100644 index 000000000..5ff4cb34f --- /dev/null +++ b/nodedb/tests/wire/cases/timeseries_having_predicate.rs @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! `HAVING` must filter a timeseries aggregate, or refuse it — never vanish. +//! +//! The timeseries engine rule lowers every aggregate to a native +//! `SqlPlan::TimeseriesScan`. That plan carries `group_by`, `aggregates`, +//! `filters`, `gap_fill`, `limit` and `sort_keys` — but no `having` slot, and +//! the rule never reads `params.having`. Every other engine rule forwards it +//! (`columnar.rs:148`, `document_schemaless.rs:153`, `document_strict.rs:153`, +//! `kv.rs:128`, `spatial.rs:140` all write `having: p.having`). +//! +//! So a `HAVING` over a timeseries aggregate is silently dropped: the planner +//! builds the predicate (`engine_rules/params.rs:146`), the rule discards it, +//! and the query answers every group the `WHERE` clause left. The user gets +//! rows they explicitly filtered out, with no error, which is worse than a +//! refusal: a refusal tells them the engine cannot do it. +//! +//! The engine refuses the clause rather than filter it, because the native +//! `TimeseriesScan` has no slot for a predicate over the aggregate result. So +//! these tests assert the refusal — a typed error naming the clause and the +//! collection — together with the two controls that make the refusal meaningful: +//! the same aggregate without `HAVING` still answers, and `WHERE`, which filters +//! input rows rather than group results, still filters. What must never happen is +//! the predicate disappearing while the statement reports success. +//! +//! `having_matrix_all_engines.rs` carries the same clause across every engine and +//! accepts either outcome, because the engines that can express the predicate do +//! filter it. + +use crate::harness::TestServer; + +/// Seed three hosts with distinguishable counts, so a `HAVING` that fires and +/// one that is dropped produce different row sets. +async fn seed(server: &TestServer, collection: &str) { + server + .exec(&format!( + "CREATE COLLECTION {collection} (ts TIMESTAMP TIME_KEY, host TEXT, value FLOAT) \ + WITH (engine='timeseries')" + )) + .await + .expect("create the timeseries collection"); + + let mut rows = Vec::new(); + // host_a: 3 events, host_b: 2, host_c: 1. + for (host, count) in [("host_a", 3), ("host_b", 2), ("host_c", 1)] { + for i in 0..count { + rows.push(format!("('2020-03-05 1{i}:00:00', '{host}', 1.0)")); + } + } + server + .exec(&format!( + "INSERT INTO {collection} (ts, host, value) VALUES {}", + rows.join(", ") + )) + .await + .expect("seed the events"); +} + +/// Without `HAVING`, every host groups. This is the control: it proves the +/// grouping itself works, so a failure above it is the `HAVING` clause and not +/// the aggregate. +#[tokio::test] +async fn group_by_without_having_returns_every_group() { + let server = TestServer::start().await; + seed(&server, "ts_no_having").await; + + let rows = server + .query_named_rows("SELECT host, COUNT(*) FROM ts_no_having GROUP BY host") + .await + .expect("GROUP BY without HAVING must answer"); + + assert_eq!( + rows.len(), + 3, + "all three hosts group when nothing filters them: {rows:?}" + ); +} + +/// A `HAVING` over a timeseries aggregate is refused with a typed error naming +/// the collection, because the native `TimeseriesScan` cannot express a +/// predicate over the aggregate result. +/// +/// The defect this pins is the silent alternative: before the fix the predicate +/// reached `plan_aggregate`, no slot carried it, and the statement answered +/// every group the `WHERE` clause left — rows the user had explicitly filtered +/// out, with no error. A refusal is the honest outcome; silently widening the +/// result set is not. +#[tokio::test] +async fn having_is_refused_not_silently_dropped() { + let server = TestServer::start().await; + seed(&server, "ts_having").await; + + let error = server + .query_named_rows("SELECT host, COUNT(*) FROM ts_having GROUP BY host HAVING COUNT(*) > 1") + .await + .expect_err("HAVING over a timeseries aggregate must refuse, not answer"); + + let message = error.to_string(); + assert!( + message.contains("HAVING") && message.contains("ts_having"), + "the refusal must name the clause and the collection, got: {message}" + ); + assert!( + message.contains("WHERE"), + "the refusal must name the alternative that works, got: {message}" + ); +} + +/// The guard is narrow: a `HAVING`-less aggregate still answers. Without this, +/// a refusal that swallowed every timeseries aggregate would pass the test +/// above, and the suite would not notice. +#[tokio::test] +async fn an_aggregate_without_having_still_answers() { + let server = TestServer::start().await; + seed(&server, "ts_still_ok").await; + + let rows = server + .query_named_rows("SELECT host, COUNT(*) FROM ts_still_ok GROUP BY host") + .await + .expect("an aggregate without HAVING must still answer"); + assert_eq!(rows.len(), 3, "all three hosts group: {rows:?}"); +} + +/// The same predicate written as a `WHERE` filter does work, which is what makes +/// the silent drop so hard to notice: the two look alike and only one applies. +#[tokio::test] +async fn where_equivalent_still_filters() { + let server = TestServer::start().await; + seed(&server, "ts_where").await; + + let rows = server + .query_named_rows( + "SELECT host, COUNT(*) FROM ts_where WHERE host <> 'host_c' GROUP BY host", + ) + .await + .expect("WHERE must filter"); + + let hosts: Vec = rows.iter().filter_map(|r| r.get("host").cloned()).collect(); + assert_eq!( + hosts.len(), + 2, + "the WHERE equivalent excludes host_c: {rows:?}" + ); +} + +/// The other shape that dropped `HAVING`: no `GROUP BY`, and a projection whose +/// only aggregate lives inside the predicate. +/// +/// `has_aggregation` decided the statement was not an aggregate one, so the +/// planner took the non-aggregate path and never read the clause. The predicate +/// then vanished and every row came back. +#[tokio::test] +async fn having_without_group_by_is_refused_not_dropped() { + let server = TestServer::start().await; + seed(&server, "ts_bare_having").await; + + let error = server + .query_named_rows("SELECT host FROM ts_bare_having HAVING COUNT(*) > 1") + .await + .expect_err("HAVING without GROUP BY must refuse on a timeseries collection"); + + let message = error.to_string(); + assert!( + message.contains("HAVING") && message.contains("ts_bare_having"), + "the refusal must name the clause and the collection, got: {message}" + ); +}