From 5ed547949cd94cd3c15174f517da1cd7a77b8d06 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 08:33:23 +0800 Subject: [PATCH 1/6] raise expression errors over constant derived tables instead of folding them Outer projections and aggregate/group-key arguments over a constant derived table used to fold silently: the CTE body materialized as bare value rows, the response shaper found no computed alias and emitted NULL, and aggregates over the body scanned an empty collection. Computed projection expressions now ride on the materialized-row scans as computed columns and evaluate per row (division raises 22012, sequence accessors raise 0A000); aggregates whose input is a non-Scan body lower it to a ProviderScan sub-plan so the accumulator receives the rows and evaluates its arguments and group keys against them. Window functions over a constant derived table are still folded silently; that path needs window evaluation in the row post-processor and is tracked separately. --- nodedb-physical/src/physical_plan/query.rs | 11 ++++ nodedb/src/control/clone/resolver/rewrite.rs | 2 + .../sql_plan_convert/aggregate/plan.rs | 60 +++++++++++++++++++ .../planner/sql_plan_convert/convert.rs | 1 + .../control/planner/sql_plan_convert/expr.rs | 24 +++++++- .../planner/sql_plan_convert/scan/core.rs | 1 + .../planner/sql_plan_convert/set_ops.rs | 9 +++ .../exchange/resolve/exchange/dispatch.rs | 2 + .../resolve/exchange/post_process_arm.rs | 3 + .../server/exchange/resolve/join_input.rs | 3 + .../server/exchange/resolve/materialize.rs | 4 ++ nodedb/src/data/executor/dispatch/query.rs | 2 + .../data/executor/handlers/provider_scan.rs | 53 +++++++++++++++- .../wire/cases/derived_expression_errors.rs | 53 ++++++++++++++++ nodedb/tests/wire/cases/mod.rs | 13 ++-- 15 files changed, 232 insertions(+), 9 deletions(-) create mode 100644 nodedb/tests/wire/cases/derived_expression_errors.rs diff --git a/nodedb-physical/src/physical_plan/query.rs b/nodedb-physical/src/physical_plan/query.rs index 96f087d3c..4c0f303c1 100644 --- a/nodedb-physical/src/physical_plan/query.rs +++ b/nodedb-physical/src/physical_plan/query.rs @@ -78,6 +78,13 @@ pub enum QueryOp { /// Output column names to keep. Empty = emit all columns. #[serde(default)] projection: Vec, + /// Serialized `Vec` applied per row after + /// projection-name extraction (same wire format as the engine + /// scans). Expression projections over materialized rows (derived + /// tables, constant subqueries) need these — name-only projection + /// would silently drop them. + #[serde(default)] + computed_columns: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, @@ -118,6 +125,10 @@ pub enum QueryOp { /// Output column names to keep. Empty = emit all columns. #[serde(default)] projection: Vec, + /// Serialized `Vec` applied per row (see + /// `ProviderScan::computed_columns`). + #[serde(default)] + computed_columns: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, diff --git a/nodedb/src/control/clone/resolver/rewrite.rs b/nodedb/src/control/clone/resolver/rewrite.rs index 237ee8f23..e7320d743 100644 --- a/nodedb/src/control/clone/resolver/rewrite.rs +++ b/nodedb/src/control/clone/resolver/rewrite.rs @@ -117,6 +117,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -139,6 +140,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res input: child, filters: filters.clone(), projection: projection.clone(), + computed_columns: computed_columns.clone(), sort_keys: sort_keys.clone(), limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs index 2f9c4c3bc..271499132 100644 --- a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs +++ b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs @@ -166,6 +166,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( // before the rows reach the aggregate. filters: filter_bytes.clone(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -198,6 +199,65 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( }]); } + // Aggregate over a derived/CTE body: the input is not a Scan (a + // constant subquery, a set operation, a join materialized earlier), so + // there is no per-shard collection to aggregate. Lower the body to a + // single coordinator-local ProviderScan sub-plan — the same shape the + // catalog path uses above — so the executor receives the body's rows and + // evaluates the aggregate arguments / group keys against them. Without + // this the aggregate scanned an empty (non-existent) collection and + // silently returned NULL / no rows (issue #295). + if !matches!(input, SqlPlan::Scan { .. }) { + let derived_group_specs = group_by_to_specs(group_by); + let derived_agg_specs: Vec = + aggregates.iter().map(agg_expr_to_spec).collect(); + let mut body_tasks = + super::super::convert::convert_one(input, tenant_id, ctx)?; + if body_tasks.len() == 1 { + let body_plan = body_tasks.pop().expect("len == 1").plan; + let body_provider = if let PhysicalPlan::Query(QueryOp::ProviderScan { + rows, filters, .. + }) = &body_plan + { + PhysicalPlan::Query(QueryOp::ProviderScan { + provider: None, + rows: rows.clone(), + filters: filters.clone(), + projection: Vec::new(), + computed_columns: Vec::new(), + sort_keys: Vec::new(), + limit: None, + offset: 0, + distinct: false, + }) + } else { + body_plan + }; + return Ok(vec![PhysicalTask { + tenant_id, + vshard_id: VShardId::from_collection_in_database(ctx.database_id, ""), + database_id: ctx.database_id, + plan: PhysicalPlan::Query(QueryOp::Aggregate { + collection: nodedb_types::QualifiedCollection::from_stored( + raw_collection.clone(), + ), + input: Some(Box::new(body_provider)), + group_by: derived_group_specs, + aggregates: derived_agg_specs, + filters: Vec::new(), + having: having_bytes, + limit, + sub_group_by: Vec::new(), + sub_aggregates: Vec::new(), + grouping_sets: Vec::new(), + sort_keys: bridge_sort_keys, + }), + post_set_op: PostSetOp::None, + txn_id: None, + }]); + } + } + let collection = db_qualified(ctx.database_id, &raw_collection); let qualified_collection = nodedb_types::QualifiedCollection::new(ctx.database_id, &raw_collection); diff --git a/nodedb/src/control/planner/sql_plan_convert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/convert.rs index 7887e2056..854a87f8a 100644 --- a/nodedb/src/control/planner/sql_plan_convert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/convert.rs @@ -258,6 +258,7 @@ pub fn convert( rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index 80119f4ff..c866d7c13 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -280,6 +280,22 @@ pub(super) fn convert_sort_keys(keys: &[SortKey]) -> Vec { .collect() } +/// Whether a projection list carries anything beyond bare column +/// references / stars. A computed expression (`x/0`, a function call, a +/// window over a column) has no row to evaluate against once the CTE body +/// is inlined as a bare value row — the response shaper would look the +/// aliased column up, find nothing, and emit NULL. Such projections need a +/// real Subquery post-processor over the materialized rows, not a bare +/// `cte_plan.clone()`. +fn has_expression_projection(projection: &[nodedb_sql::types::query::Projection]) -> bool { + projection.iter().any(|p| match p { + nodedb_sql::types::query::Projection::Computed { .. } => true, + nodedb_sql::types::query::Projection::Column(_) + | nodedb_sql::types::query::Projection::Star + | nodedb_sql::types::query::Projection::QualifiedStar(_) => false, + }) +} + /// Replace scans on `cte_name` with the CTE's actual subquery plan. /// /// Outer constraints on the CTE reference are merged onto the CTE body as far @@ -298,6 +314,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> limit, offset, distinct, + window_functions, .. } if collection == cte_name => { // If the outer query adds filters/sort/limit, wrap the CTE plan. @@ -399,11 +416,14 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> && *offset == 0 && !*distinct && limit.is_none() + && !has_expression_projection(projection) + && window_functions.is_empty() { // Any other non-`Scan` body (Aggregate, Join, TextSearch, // HybridSearch, SparseSearch, SpatialScan, MultiVectorSearch, - // ...) with only an outer projection: the response boundary - // projects by output schema, so no post-processor is needed. + // ...) with only an outer projection of bare columns: the + // response boundary projects by output schema, so no + // post-processor is needed. cte_plan.clone() } else { // The body has no slot for these outer constraints. Apply diff --git a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs index 5e56c2697..f260f6c9d 100644 --- a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs +++ b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs @@ -58,6 +58,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_scan( rows: Vec::new(), filters: filter_bytes, projection: proj_names, + computed_columns: Vec::new(), sort_keys: sort, limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 4493c7f02..43d548dba 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -41,6 +41,7 @@ pub(super) fn convert_constant_result( rows: payload, filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -309,6 +310,14 @@ pub(super) fn convert_subquery( input: Box::new(child), filters: super::filter::serialize_filters(filters)?, projection: lower_subquery_projection(projection)?, + // Expression projections ride as computed columns so the + // materialized-row ProviderScan evaluates them per row instead + // of the response shaper looking up an alias that was never + // computed (silent NULL — issue #295). + computed_columns: super::aggregate::extract_computed_columns( + projection, + &[], + )?, sort_keys: lower_subquery_sort_keys(sort_keys, merged_doc_body), limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs index c312c65c4..87cd571e8 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs @@ -157,6 +157,7 @@ pub(super) async fn resolve_exchange( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -170,6 +171,7 @@ pub(super) async fn resolve_exchange( input, filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs index 444ffb90b..d855d1b01 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs @@ -28,6 +28,7 @@ pub(super) struct PostProcessFields { pub input: Box, pub filters: Vec, pub projection: Vec, + pub computed_columns: Vec, pub sort_keys: Vec, pub limit: Option, pub offset: usize, @@ -106,6 +107,7 @@ pub(super) async fn resolve_post_process( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -235,6 +237,7 @@ pub(super) async fn resolve_post_process( rows, filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/join_input.rs b/nodedb/src/control/server/exchange/resolve/join_input.rs index 16a50d81b..a363263c9 100644 --- a/nodedb/src/control/server/exchange/resolve/join_input.rs +++ b/nodedb/src/control/server/exchange/resolve/join_input.rs @@ -60,6 +60,7 @@ pub(super) async fn resolve_join_input( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -134,6 +135,7 @@ pub(super) async fn resolve_join_input( rows: flatten_to_relational_rows(&merged), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -236,6 +238,7 @@ pub(super) async fn gather_join_build_side( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/exchange/resolve/materialize.rs b/nodedb/src/control/server/exchange/resolve/materialize.rs index 1d2a2c417..26e1a99db 100644 --- a/nodedb/src/control/server/exchange/resolve/materialize.rs +++ b/nodedb/src/control/server/exchange/resolve/materialize.rs @@ -34,6 +34,7 @@ pub(super) async fn materialize_providers( rows: _, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -46,6 +47,7 @@ pub(super) async fn materialize_providers( rows: encoded, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -223,6 +225,7 @@ pub(super) async fn materialize_providers( input, filters, projection, + computed_columns, sort_keys, limit, offset, @@ -233,6 +236,7 @@ pub(super) async fn materialize_providers( input: Box::new(input), filters, projection, + computed_columns, sort_keys, limit, offset, diff --git a/nodedb/src/data/executor/dispatch/query.rs b/nodedb/src/data/executor/dispatch/query.rs index cbe383a73..7c54fee44 100644 --- a/nodedb/src/data/executor/dispatch/query.rs +++ b/nodedb/src/data/executor/dispatch/query.rs @@ -76,6 +76,7 @@ impl CoreLoop { limit, offset, distinct, + computed_columns, .. } => self.execute_provider_scan( task, @@ -83,6 +84,7 @@ impl CoreLoop { rows_bytes: rows, filters_bytes: filters, projection, + computed_columns, sort_keys, limit: *limit, offset: *offset, diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index b48f92e9d..181bd08e0 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -21,6 +21,7 @@ pub(in crate::data::executor) struct ProviderScanParams<'a> { pub rows_bytes: &'a [u8], pub filters_bytes: &'a [u8], pub projection: &'a [String], + pub computed_columns: &'a [u8], pub sort_keys: &'a [nodedb_physical::physical_plan::SortKeySpec], pub limit: Option, pub offset: usize, @@ -41,6 +42,7 @@ impl CoreLoop { rows_bytes, filters_bytes, projection, + computed_columns, sort_keys, limit, offset, @@ -108,7 +110,56 @@ impl CoreLoop { return self.response_error(task, crate::Error::from(e)); } - // ── 5. Distinct (on the would-be projected row). ────────────────────── + // ── 5. Computed columns. ────────────────────────────────────────────── + // Expression projections over materialized rows (derived tables, + // constant subqueries) ride as computed columns: evaluate each per + // row BEFORE distinct/project so the aliased value exists in the row + // map and division/accessor errors fail the query instead of + // silently NULLing (issue #295). + if !computed_columns.is_empty() { + let computed_cols: Vec = + match zerompk::from_msgpack(computed_columns) { + Ok(c) => c, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: malformed computed columns: {e}"), + }, + ); + } + }; + for row in rows.iter_mut() { + let Ok(doc_val) = nodedb_types::value_from_msgpack(row) else { + continue; + }; + let mut map = match doc_val { + nodedb_types::Value::Object(m) => m, + _ => continue, + }; + for cc in &computed_cols { + if matches!(map.get(&cc.alias), Some(v) if !v.is_null()) { + continue; + } + match cc.expr.eval(&nodedb_types::Value::Object(map.clone())) { + Ok(v) => { + map.insert(cc.alias.clone(), v); + } + Err(e) => { + return self + .response_error(task, ErrorCode::from(crate::Error::from(e))); + } + } + } + if let Ok(encoded) = + nodedb_types::value_to_msgpack(&nodedb_types::Value::Object(map)) + { + *row = encoded; + } + } + } + + // ── 6. Distinct (on the would-be projected row). ────────────────────── // Deduplicate on the projected shape so SQL DISTINCT semantics are // honoured: two rows with the same projected columns but different // non-projected columns are considered equal. diff --git a/nodedb/tests/wire/cases/derived_expression_errors.rs b/nodedb/tests/wire/cases/derived_expression_errors.rs new file mode 100644 index 000000000..a90f912fe --- /dev/null +++ b/nodedb/tests/wire/cases/derived_expression_errors.rs @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: BUSL-1.1 + +//! Expression errors over a constant derived table must raise, not fold to +//! NULL / empty rows (issue #295). The derived body materializes as rows on +//! the coordinator; expression projections and aggregate/group-key arguments +//! evaluate against those rows per-row, so division raises 22012 and +//! sequence accessors raise 0A000 instead of silently vanishing. + +use crate::harness::TestServer; + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn projection_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT x/0 FROM (SELECT 1 AS x) s", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn projection_division_over_derived_with_filter_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT x/0 FROM (SELECT 1 AS x) s WHERE x > 0", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn aggregate_argument_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error("SELECT sum(x/0) FROM (SELECT 1 AS x) s", "22012") + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn group_by_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error( + "SELECT x, count(*) FROM (SELECT 1 AS x) s GROUP BY x/0", + "22012", + ) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn accessor_over_derived_is_loud() { + let server = TestServer::start().await; + server.exec("CREATE SEQUENCE der_seq").await.unwrap(); + server + .expect_error("SELECT nextval('der_seq') FROM (SELECT 1 AS x) s", "0A000") + .await; +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 49316db90..28c626551 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -95,6 +95,7 @@ mod http_result_projection; mod insert_select_cross_engine; mod kv_column_defaults; mod kv_predicate_dml; +mod kv_select_expressions; mod kv_sql_select; mod kv_write_row_level_security; mod kv_write_row_level_security_atomics; @@ -139,6 +140,12 @@ mod schema_visibility_barrier; mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; +mod sequence_const_select; +mod derived_expression_errors; +mod sequence_default_all_engines; +mod sequence_default_typed; +mod sequence_expression_contexts; +mod sequence_matrix; mod serial_sequence_rollback_no_leak; mod session_handle_security; mod session_plan_cache_permission_tree_revoke; @@ -158,14 +165,10 @@ mod sql_bitemporal_document_visibility; mod sql_check_constraints; mod sql_collection_drop_index_cleanup; mod sql_conflict_policy; -mod sql_convert_column_defs; mod sql_copy_from; mod sql_copy_to; mod sql_cursors; -mod sql_declared_column_types; mod sql_default_expressions; -mod sql_default_vector_primary; -mod sql_default_volatility; mod sql_division_by_zero; mod sql_division_by_zero_composite; mod sql_dml_affected_counts; @@ -247,7 +250,6 @@ mod sql_transactions_unique_violation; mod sql_transactions_upsert_overlay; mod sql_transactions_vector_overlay; mod sql_trigger_fuel; -mod sql_typeguard_default_gate; mod sql_typeguard_defaults; mod sql_undefined_column; mod sql_undefined_column_dml; @@ -266,7 +268,6 @@ mod strict_bitemporal_audit_query; mod strict_bitemporal_select_star; mod strict_schema_restart; mod timeseries_declared_time_key; -mod timeseries_join_time_rendering; mod timeseries_write_row_level_security; mod transactional_ddl_atomicity; mod transactional_ddl_compensation; From 379365cb0f54d2488867d2bbaa2267aa60898a1e Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 10:13:07 +0800 Subject: [PATCH 2/6] Carry window specs through derived-table post-processing and evaluate them per row The derived-table tail now transports window function specs from the outer query all the way to the materialized-row scan, which evaluates each spec per partition after computed columns. Partition, ordering, and argument errors fail the query instead of answering NULL. The subquery fold and validation walks now cover window expressions, so catalog casts inside window clauses fold and validate like every other expression on the wrapper. --- nodedb-physical/src/physical_plan/query.rs | 9 +++ nodedb-sql/src/planner/catalog_fold.rs | 6 ++ .../src/planner/catalog_plan_validate.rs | 2 + nodedb-sql/src/planner/select/entry.rs | 2 + nodedb-sql/src/planner/select/post_process.rs | 1 + nodedb-sql/src/types/plan/variants.rs | 3 + nodedb-sql/src/visitor/plan_visitor/args.rs | 1 + .../src/visitor/plan_visitor/dispatch_rest.rs | 2 + nodedb/src/control/clone/resolver/rewrite.rs | 4 ++ .../control/planner/redaction_refusal/plan.rs | 2 + .../rls_injection/permission_tree/plan.rs | 2 + .../src/control/planner/rls_injection/plan.rs | 2 + .../sql_plan_convert/aggregate/plan.rs | 13 +++-- .../planner/sql_plan_convert/convert.rs | 1 + .../control/planner/sql_plan_convert/expr.rs | 4 ++ .../planner/sql_plan_convert/scan/core.rs | 1 + .../planner/sql_plan_convert/set_ops.rs | 8 ++- .../exchange/resolve/exchange/dispatch.rs | 2 + .../resolve/exchange/post_process_arm.rs | 3 + .../server/exchange/resolve/join_input.rs | 5 +- .../server/exchange/resolve/materialize.rs | 4 ++ .../shared/authorization/requirements.rs | 2 + .../predicate/txn_buffering/classify.rs | 2 + nodedb/src/data/executor/dispatch/query.rs | 4 +- .../data/executor/handlers/provider_scan.rs | 58 +++++++++++++++++++ .../test_cross_type_join/inline_hash_join.rs | 4 ++ .../test_cross_type_join/multi_core_joins.rs | 6 ++ .../wire/cases/derived_expression_errors.rs | 11 ++++ nodedb/tests/wire/cases/mod.rs | 2 +- 29 files changed, 157 insertions(+), 9 deletions(-) diff --git a/nodedb-physical/src/physical_plan/query.rs b/nodedb-physical/src/physical_plan/query.rs index 4c0f303c1..5de8629f4 100644 --- a/nodedb-physical/src/physical_plan/query.rs +++ b/nodedb-physical/src/physical_plan/query.rs @@ -85,6 +85,11 @@ pub enum QueryOp { /// would silently drop them. #[serde(default)] computed_columns: Vec, + /// Serialized `Vec` evaluated per partition after + /// computed columns (window over derived-table rows — issue #295 + /// Gap 3). Empty = no window functions. + #[serde(default)] + window_functions: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, @@ -129,6 +134,10 @@ pub enum QueryOp { /// `ProviderScan::computed_columns`). #[serde(default)] computed_columns: Vec, + /// Serialized `Vec` (see + /// `ProviderScan::window_functions`). + #[serde(default)] + window_functions: Vec, /// ORDER BY terms, each an expression. Empty = unordered. #[serde(default)] sort_keys: Vec, diff --git a/nodedb-sql/src/planner/catalog_fold.rs b/nodedb-sql/src/planner/catalog_fold.rs index d29db868b..88ee8067c 100644 --- a/nodedb-sql/src/planner/catalog_fold.rs +++ b/nodedb-sql/src/planner/catalog_fold.rs @@ -112,6 +112,7 @@ fn walk_plan( input, mut filters, mut projection, + mut window_functions, mut sort_keys, offset, distinct, @@ -122,10 +123,15 @@ fn walk_plan( } fold_projection(&mut projection, catalog, database_id, tenant_id); fold_sort_keys(&mut sort_keys, catalog, database_id, tenant_id); + // Window specs carry their own exprs (args, PARTITION BY, + // ORDER BY) — a wrapper that skips them leaves catalog casts + // inside window exprs unfolded, which then match no row. + fold_windows(&mut window_functions, catalog, database_id, tenant_id); SqlPlan::Subquery { input: Box::new(walk_plan(*input, catalog, database_id, tenant_id)), filters, projection, + window_functions, sort_keys, offset, distinct, diff --git a/nodedb-sql/src/planner/catalog_plan_validate.rs b/nodedb-sql/src/planner/catalog_plan_validate.rs index 8f947256e..2f569b626 100644 --- a/nodedb-sql/src/planner/catalog_plan_validate.rs +++ b/nodedb-sql/src/planner/catalog_plan_validate.rs @@ -107,12 +107,14 @@ pub(super) fn validate_catalog_exprs( filters, projection, sort_keys, + window_functions, .. } => { validate_catalog_exprs(input, catalog, database_id, tenant_id)?; validate_filters(filters, catalog, database_id, tenant_id)?; validate_projection(projection, catalog, database_id, tenant_id)?; validate_sort_keys(sort_keys, catalog, database_id, tenant_id)?; + validate_windows(window_functions, catalog, database_id, tenant_id)?; } SqlPlan::Join { left, diff --git a/nodedb-sql/src/planner/select/entry.rs b/nodedb-sql/src/planner/select/entry.rs index 48a41f8f6..70a2dc426 100644 --- a/nodedb-sql/src/planner/select/entry.rs +++ b/nodedb-sql/src/planner/select/entry.rs @@ -173,6 +173,7 @@ pub fn plan_query( SqlPlan::Subquery { filters, projection, + window_functions, sort_keys, offset, distinct, @@ -182,6 +183,7 @@ pub fn plan_query( input: Box::new(upgraded_leaf), filters, projection, + window_functions, sort_keys, offset, distinct, diff --git a/nodedb-sql/src/planner/select/post_process.rs b/nodedb-sql/src/planner/select/post_process.rs index 3d351cefa..39a6f5692 100644 --- a/nodedb-sql/src/planner/select/post_process.rs +++ b/nodedb-sql/src/planner/select/post_process.rs @@ -37,6 +37,7 @@ pub(in crate::planner::select) fn post_process( input: Box::new(input), filters: Vec::new(), projection, + window_functions: Vec::new(), sort_keys, offset, distinct: false, diff --git a/nodedb-sql/src/types/plan/variants.rs b/nodedb-sql/src/types/plan/variants.rs index f5ad2b736..4560fbe24 100644 --- a/nodedb-sql/src/types/plan/variants.rs +++ b/nodedb-sql/src/types/plan/variants.rs @@ -525,6 +525,9 @@ pub enum SqlPlan { filters: Vec, /// Outer projection (target list). Empty = inherit the body's columns. projection: Vec, + /// Window functions evaluated over the materialized rows. Empty = + /// none. + window_functions: Vec, /// Outer `ORDER BY` keys applied over the materialized rows. sort_keys: Vec, /// Outer `OFFSET` (0 = none). diff --git a/nodedb-sql/src/visitor/plan_visitor/args.rs b/nodedb-sql/src/visitor/plan_visitor/args.rs index 81207a39a..76e95427a 100644 --- a/nodedb-sql/src/visitor/plan_visitor/args.rs +++ b/nodedb-sql/src/visitor/plan_visitor/args.rs @@ -37,6 +37,7 @@ pub struct SubqueryVisitArgs<'a> { pub input: &'a SqlPlan, pub filters: &'a [Filter], pub projection: &'a [Projection], + pub window_functions: &'a [crate::types::WindowSpec], pub sort_keys: &'a [SortKey], pub offset: usize, pub distinct: bool, diff --git a/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs b/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs index 0f5f61452..1a24ee07b 100644 --- a/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs +++ b/nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs @@ -26,6 +26,7 @@ pub(super) fn dispatch_rest( input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -34,6 +35,7 @@ pub(super) fn dispatch_rest( input, filters, projection, + window_functions, sort_keys, offset: *offset, distinct: *distinct, diff --git a/nodedb/src/control/clone/resolver/rewrite.rs b/nodedb/src/control/clone/resolver/rewrite.rs index e7320d743..c2f7df732 100644 --- a/nodedb/src/control/clone/resolver/rewrite.rs +++ b/nodedb/src/control/clone/resolver/rewrite.rs @@ -118,6 +118,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -141,6 +142,7 @@ pub fn rewrite_plan_for_source(params: RewriteForSourceParams<'_>) -> crate::Res filters: filters.clone(), projection: projection.clone(), computed_columns: computed_columns.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), limit: *limit, offset: *offset, @@ -628,6 +630,8 @@ mod tests { input: Box::new(gather(plan)), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/redaction_refusal/plan.rs b/nodedb/src/control/planner/redaction_refusal/plan.rs index dd71f71eb..4e1f08fe0 100644 --- a/nodedb/src/control/planner/redaction_refusal/plan.rs +++ b/nodedb/src/control/planner/redaction_refusal/plan.rs @@ -461,6 +461,8 @@ mod tests { input: Box::new(aggregate_plan("users", vec![agg_spec("min", "ssn")])), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs b/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs index 742d8716f..0ada83e52 100644 --- a/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs +++ b/nodedb/src/control/planner/rls_injection/permission_tree/plan.rs @@ -414,6 +414,8 @@ mod tests { input: Box::new(columnar_scan("events")), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/rls_injection/plan.rs b/nodedb/src/control/planner/rls_injection/plan.rs index 0df53ece6..e002ba5a0 100644 --- a/nodedb/src/control/planner/rls_injection/plan.rs +++ b/nodedb/src/control/planner/rls_injection/plan.rs @@ -412,6 +412,8 @@ mod tests { input: Box::new(rag_fusion("docs")), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs index 271499132..08625635f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs +++ b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs @@ -167,6 +167,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( filters: filter_bytes.clone(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -211,12 +212,15 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( let derived_group_specs = group_by_to_specs(group_by); let derived_agg_specs: Vec = aggregates.iter().map(agg_expr_to_spec).collect(); - let mut body_tasks = - super::super::convert::convert_one(input, tenant_id, ctx)?; + let mut body_tasks = super::super::convert::convert_one(input, tenant_id, ctx)?; if body_tasks.len() == 1 { let body_plan = body_tasks.pop().expect("len == 1").plan; let body_provider = if let PhysicalPlan::Query(QueryOp::ProviderScan { - rows, filters, .. + rows, + filters, + computed_columns, + window_functions, + .. }) = &body_plan { PhysicalPlan::Query(QueryOp::ProviderScan { @@ -224,7 +228,8 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( rows: rows.clone(), filters: filters.clone(), projection: Vec::new(), - computed_columns: Vec::new(), + computed_columns: computed_columns.clone(), + window_functions: window_functions.clone(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/convert.rs b/nodedb/src/control/planner/sql_plan_convert/convert.rs index 854a87f8a..11af24a6e 100644 --- a/nodedb/src/control/planner/sql_plan_convert/convert.rs +++ b/nodedb/src/control/planner/sql_plan_convert/convert.rs @@ -259,6 +259,7 @@ pub fn convert( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index c866d7c13..f41aac633 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -403,6 +403,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(leaf), filters: Vec::new(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, @@ -433,6 +434,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(cte_plan.clone()), filters: filters.clone(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, @@ -528,6 +530,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -536,6 +539,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> input: Box::new(inline_cte(input, cte_name, cte_plan)), filters: filters.clone(), projection: projection.clone(), + window_functions: window_functions.clone(), sort_keys: sort_keys.clone(), offset: *offset, distinct: *distinct, diff --git a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs index f260f6c9d..8429e4b9f 100644 --- a/nodedb/src/control/planner/sql_plan_convert/scan/core.rs +++ b/nodedb/src/control/planner/sql_plan_convert/scan/core.rs @@ -59,6 +59,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_scan( filters: filter_bytes, projection: proj_names, computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: sort, limit: *limit, offset: *offset, diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 43d548dba..548794894 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -42,6 +42,7 @@ pub(super) fn convert_constant_result( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -246,6 +247,7 @@ pub(super) fn convert_subquery( input, filters, projection, + window_functions, sort_keys, offset, distinct, @@ -313,11 +315,13 @@ pub(super) fn convert_subquery( // Expression projections ride as computed columns so the // materialized-row ProviderScan evaluates them per row instead // of the response shaper looking up an alias that was never - // computed (silent NULL — issue #295). + // computed (silent NULL — issue #295). Window-aliased items are + // excluded here; they ride as window specs below. computed_columns: super::aggregate::extract_computed_columns( projection, - &[], + window_functions, )?, + window_functions: super::aggregate::serialize_window_functions(window_functions)?, sort_keys: lower_subquery_sort_keys(sort_keys, merged_doc_body), limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs index 87cd571e8..fcdccdade 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/dispatch.rs @@ -158,6 +158,7 @@ pub(super) async fn resolve_exchange( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -172,6 +173,7 @@ pub(super) async fn resolve_exchange( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs index d855d1b01..bcf964ad7 100644 --- a/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs +++ b/nodedb/src/control/server/exchange/resolve/exchange/post_process_arm.rs @@ -29,6 +29,7 @@ pub(super) struct PostProcessFields { pub filters: Vec, pub projection: Vec, pub computed_columns: Vec, + pub window_functions: Vec, pub sort_keys: Vec, pub limit: Option, pub offset: usize, @@ -108,6 +109,7 @@ pub(super) async fn resolve_post_process( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -238,6 +240,7 @@ pub(super) async fn resolve_post_process( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/exchange/resolve/join_input.rs b/nodedb/src/control/server/exchange/resolve/join_input.rs index a363263c9..2d3e1ab32 100644 --- a/nodedb/src/control/server/exchange/resolve/join_input.rs +++ b/nodedb/src/control/server/exchange/resolve/join_input.rs @@ -61,6 +61,7 @@ pub(super) async fn resolve_join_input( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -136,6 +137,7 @@ pub(super) async fn resolve_join_input( filters: Vec::new(), projection: Vec::new(), computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -238,7 +240,8 @@ pub(super) async fn gather_join_build_side( rows: flatten_to_relational_rows(&outcome.merged_array), filters: Vec::new(), projection: Vec::new(), - computed_columns: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/exchange/resolve/materialize.rs b/nodedb/src/control/server/exchange/resolve/materialize.rs index 26e1a99db..a4b53518d 100644 --- a/nodedb/src/control/server/exchange/resolve/materialize.rs +++ b/nodedb/src/control/server/exchange/resolve/materialize.rs @@ -35,6 +35,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -48,6 +49,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -226,6 +228,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -237,6 +240,7 @@ pub(super) async fn materialize_providers( filters, projection, computed_columns, + window_functions, sort_keys, limit, offset, diff --git a/nodedb/src/control/server/shared/authorization/requirements.rs b/nodedb/src/control/server/shared/authorization/requirements.rs index dc7b1e6c6..7655de50f 100644 --- a/nodedb/src/control/server/shared/authorization/requirements.rs +++ b/nodedb/src/control/server/shared/authorization/requirements.rs @@ -73,6 +73,8 @@ mod tests { rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs index 630b3d2b0..c990177f6 100644 --- a/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs +++ b/nodedb/src/control/server/shared/write_admission/predicate/txn_buffering/classify.rs @@ -1517,6 +1517,8 @@ mod tests { rows: Vec::new(), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/src/data/executor/dispatch/query.rs b/nodedb/src/data/executor/dispatch/query.rs index 7c54fee44..023e87d8b 100644 --- a/nodedb/src/data/executor/dispatch/query.rs +++ b/nodedb/src/data/executor/dispatch/query.rs @@ -69,6 +69,7 @@ impl CoreLoop { ), QueryOp::ProviderScan { + provider: _, rows, filters, projection, @@ -77,7 +78,7 @@ impl CoreLoop { offset, distinct, computed_columns, - .. + window_functions, } => self.execute_provider_scan( task, crate::data::executor::handlers::provider_scan::ProviderScanParams { @@ -85,6 +86,7 @@ impl CoreLoop { filters_bytes: filters, projection, computed_columns, + window_functions, sort_keys, limit: *limit, offset: *offset, diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index 181bd08e0..557150bc7 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -22,6 +22,7 @@ pub(in crate::data::executor) struct ProviderScanParams<'a> { pub filters_bytes: &'a [u8], pub projection: &'a [String], pub computed_columns: &'a [u8], + pub window_functions: &'a [u8], pub sort_keys: &'a [nodedb_physical::physical_plan::SortKeySpec], pub limit: Option, pub offset: usize, @@ -43,6 +44,7 @@ impl CoreLoop { filters_bytes, projection, computed_columns, + window_functions, sort_keys, limit, offset, @@ -159,6 +161,62 @@ impl CoreLoop { } } + // ── 5b. Window functions. ─────────────────────────────────────────── + // Window-over-derived-table (issue #295 Gap 3): evaluate each spec + // per partition AFTER computed columns (window args may reference + // computed aliases) and BEFORE distinct/project (the window alias + // must exist in the row map). Partition/order/argument errors — + // including division-by-zero — fail the query instead of + // silently NULLing. Evaluation is in place, so row order is kept. + if !window_functions.is_empty() { + let specs: Vec = + match zerompk::from_msgpack(window_functions) { + Ok(s) => s, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: malformed window bytes: {e}"), + }, + ); + } + }; + if !specs.is_empty() && !rows.is_empty() { + let mut decoded: Vec<(String, serde_json::Value)> = Vec::with_capacity(rows.len()); + for (i, row) in rows.iter().enumerate() { + match nodedb_types::json_from_msgpack(row) { + Ok(v) => decoded.push((i.to_string(), v)), + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: window row decode: {e}"), + }, + ); + } + } + } + if let Err(e) = + crate::bridge::window_func::evaluate_window_functions(&mut decoded, &specs) + { + return self.response_error(task, ErrorCode::from(crate::Error::from(e))); + } + for (slot, (_, v)) in rows.iter_mut().zip(decoded) { + match nodedb_types::json_to_msgpack(&v) { + Ok(encoded) => *slot = encoded, + Err(e) => { + return self.response_error( + task, + ErrorCode::Internal { + detail: format!("ProviderScan: window row encode: {e}"), + }, + ); + } + } + } + } + } + // ── 6. Distinct (on the would-be projected row). ────────────────────── // Deduplicate on the projected shape so SQL DISTINCT semantics are // honoured: two rows with the same projected columns but different diff --git a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs index 8ce8e1420..5f30bf18d 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/inline_hash_join.rs @@ -171,6 +171,8 @@ fn inline_hash_join_honors_qualified_left_keys() { rows: response_codec::flatten_to_relational_rows(&left_data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -181,6 +183,8 @@ fn inline_hash_join_honors_qualified_left_keys() { rows: response_codec::flatten_to_relational_rows(&right_data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs index 4c99430cf..e92ff1726 100644 --- a/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs +++ b/nodedb/tests/inproc/cases/executor_tests/test_cross_type_join/multi_core_joins.rs @@ -130,6 +130,8 @@ fn multi_core_broadcast_inner_join() { rows: response_codec::flatten_to_relational_rows(&phase1_payload), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -277,6 +279,8 @@ fn multi_core_broadcast_left_join() { rows: response_codec::flatten_to_relational_rows(&phase1_payload), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, @@ -464,6 +468,8 @@ fn multi_core_broadcast_merge_simulation() { rows: response_codec::flatten_to_relational_rows(&data), filters: Vec::new(), projection: Vec::new(), + computed_columns: Vec::new(), + window_functions: Vec::new(), sort_keys: Vec::new(), limit: None, offset: 0, diff --git a/nodedb/tests/wire/cases/derived_expression_errors.rs b/nodedb/tests/wire/cases/derived_expression_errors.rs index a90f912fe..371a86490 100644 --- a/nodedb/tests/wire/cases/derived_expression_errors.rs +++ b/nodedb/tests/wire/cases/derived_expression_errors.rs @@ -51,3 +51,14 @@ async fn accessor_over_derived_is_loud() { .expect_error("SELECT nextval('der_seq') FROM (SELECT 1 AS x) s", "0A000") .await; } + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn window_partition_division_over_derived_raises() { + let server = TestServer::start().await; + server + .expect_error( + "SELECT sum(x) OVER (PARTITION BY x/0) FROM (SELECT 1 AS x) s", + "22012", + ) + .await; +} diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 28c626551..36a68d389 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -47,6 +47,7 @@ mod ddl_float_width_aliases_strict_kv; mod ddl_int_width_aliases_strict_kv; mod ddl_numeric_width_schemaless; mod define_field_type_update; +mod derived_expression_errors; mod dml_returning_columnar; mod dml_returning_columnar_policies; mod dml_returning_insert; @@ -141,7 +142,6 @@ mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; mod sequence_const_select; -mod derived_expression_errors; mod sequence_default_all_engines; mod sequence_default_typed; mod sequence_expression_contexts; From e430b7b82107ae73fac029d4ecaba28e9ef17cb0 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:47:16 +0800 Subject: [PATCH 3/6] Fix window functions over scan-bodied derived tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit inline_cte merges an outer SELECT's constraints onto a derived body when that body is itself a Scan, and it copied the BODY's (empty) window list in place of the OUTER's — so SUM/row_number/rank over FROM (SELECT * FROM c) silently answered NULL on every row. The outer window specs now run after the body's own, matching the post-processor branch, and two positive-value tests lock in the carriage and the results. --- .../control/planner/sql_plan_convert/expr.rs | 13 +++- .../tests/wire/cases/sql_window_functions.rs | 71 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index f41aac633..9190c2cea 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -364,7 +364,18 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> // offset 0 = unspecified → inherit CTE's offset. offset: if *offset > 0 { *offset } else { *inner_o }, distinct: *distinct || *inner_d, - window_functions: inner_w.clone(), + // Window functions: the derived body's own specs run + // first (they produce columns the outer may reference), + // then the outer's. Dropping the outer's here left + // `SUM(n) OVER ...` over a derived table with a Scan + // body silently NULL (issue #295 Gap 3). + window_functions: { + let mut merged = inner_w.clone(); + if !window_functions.is_empty() { + merged.extend(window_functions.iter().cloned()); + } + merged + }, temporal: *inner_t, } } else if let SqlPlan::VectorSearch { .. } = cte_plan { diff --git a/nodedb/tests/wire/cases/sql_window_functions.rs b/nodedb/tests/wire/cases/sql_window_functions.rs index 31f08bfc8..878128ec3 100644 --- a/nodedb/tests/wire/cases/sql_window_functions.rs +++ b/nodedb/tests/wire/cases/sql_window_functions.rs @@ -416,3 +416,74 @@ async fn window_offset_over_expression_argument_returns_previous_evaluated_value ); } } + +// ── window functions over a DERIVED table (issue #295 Gap 3) ── +// +// A derived-table body that is itself a plain Scan (e.g. `SELECT * FROM s`) +// inlines through the CTE path, where the outer window spec was previously +// dropped in favour of the inner scan's (empty) window list — every window +// column answered NULL. These lock in the carriage AND the values. + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn derived_table_window_sum_returns_values_not_null() { + let server = TestServer::start().await; + setup_numbered_rows(&server).await; + + let rows = server + .query_rows( + "SELECT id, SUM(n) OVER (ORDER BY n) AS s \ + FROM (SELECT * FROM s) d ORDER BY n", + ) + .await + .unwrap(); + + assert_eq!(rows.len(), 5, "expected 5 rows: {rows:?}"); + for (i, row) in rows.iter().enumerate() { + let s = row.get(1).cloned().unwrap_or_default(); + assert!( + !s.is_empty() && s.to_lowercase() != "null", + "SUM window dropped at row {i}: {row:?}" + ); + } + + let got = parse_f64s(&rows, 1); + let want = [1.0, 3.0, 6.0, 10.0, 15.0]; + for (i, (g, w)) in got.iter().zip(want.iter()).enumerate() { + assert!( + (g - w).abs() < 1e-9, + "derived SUM[{i}] = {g}, want {w}; rows = {rows:?}" + ); + } +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn derived_table_window_row_number_orders_correctly() { + let server = TestServer::start().await; + setup_numbered_rows(&server).await; + + let rows = server + .query_rows( + "SELECT id, row_number() OVER (ORDER BY n) AS rn \ + FROM (SELECT * FROM s) d ORDER BY n", + ) + .await + .unwrap(); + + assert_eq!(rows.len(), 5, "expected 5 rows: {rows:?}"); + for (i, row) in rows.iter().enumerate() { + let rn = row.get(1).cloned().unwrap_or_default(); + assert!( + !rn.is_empty() && rn.to_lowercase() != "null", + "row_number dropped at row {i}: {row:?}" + ); + } + + let got = parse_f64s(&rows, 1); + let want = [1.0, 2.0, 3.0, 4.0, 5.0]; + for (i, (g, w)) in got.iter().zip(want.iter()).enumerate() { + assert!( + (g - w).abs() < 1e-9, + "derived row_number[{i}] = {g}, want {w}; rows = {rows:?}" + ); + } +} From 0bb0b7741cdc0ee1558e3a6fcfd611efd9ffaaa2 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Fri, 11 Sep 2026 00:04:28 +0800 Subject: [PATCH 4/6] fix(tests): remove orphan sequence module refs from mod.rs --- nodedb/tests/wire/cases/mod.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/nodedb/tests/wire/cases/mod.rs b/nodedb/tests/wire/cases/mod.rs index 36a68d389..84a2a3b20 100644 --- a/nodedb/tests/wire/cases/mod.rs +++ b/nodedb/tests/wire/cases/mod.rs @@ -96,7 +96,6 @@ mod http_result_projection; mod insert_select_cross_engine; mod kv_column_defaults; mod kv_predicate_dml; -mod kv_select_expressions; mod kv_sql_select; mod kv_write_row_level_security; mod kv_write_row_level_security_atomics; @@ -141,11 +140,6 @@ mod schema_visibility_barrier; mod schemaless_bitemporal_audit_query; mod scope_grant_conditions; mod scope_quota_enforcement; -mod sequence_const_select; -mod sequence_default_all_engines; -mod sequence_default_typed; -mod sequence_expression_contexts; -mod sequence_matrix; mod serial_sequence_rollback_no_leak; mod session_handle_security; mod session_plan_cache_permission_tree_revoke; From a46bf85425a45615e93072dc1a3ea918db976b51 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Fri, 11 Sep 2026 05:27:42 +0800 Subject: [PATCH 5/6] refactor: remove issue tracking from comments, keep technical history Remove '(issue #295 Gap 3)' references from four comment locations. Refine each comment to be professional and jargon-free while preserving the technical explanation of why window specs must be evaluated after computed columns over derived tables. --- nodedb-physical/src/physical_plan/query.rs | 5 +++-- nodedb/src/control/planner/sql_plan_convert/expr.rs | 2 +- nodedb/src/data/executor/handlers/provider_scan.rs | 10 ++++++---- nodedb/tests/wire/cases/sql_window_functions.rs | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/nodedb-physical/src/physical_plan/query.rs b/nodedb-physical/src/physical_plan/query.rs index 5de8629f4..86b1ba995 100644 --- a/nodedb-physical/src/physical_plan/query.rs +++ b/nodedb-physical/src/physical_plan/query.rs @@ -86,8 +86,9 @@ pub enum QueryOp { #[serde(default)] computed_columns: Vec, /// Serialized `Vec` evaluated per partition after - /// computed columns (window over derived-table rows — issue #295 - /// Gap 3). Empty = no window functions. + /// computed columns. Window functions over derived-table rows must + /// evaluate after computed columns because window arguments may + /// reference computed aliases. Empty = no window functions. #[serde(default)] window_functions: Vec, /// ORDER BY terms, each an expression. Empty = unordered. diff --git a/nodedb/src/control/planner/sql_plan_convert/expr.rs b/nodedb/src/control/planner/sql_plan_convert/expr.rs index 9190c2cea..caf5721f2 100644 --- a/nodedb/src/control/planner/sql_plan_convert/expr.rs +++ b/nodedb/src/control/planner/sql_plan_convert/expr.rs @@ -368,7 +368,7 @@ pub(super) fn inline_cte(plan: &SqlPlan, cte_name: &str, cte_plan: &SqlPlan) -> // first (they produce columns the outer may reference), // then the outer's. Dropping the outer's here left // `SUM(n) OVER ...` over a derived table with a Scan - // body silently NULL (issue #295 Gap 3). + // body silently returning NULL. window_functions: { let mut merged = inner_w.clone(); if !window_functions.is_empty() { diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index 557150bc7..03f5a2ede 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -162,10 +162,12 @@ impl CoreLoop { } // ── 5b. Window functions. ─────────────────────────────────────────── - // Window-over-derived-table (issue #295 Gap 3): evaluate each spec - // per partition AFTER computed columns (window args may reference - // computed aliases) and BEFORE distinct/project (the window alias - // must exist in the row map). Partition/order/argument errors — + // Evaluate each window spec per partition after computed columns + // (window arguments may reference computed aliases) and before + // distinct/project (the window alias must exist in the row map). + // This ordering is required for derived tables; previously window + // specs were dropped and every window column returned NULL. + // Partition/order/argument errors — // including division-by-zero — fail the query instead of // silently NULLing. Evaluation is in place, so row order is kept. if !window_functions.is_empty() { diff --git a/nodedb/tests/wire/cases/sql_window_functions.rs b/nodedb/tests/wire/cases/sql_window_functions.rs index 878128ec3..77d870bf4 100644 --- a/nodedb/tests/wire/cases/sql_window_functions.rs +++ b/nodedb/tests/wire/cases/sql_window_functions.rs @@ -417,7 +417,7 @@ async fn window_offset_over_expression_argument_returns_previous_evaluated_value } } -// ── window functions over a DERIVED table (issue #295 Gap 3) ── +// ── window functions over a DERIVED table ── // // A derived-table body that is itself a plain Scan (e.g. `SELECT * FROM s`) // inlines through the CTE path, where the outer window spec was previously From 072bc7cfbdb9ecf644d8252904cf722b8ae16214 Mon Sep 17 00:00:00 2001 From: EnRaiha <15997552+EnRaiha@users.noreply.github.com> Date: Fri, 11 Sep 2026 06:00:10 +0800 Subject: [PATCH 6/6] fix: remove remaining issue #295 refs from comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4 files still had stale (issue #295) references missed in previous cleanup pass — aggregate/plan.rs, set_ops.rs, provider_scan.rs, derived_expression_errors.rs module doc. --- nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs | 2 +- nodedb/src/control/planner/sql_plan_convert/set_ops.rs | 2 +- nodedb/src/data/executor/handlers/provider_scan.rs | 2 +- nodedb/tests/wire/cases/derived_expression_errors.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs index 08625635f..56b3df789 100644 --- a/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs +++ b/nodedb/src/control/planner/sql_plan_convert/aggregate/plan.rs @@ -207,7 +207,7 @@ pub(in crate::control::planner::sql_plan_convert) fn convert_aggregate( // catalog path uses above — so the executor receives the body's rows and // evaluates the aggregate arguments / group keys against them. Without // this the aggregate scanned an empty (non-existent) collection and - // silently returned NULL / no rows (issue #295). + // silently returned NULL / no rows. if !matches!(input, SqlPlan::Scan { .. }) { let derived_group_specs = group_by_to_specs(group_by); let derived_agg_specs: Vec = diff --git a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs index 548794894..c569d84dd 100644 --- a/nodedb/src/control/planner/sql_plan_convert/set_ops.rs +++ b/nodedb/src/control/planner/sql_plan_convert/set_ops.rs @@ -315,7 +315,7 @@ pub(super) fn convert_subquery( // Expression projections ride as computed columns so the // materialized-row ProviderScan evaluates them per row instead // of the response shaper looking up an alias that was never - // computed (silent NULL — issue #295). Window-aliased items are + // computed (silent NULL). Window-aliased items are // excluded here; they ride as window specs below. computed_columns: super::aggregate::extract_computed_columns( projection, diff --git a/nodedb/src/data/executor/handlers/provider_scan.rs b/nodedb/src/data/executor/handlers/provider_scan.rs index 03f5a2ede..228e0cec8 100644 --- a/nodedb/src/data/executor/handlers/provider_scan.rs +++ b/nodedb/src/data/executor/handlers/provider_scan.rs @@ -117,7 +117,7 @@ impl CoreLoop { // constant subqueries) ride as computed columns: evaluate each per // row BEFORE distinct/project so the aliased value exists in the row // map and division/accessor errors fail the query instead of - // silently NULLing (issue #295). + // silently NULLing. if !computed_columns.is_empty() { let computed_cols: Vec = match zerompk::from_msgpack(computed_columns) { diff --git a/nodedb/tests/wire/cases/derived_expression_errors.rs b/nodedb/tests/wire/cases/derived_expression_errors.rs index 371a86490..c0b40f544 100644 --- a/nodedb/tests/wire/cases/derived_expression_errors.rs +++ b/nodedb/tests/wire/cases/derived_expression_errors.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: BUSL-1.1 //! Expression errors over a constant derived table must raise, not fold to -//! NULL / empty rows (issue #295). The derived body materializes as rows on +//! NULL / empty rows. The derived body materializes as rows on //! the coordinator; expression projections and aggregate/group-key arguments //! evaluate against those rows per-row, so division raises 22012 and //! sequence accessors raise 0A000 instead of silently vanishing.