Skip to content

Commit d58d052

Browse files
committed
test: add regression coverage for aggregate ordering, DDL correctness, and memory accounting
New test files: - sql_order_by: GROUP BY + ORDER BY must sort groups in declared order. - sql_aggregate_functions: aggregate alias casing and count_distinct must not return NULL under any column-name mapping. - sql_subquery_from: FROM (SELECT …) AS t must plan and execute correctly. - sql_drop_collection: DROP COLLECTION IF EXISTS must not error when the target does not exist; DROP TABLE must be accepted as a synonym. - memory_accounting_balance: over-release counter must be zero after a balanced alloc/release cycle; non-zero signals an accounting bug. - engine_surface_timeseries: continuous aggregate must appear in SHOW after CREATE and survive a server restart. - catalog_integrity_verify: continuous aggregates included in orphan-row integrity sweep. Existing tests updated to compile against the new sort_keys field on SqlPlan::Aggregate and the expanded pgwire harness helper surface.
1 parent 73c7e96 commit d58d052

16 files changed

Lines changed: 891 additions & 0 deletions

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.

nodedb-test-support/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ nodedb = { workspace = true }
1616
nodedb-array = { workspace = true }
1717
nodedb-bridge = { workspace = true }
1818
nodedb-cluster = { workspace = true }
19+
nodedb-mem = { workspace = true }
1920
nodedb-types = { workspace = true }
2021
nodedb-wal = { workspace = true }
2122

nodedb-test-support/src/pgwire_harness.rs

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,30 @@ use std::time::Duration;
1010

1111
use nodedb::bridge::dispatch::Dispatcher;
1212

13+
/// Build a `MemoryGovernor` sized for integration tests: 64 MiB per
14+
/// engine, global ceiling = 2× the sum of per-engine budgets. Returns
15+
/// `None` only if the underlying `GovernorConfig` validation fails,
16+
/// matching the original inline behaviour.
17+
///
18+
/// Without a wired governor any test that asserts on balanced
19+
/// acquire/release after a workload trips on `governor.is_none()`
20+
/// instead of the real accounting bug — every test entry point that
21+
/// hands out a `SharedState` must install one.
22+
fn init_test_memory_governor() -> Option<Arc<nodedb_mem::MemoryGovernor>> {
23+
let test_budget: usize = 64 * 1024 * 1024; // 64 MiB / engine
24+
let mut engine_limits = std::collections::HashMap::new();
25+
for id in nodedb_mem::EngineId::ALL {
26+
engine_limits.insert(*id, test_budget);
27+
}
28+
let global_ceiling = test_budget * nodedb_mem::EngineId::ALL.len() * 2;
29+
nodedb_mem::MemoryGovernor::new(nodedb_mem::GovernorConfig {
30+
global_ceiling,
31+
engine_limits,
32+
})
33+
.ok()
34+
.map(Arc::new)
35+
}
36+
1337
/// Generate a short hex string suitable for unique test name suffixes.
1438
fn uuid_v4_hex() -> String {
1539
let id = uuid::Uuid::new_v4();
@@ -166,6 +190,7 @@ impl TestServer {
166190
// Deterministic 32-byte key — same value every test run.
167191
if let Some(s) = Arc::get_mut(&mut shared) {
168192
s.backup_kek = Some(Arc::new([0x42u8; 32]));
193+
s.governor = init_test_memory_governor();
169194
}
170195
let shared = shared;
171196

@@ -428,6 +453,7 @@ impl TestServer {
428453
SharedState::new_with_credentials(dispatcher, Arc::clone(&wal), credentials);
429454
if let Some(s) = Arc::get_mut(&mut shared) {
430455
s.backup_kek = Some(Arc::new([0x42u8; 32]));
456+
s.governor = init_test_memory_governor();
431457
}
432458
let shared = shared;
433459
nodedb::bootstrap::credentials::replay_surrogate_wal(&shared, &wal_records);
@@ -504,6 +530,14 @@ impl TestServer {
504530
.unwrap();
505531
}
506532

533+
// Re-register every persisted continuous aggregate on the local
534+
// Data Plane manager: the registry is per-core in-memory state
535+
// and is otherwise lost across restart.
536+
nodedb::control::server::pgwire::ddl::continuous_agg::register_persisted_continuous_aggregates(
537+
&shared,
538+
)
539+
.await;
540+
507541
let watermark_store =
508542
Arc::new(nodedb::event::watermark::WatermarkStore::open(dir_path).unwrap());
509543
let trigger_dlq = Arc::new(std::sync::Mutex::new(
@@ -613,6 +647,7 @@ impl TestServer {
613647
SharedState::new_with_credentials(dispatcher, Arc::clone(&wal), credentials);
614648
if let Some(s) = Arc::get_mut(&mut shared) {
615649
s.backup_kek = Some(Arc::new([0x42u8; 32]));
650+
s.governor = init_test_memory_governor();
616651
}
617652
let shared = shared;
618653

nodedb-test-support/src/tx_batch_helpers.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,7 @@ pub fn columnar_count(collection: &str) -> PhysicalPlan {
246246
sub_group_by: Vec::new(),
247247
sub_aggregates: Vec::new(),
248248
grouping_sets: Vec::new(),
249+
sort_keys: Vec::new(),
249250
})
250251
}
251252

nodedb/tests/catalog_integrity_verify.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,12 @@ fn classify(entry: &CatalogEntry) -> VariantClass {
140140
CatalogEntry::DeleteSchedule { .. } => VariantClass::ParentReplicated,
141141
CatalogEntry::DeleteChangeStream { .. } => VariantClass::ParentReplicated,
142142

143+
// Continuous aggregates carry a parent-replicated definition row
144+
// plus an owner row (apply path calls `put_parent_owner` /
145+
// `delete_parent_owner`).
146+
CatalogEntry::PutContinuousAggregate(_) => VariantClass::ParentReplicated,
147+
CatalogEntry::DeleteContinuousAggregate { .. } => VariantClass::ParentReplicated,
148+
143149
CatalogEntry::PutOwner(_) => VariantClass::Exempt,
144150
CatalogEntry::DeleteOwner { .. } => VariantClass::Exempt,
145151

nodedb/tests/engine_surface_timeseries.rs

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -176,3 +176,127 @@ async fn wal_restart_durability() {
176176
let expected = 3.14_f64;
177177
assert!((v - expected).abs() < 0.01);
178178
}
179+
180+
// ── Continuous-aggregate registration must survive past the in-memory Data ──
181+
//
182+
// `CREATE CONTINUOUS AGGREGATE` currently dispatches `RegisterContinuousAggregate`
183+
// straight to the Data Plane manager and returns success — no catalog row, no
184+
// target collection, no Raft replication. Every assertion below pins one
185+
// observable consequence of "no catalog persistence":
186+
//
187+
// 1. SHOW CONTINUOUS AGGREGATES must list the registration immediately.
188+
// 2. The aggregate must still be listed after a restart.
189+
// 3. The aggregate name must resolve as a queryable relation, not "unknown
190+
// table" — that is the whole point of materializing the result.
191+
//
192+
// All three are silent-failure spec assertions: if the CA registration ever
193+
// regresses back to "succeeds locally, vanishes elsewhere", a single test in
194+
// this group catches it.
195+
196+
/// SHOW CONTINUOUS AGGREGATES must list a freshly-created aggregate. The
197+
/// `CREATE` path returns success without registering anything observable.
198+
#[tokio::test]
199+
async fn continuous_aggregate_visible_in_show_after_create() {
200+
let srv = TestServer::start().await;
201+
srv.exec(
202+
"CREATE COLLECTION ts_cagg_show_src \
203+
COLUMNS (id TEXT, ts BIGINT TIME_KEY, value FLOAT) \
204+
WITH (engine='timeseries')",
205+
)
206+
.await
207+
.unwrap();
208+
srv.exec(
209+
"CREATE CONTINUOUS AGGREGATE ts_cagg_show_view \
210+
ON ts_cagg_show_src BUCKET '5m' \
211+
AGGREGATE SUM(value) AS total_value",
212+
)
213+
.await
214+
.unwrap();
215+
216+
let rows = srv.query_rows("SHOW CONTINUOUS AGGREGATES").await.unwrap();
217+
218+
assert!(
219+
rows.iter()
220+
.any(|r| r.first().map(String::as_str) == Some("ts_cagg_show_view")),
221+
"SHOW CONTINUOUS AGGREGATES must list the just-created aggregate; \
222+
got {rows:?}. Silent absence means the registration never reached \
223+
a durable location and only lives in transient Data Plane state."
224+
);
225+
}
226+
227+
/// A continuous aggregate created before a restart must still be present
228+
/// after one. The registration is meaningless if it disappears on every
229+
/// process restart — that is the catalog-persistence gap.
230+
#[tokio::test]
231+
async fn continuous_aggregate_survives_restart() {
232+
let srv = TestServer::start().await;
233+
srv.exec(
234+
"CREATE COLLECTION ts_cagg_persist_src \
235+
COLUMNS (id TEXT, ts BIGINT TIME_KEY, value FLOAT) \
236+
WITH (engine='timeseries')",
237+
)
238+
.await
239+
.unwrap();
240+
srv.exec(
241+
"CREATE CONTINUOUS AGGREGATE ts_cagg_persist_view \
242+
ON ts_cagg_persist_src BUCKET '5m' \
243+
AGGREGATE SUM(value) AS total_value",
244+
)
245+
.await
246+
.unwrap();
247+
248+
let (srv, dir) = srv.take_dir();
249+
srv.graceful_shutdown().await;
250+
251+
let (srv2, _dir) = TestServer::open_on_path(dir).await;
252+
let rows = srv2.query_rows("SHOW CONTINUOUS AGGREGATES").await.unwrap();
253+
254+
assert!(
255+
rows.iter()
256+
.any(|r| r.first().map(String::as_str) == Some("ts_cagg_persist_view")),
257+
"SHOW CONTINUOUS AGGREGATES must still list the aggregate after a \
258+
restart; got {rows:?}. Loss across restart is the catalog-persistence \
259+
gap: the registration is in-memory Data Plane state only."
260+
);
261+
}
262+
263+
/// The aggregate name must resolve as a queryable relation — `SELECT
264+
/// … FROM <ca_name>` must not error with "unknown table". The bench's
265+
/// reported symptom is precisely that: queries against the
266+
/// materialized name return `42P01` because no target collection ever
267+
/// existed. Data correctness of the rolled-up rows is a separate
268+
/// spec (it depends on a refresh path that doesn't ship yet); this
269+
/// test only pins the "name resolves" half of the bug.
270+
#[tokio::test]
271+
async fn continuous_aggregate_name_is_queryable_after_create() {
272+
let srv = TestServer::start().await;
273+
srv.exec(
274+
"CREATE COLLECTION ts_cagg_query_src \
275+
COLUMNS (id TEXT, ts BIGINT TIME_KEY, value FLOAT) \
276+
WITH (engine='timeseries')",
277+
)
278+
.await
279+
.unwrap();
280+
281+
srv.exec(
282+
"CREATE CONTINUOUS AGGREGATE ts_cagg_query_view \
283+
ON ts_cagg_query_src BUCKET '1m' \
284+
AGGREGATE SUM(value) AS total_value",
285+
)
286+
.await
287+
.unwrap();
288+
289+
let _rows = srv
290+
.query_rows("SELECT * FROM ts_cagg_query_view")
291+
.await
292+
.unwrap_or_else(|e| {
293+
panic!(
294+
"SELECT against the CA name must succeed once the aggregate \
295+
is registered (rolled-up data may be empty until a refresh \
296+
path is wired, but the name must resolve to a real relation); \
297+
got error: {e}. \"unknown table\" here is the bench's \
298+
reported symptom — no target collection was ever \
299+
materialized for the aggregate name."
300+
)
301+
});
302+
}

nodedb/tests/executor_tests/test_aggregate_aliases.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ fn aggregate_output_uses_user_alias_but_having_reads_canonical_key() {
7777
sub_group_by: Vec::new(),
7878
sub_aggregates: Vec::new(),
7979
grouping_sets: Vec::new(),
80+
sort_keys: Vec::new(),
8081
}),
8182
);
8283

nodedb/tests/executor_tests/test_array_ops.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,7 @@ fn array_agg_aggregate() {
216216
sub_group_by: Vec::new(),
217217
sub_aggregates: Vec::new(),
218218
grouping_sets: Vec::new(),
219+
sort_keys: Vec::new(),
219220
}),
220221
);
221222

nodedb/tests/executor_tests/test_columnar_aggregate.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ fn aggregate_count_reads_plain_columnar_engine_rows() {
5757
sub_group_by: Vec::new(),
5858
sub_aggregates: Vec::new(),
5959
grouping_sets: Vec::new(),
60+
sort_keys: Vec::new(),
6061
}),
6162
);
6263

@@ -122,6 +123,7 @@ fn columnar_having_uses_canonical_key_but_output_keeps_user_alias() {
122123
sub_group_by: Vec::new(),
123124
sub_aggregates: Vec::new(),
124125
grouping_sets: Vec::new(),
126+
sort_keys: Vec::new(),
125127
}),
126128
);
127129

@@ -245,6 +247,7 @@ fn aggregate_group_by_does_not_require_full_materialization() {
245247
sub_group_by: Vec::new(),
246248
sub_aggregates: Vec::new(),
247249
grouping_sets: Vec::new(),
250+
sort_keys: Vec::new(),
248251
}),
249252
);
250253

nodedb/tests/executor_tests/test_cross_type_join/cross_semi_joins.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@ fn cross_join_uses_inline_right_scalar_aggregate_for_post_filter() {
8585
sub_group_by: Vec::new(),
8686
sub_aggregates: Vec::new(),
8787
grouping_sets: Vec::new(),
88+
sort_keys: Vec::new(),
8889
}))),
8990
inline_left_bitmap: None,
9091
inline_right_bitmap: None,
@@ -181,6 +182,7 @@ fn cross_join_uses_unaliased_scalar_aggregate_key_for_post_filter() {
181182
sub_group_by: Vec::new(),
182183
sub_aggregates: Vec::new(),
183184
grouping_sets: Vec::new(),
185+
sort_keys: Vec::new(),
184186
}))),
185187
inline_left_bitmap: None,
186188
inline_right_bitmap: None,
@@ -313,6 +315,7 @@ fn semi_join_uses_nested_scalar_subquery_result_as_inline_right() {
313315
sub_group_by: Vec::new(),
314316
sub_aggregates: Vec::new(),
315317
grouping_sets: Vec::new(),
318+
sort_keys: Vec::new(),
316319
}))),
317320
inline_left_bitmap: None,
318321
inline_right_bitmap: None,

0 commit comments

Comments
 (0)