Skip to content

Commit 79401bc

Browse files
committed
fix(sql): apply outer ORDER BY / OFFSET / DISTINCT over subquery bodies
A subquery/derived-table body that cannot absorb the outer query's constraints (e.g. a k-NN search leaf under an outer ORDER BY / OFFSET / DISTINCT, or a reorder-dependent LIMIT) previously dropped those constraints silently, returning the body's raw rows. Add SqlPlan::Subquery, lowered to a coordinator-resolved QueryOp::PostProcess that gathers the body's rows and applies filter -> offset -> sort -> distinct -> project -> limit exactly once over the full result, then wire it through every plan visitor, routing, RLS injection, authorization, schema-derivation, and materialization site that matches on SqlPlan/QueryOp/PhysicalPlan.
1 parent 71e083a commit 79401bc

27 files changed

Lines changed: 858 additions & 25 deletions

File tree

nodedb-cluster-tests/tests/transport_security.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,12 @@ fn insecure_counter_guard() -> &'static Mutex<()> {
3838
LOCK.get_or_init(|| Mutex::new(()))
3939
}
4040

41+
use nodedb_cluster::topology::{ClusterTopology, NodeInfo, NodeState};
4142
use nodedb_cluster::transport::pki_types::{CertificateDer, PrivateKeyDer, PrivatePkcs8KeyDer};
4243
use nodedb_cluster::{
4344
MacKey, NexarTransport, RaftRpcHandler, TlsCredentials, TransportCredentials,
4445
generate_node_credentials, insecure_transport_count, spki_pin_from_cert_der,
4546
};
46-
use nodedb_cluster::topology::{ClusterTopology, NodeInfo, NodeState};
4747
use nodedb_raft::message::{AppendEntriesRequest, AppendEntriesResponse, RequestVoteResponse};
4848
use nodedb_raft::transport::RaftTransport;
4949

nodedb-physical/src/physical_plan/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,6 +324,8 @@ impl PhysicalPlan {
324324
| PhysicalPlan::Graph(GraphOp::WccSuperstep(_)) => None,
325325
// Exchange: recurse into the child plan to extract the collection.
326326
PhysicalPlan::Query(QueryOp::Exchange(op)) => op.child.collection(),
327+
// PostProcess: recurse into the materialized input plan.
328+
PhysicalPlan::Query(QueryOp::PostProcess { input, .. }) => input.collection(),
327329
// ProviderScan is a catalog/constant source — no user collection.
328330
PhysicalPlan::Query(QueryOp::ProviderScan { .. }) => None,
329331
// KV ops carry their own collection (sorted-index-only ops → None).

nodedb-physical/src/physical_plan/query.rs

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,46 @@ pub enum QueryOp {
9292
distinct: bool,
9393
},
9494

95+
/// Relational post-processing over a materialized child plan.
96+
///
97+
/// Coordinator-resolved, exactly like [`QueryOp::Exchange`]: at resolve time
98+
/// the coordinator gathers `input`'s rows, flattens them to the relational
99+
/// row shape, and rewrites this node into a [`QueryOp::ProviderScan`]
100+
/// carrying the same relational parameters — which applies
101+
/// filter→offset→sort→distinct→project→limit on a single core. A Data-Plane
102+
/// core must NEVER see this node (defensive error if it does).
103+
///
104+
/// Lowered from `SqlPlan::Subquery`: an outer `ORDER BY` / `OFFSET` /
105+
/// `DISTINCT` / post-reorder `LIMIT` over a subquery/derived-table body
106+
/// whose leaf plan could not absorb those constraints. Constraints the leaf
107+
/// DID absorb (a `WHERE` pushed into a search engine, an unordered `LIMIT`
108+
/// folded into `top_k`) are applied by `input` and not repeated here.
109+
PostProcess {
110+
/// The subquery body to materialize. Wrapped in `Exchange{Gather}` by
111+
/// the converter when the body is a sharded source, so the gather runs
112+
/// exactly once over the full union before post-processing.
113+
input: Box<crate::physical_plan::PhysicalPlan>,
114+
/// Serialized `Vec<ScanFilter>` (MessagePack). Empty = no predicate.
115+
#[serde(default)]
116+
filters: Vec<u8>,
117+
/// Output column names to keep. Empty = emit all columns.
118+
#[serde(default)]
119+
projection: Vec<String>,
120+
/// Sort keys: `(column_name, ascending)`. Empty = unordered.
121+
#[serde(default)]
122+
sort_keys: Vec<(String, bool)>,
123+
/// Maximum rows to emit after offset/sort/distinct/projection.
124+
/// `None` = unlimited.
125+
#[serde(default)]
126+
limit: Option<usize>,
127+
/// Number of rows to skip before applying limit.
128+
#[serde(default)]
129+
offset: usize,
130+
/// SQL DISTINCT: deduplicate on the projected row.
131+
#[serde(default)]
132+
distinct: bool,
133+
},
134+
95135
/// Aggregate: GROUP BY + aggregate functions.
96136
Aggregate {
97137
collection: String,

nodedb-physical/src/physical_plan/routing.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,13 @@ pub fn plan_contains_cluster_partitioned_leaf(plan: &PhysicalPlan) -> bool {
7575
plan_contains_cluster_partitioned_leaf(child)
7676
}
7777

78+
// Recurse through PostProcess (its materialized input is the real
79+
// plan). Normally resolved to a `ProviderScan` before routing, but a
80+
// conservative recursion keeps routing correct if one survives.
81+
PhysicalPlan::Query(QueryOp::PostProcess { input, .. }) => {
82+
plan_contains_cluster_partitioned_leaf(input)
83+
}
84+
7885
// Recurse through lateral outer plans.
7986
PhysicalPlan::Query(QueryOp::LateralTopK { outer_plan, .. })
8087
| PhysicalPlan::Query(QueryOp::LateralLoop { outer_plan, .. }) => {

nodedb-sql/src/types/plan/cacheability.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ impl SqlPlan {
7676
.fold(outer.cache_eligibility(), |eligibility, (_, plan)| {
7777
eligibility.combine(plan.cache_eligibility())
7878
}),
79+
Self::Subquery { input, .. } => input.cache_eligibility(),
7980
Self::LateralTopK { outer, .. } => outer.cache_eligibility(),
8081
Self::LateralLoop { outer, inner, .. } => {
8182
outer.cache_eligibility().combine(inner.cache_eligibility())

nodedb-sql/src/types/plan/variants.rs

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,40 @@ pub enum SqlPlan {
478478
outer: Box<SqlPlan>,
479479
},
480480

481+
/// Relational post-processing over a subquery/derived-table body whose leaf
482+
/// plan cannot absorb the outer query's constraints.
483+
///
484+
/// Produced by CTE / derived-table inlining when the referenced body is not
485+
/// a plain `Scan` (which carries its own filters/sort/limit/offset/distinct)
486+
/// and the outer reference adds constraints the body has no slot for — an
487+
/// `ORDER BY`, `OFFSET`, `DISTINCT`, or a `LIMIT` that must apply *after* a
488+
/// reorder. Without this node those constraints were silently dropped,
489+
/// returning the body's unordered/unoffset/undeduplicated rows.
490+
///
491+
/// The Data Plane materializes `input`'s rows, then applies, in this order:
492+
/// filter → offset → sort → distinct → project → limit — matching
493+
/// `QueryOp::ProviderScan` semantics (which this lowers onto). Constraints
494+
/// the body CAN absorb (e.g. an unordered `LIMIT` folded into a vector
495+
/// search's `top_k`, or a `WHERE` pushed into the engine as a post-filter)
496+
/// are applied at the leaf during inlining and are NOT repeated here.
497+
Subquery {
498+
/// The subquery body whose rows are post-processed.
499+
input: Box<SqlPlan>,
500+
/// Predicates applied to the materialized rows (outer `WHERE` that the
501+
/// body leaf did not absorb).
502+
filters: Vec<Filter>,
503+
/// Outer projection (target list). Empty = inherit the body's columns.
504+
projection: Vec<Projection>,
505+
/// Outer `ORDER BY` keys applied over the materialized rows.
506+
sort_keys: Vec<SortKey>,
507+
/// Outer `OFFSET` (0 = none).
508+
offset: usize,
509+
/// Outer `DISTINCT`.
510+
distinct: bool,
511+
/// Outer `LIMIT` (`None` = unbounded).
512+
limit: Option<usize>,
513+
},
514+
481515
// ── Array (ND sparse) ─────────────────────────────────────
482516
/// `CREATE ARRAY <name> DIMS (...) ATTRS (...) TILE_EXTENTS (...)`.
483517
/// AST is engine-agnostic — the Origin converter builds the typed

nodedb-sql/src/visitor/plan_visitor/args.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,17 @@ pub struct ScanVisitArgs<'a> {
3232
pub temporal: &'a TemporalScope,
3333
}
3434

35+
/// Parameters for [`super::trait_def::PlanVisitor::subquery`].
36+
pub struct SubqueryVisitArgs<'a> {
37+
pub input: &'a SqlPlan,
38+
pub filters: &'a [Filter],
39+
pub projection: &'a [Projection],
40+
pub sort_keys: &'a [SortKey],
41+
pub offset: usize,
42+
pub distinct: bool,
43+
pub limit: Option<usize>,
44+
}
45+
3546
/// Parameters for [`super::trait_def::PlanVisitor::document_index_lookup`].
3647
pub struct DocumentIndexLookupVisitArgs<'a> {
3748
pub collection: &'a str,

nodedb-sql/src/visitor/plan_visitor/dispatch_rest.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,23 @@ pub(super) fn dispatch_rest<V: PlanVisitor>(
2222
SqlPlan::Intersect { left, right, all } => visitor.intersect(left, right, *all),
2323
SqlPlan::Except { left, right, all } => visitor.except(left, right, *all),
2424
SqlPlan::Cte { definitions, outer } => visitor.cte(definitions, outer),
25+
SqlPlan::Subquery {
26+
input,
27+
filters,
28+
projection,
29+
sort_keys,
30+
offset,
31+
distinct,
32+
limit,
33+
} => visitor.subquery(super::args::SubqueryVisitArgs {
34+
input,
35+
filters,
36+
projection,
37+
sort_keys,
38+
offset: *offset,
39+
distinct: *distinct,
40+
limit: *limit,
41+
}),
2542
SqlPlan::CreateArray {
2643
name,
2744
dims,

nodedb-sql/src/visitor/plan_visitor/trait_def.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ use super::args::{
99
AggregateVisitArgs, CreateArrayVisitArgs, DocumentIndexLookupVisitArgs,
1010
HybridSearchTripleVisitArgs, HybridSearchVisitArgs, InsertVisitArgs, JoinVisitArgs,
1111
LateralLoopVisitArgs, LateralTopKVisitArgs, MergeVisitArgs, RecursiveScanVisitArgs,
12-
RecursiveValueVisitArgs, ScanVisitArgs, SpatialScanVisitArgs, TimeseriesScanVisitArgs,
13-
UpdateFromVisitArgs, UpsertVisitArgs, VectorSearchVisitArgs,
12+
RecursiveValueVisitArgs, ScanVisitArgs, SpatialScanVisitArgs, SubqueryVisitArgs,
13+
TimeseriesScanVisitArgs, UpdateFromVisitArgs, UpsertVisitArgs, VectorSearchVisitArgs,
1414
};
1515
use crate::fts_types::FtsQuery;
1616
use crate::temporal::TemporalScope;
@@ -234,6 +234,9 @@ pub trait PlanVisitor {
234234
outer: &SqlPlan,
235235
) -> Result<Self::Output, Self::Error>;
236236

237+
/// Handle [`SqlPlan::Subquery`].
238+
fn subquery(&mut self, args: SubqueryVisitArgs<'_>) -> Result<Self::Output, Self::Error>;
239+
237240
/// Handle [`SqlPlan::CreateArray`].
238241
fn create_array(&mut self, args: CreateArrayVisitArgs<'_>)
239242
-> Result<Self::Output, Self::Error>;

nodedb/src/control/exec_receiver/support.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,10 @@ pub(super) fn plan_contains_exchange(plan: &PhysicalPlan) -> bool {
6464
.any(|child| plan_contains_exchange(child)),
6565
QueryOp::LateralTopK { outer_plan, .. } => plan_contains_exchange(outer_plan),
6666
QueryOp::LateralLoop { outer_plan, .. } => plan_contains_exchange(outer_plan),
67+
// PostProcess wraps a materialized child that, before coordinator
68+
// resolution, still carries an `Exchange{Gather}` — recurse so an
69+
// unresolved PostProcess is correctly flagged as Exchange-bearing.
70+
QueryOp::PostProcess { input, .. } => plan_contains_exchange(input),
6771
// Aggregate may carry a sub-plan input (catalog `ProviderScan`),
6872
// which could in principle nest an Exchange — recurse when present.
6973
QueryOp::Aggregate { input, .. } => {

0 commit comments

Comments
 (0)