Skip to content

Commit 03de4e9

Browse files
committed
feat(document): partial index support with cluster-wide backfill fan-out
Add WHERE-predicate support to document secondary indexes throughout the full stack, fixing the silent-miss class (issue #71) where a partial UNIQUE index applied uniqueness constraints to rows outside its predicate scope. Predicate text is carried on RegisteredIndex from DDL through the wire format to the Data Plane, where it is parsed once into IndexPredicate and evaluated per row on every write, UNIQUE check, and backfill pass. Only rows for which the predicate is explicitly true are indexed or constrained (Postgres partial-index semantics — NULL and non-boolean results exclude the row). Cluster CREATE INDEX now fans the BackfillIndex primitive to every Active peer via RaftRpc::ExecuteRequest after the coordinator's local dispatch succeeds. Without the fan-out, non-coordinator nodes that hosted rows the coordinator never visited would produce incomplete indexes on Ready flip. The planner rewrite also gains conjunct-aware equality extraction so a WHERE clause of the form `a = 1 AND b > 2` correctly drives an index lookup on `a` while carrying `b > 2` as a residual post-filter, and partial indexes are rejected when the query's conjuncts don't entail the index predicate (conservative structural check). DISTINCT is no longer an automatic fallback to full-scan on the indexed-fetch path.
1 parent 232297e commit 03de4e9

18 files changed

Lines changed: 510 additions & 35 deletions

File tree

nodedb-sql/src/engine_rules/mod.rs

Lines changed: 167 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -124,36 +124,71 @@ pub(crate) fn try_document_index_lookup(
124124
params: &ScanParams,
125125
engine: EngineType,
126126
) -> Option<SqlPlan> {
127-
// Sort / distinct / window functions are not yet supported on the
128-
// indexed-fetch path — fall back to a full scan so existing behavior
129-
// stays correct. Extending the handler later is additive and doesn't
130-
// invalidate the rewrite.
131-
if !params.sort_keys.is_empty() || params.distinct || !params.window_functions.is_empty() {
127+
// Sort / window functions still force a full scan because the
128+
// IndexedFetch handler does not yet order rows or evaluate window
129+
// aggregates. DISTINCT is safe on the index path: the handler
130+
// emits each matched doc exactly once and any further dedup is a
131+
// cheap Control-Plane pass on the scan-shaped response.
132+
if !params.sort_keys.is_empty() || !params.window_functions.is_empty() {
132133
return None;
133134
}
134135

135136
// Iterate filters to find the first equality candidate that lines up
136137
// with a Ready index. Keep the remaining filters as post-filters.
137-
// Two predicate shapes appear in practice: the resolver-emitted
138-
// `FilterExpr::Comparison` (compact) and the generic
139-
// `FilterExpr::Expr(SqlExpr::BinaryOp { Column, Eq, Literal })`
140-
// wrapper the default path produces. Both unambiguously express
141-
// equality on a column — handle both.
138+
// Predicates appear in three shapes:
139+
// - `FilterExpr::Comparison { field, Eq, value }` — resolver path.
140+
// - `FilterExpr::Expr(BinaryOp { Column, Eq, Literal })` — generic.
141+
// - `FilterExpr::Expr(BinaryOp { left AND right })` — the
142+
// top-level AND the `convert_where_to_filters` path produces
143+
// for compound WHERE clauses like `a = 1 AND b > 2`. Descend
144+
// into AND conjuncts so an equality nested in a conjunction
145+
// still picks up the index, and the non-equality siblings
146+
// survive as a residual conjunction carried on the
147+
// IndexedFetch node (not as a wrapping Filter plan node —
148+
// that wrapping is the Control-Plane post-filter anti-pattern
149+
// the handler's `filters` payload exists to eliminate).
150+
// Pre-collect the query's top-level conjuncts once. Used for
151+
// partial-index entailment: every conjunct of the index predicate
152+
// must appear as a conjunct of the query WHERE for the rewrite to
153+
// be sound, otherwise the index would silently omit rows the
154+
// query expects to see.
155+
let query_conjuncts: Vec<SqlExpr> = params
156+
.filters
157+
.iter()
158+
.flat_map(|f| match &f.expr {
159+
FilterExpr::Expr(e) => {
160+
let mut v = Vec::new();
161+
flatten_and(e, &mut v);
162+
v
163+
}
164+
_ => Vec::new(),
165+
})
166+
.collect();
167+
142168
for (i, f) in params.filters.iter().enumerate() {
143-
let Some((field, value)) = extract_equality(&f.expr) else {
169+
let Some((field, value, residual)) = extract_equality_with_residual(&f.expr) else {
144170
continue;
145171
};
146172
let canonical = canonical_index_field(&field);
147-
let Some(idx) = params
148-
.indexes
149-
.iter()
150-
.find(|i| i.state == IndexState::Ready && i.field == canonical)
151-
else {
173+
let Some(idx) = params.indexes.iter().find(|i| {
174+
i.state == IndexState::Ready
175+
&& i.field == canonical
176+
&& partial_index_entailed(i.predicate.as_deref(), &query_conjuncts)
177+
}) else {
152178
continue;
153179
};
154180

155181
let mut remaining = params.filters.clone();
156-
remaining.remove(i);
182+
match residual {
183+
Some(expr) => {
184+
remaining[i] = Filter {
185+
expr: FilterExpr::Expr(expr),
186+
};
187+
}
188+
None => {
189+
remaining.remove(i);
190+
}
191+
}
157192

158193
let lookup_value = if idx.case_insensitive {
159194
lowercase_string_value(&value)
@@ -180,32 +215,131 @@ pub(crate) fn try_document_index_lookup(
180215
None
181216
}
182217

183-
/// Pull `(column_name, equality_value)` out of a filter expression if it
184-
/// is a column-equals-literal predicate in either of the planner's two
185-
/// encodings.
186-
fn extract_equality(expr: &FilterExpr) -> Option<(String, SqlValue)> {
218+
/// Pull `(column_name, equality_value, residual_expr)` out of a filter
219+
/// expression if it contains a column-equals-literal predicate that can
220+
/// drive an index lookup. Returns `None` if no usable equality is
221+
/// present. The returned residual is the rest of the conjunction with
222+
/// the equality removed — `None` when the filter was a bare equality.
223+
fn extract_equality_with_residual(
224+
expr: &FilterExpr,
225+
) -> Option<(String, SqlValue, Option<SqlExpr>)> {
187226
match expr {
188227
FilterExpr::Comparison {
189228
field,
190229
op: CompareOp::Eq,
191230
value,
192-
} => Some((field.clone(), value.clone())),
193-
FilterExpr::Expr(SqlExpr::BinaryOp { left, op, right }) => {
194-
let (col, lit) = match (left.as_ref(), op, right.as_ref()) {
195-
(SqlExpr::Column { name, .. }, BinaryOp::Eq, SqlExpr::Literal(v)) => {
196-
(name.clone(), v.clone())
197-
}
198-
(SqlExpr::Literal(v), BinaryOp::Eq, SqlExpr::Column { name, .. }) => {
199-
(name.clone(), v.clone())
200-
}
201-
_ => return None,
202-
};
203-
Some((col, lit))
231+
} => Some((field.clone(), value.clone(), None)),
232+
FilterExpr::Expr(sql_expr) => {
233+
let (col, lit, residual) = split_equality_from_expr(sql_expr)?;
234+
Some((col, lit, residual))
204235
}
205236
_ => None,
206237
}
207238
}
208239

240+
/// Return `(column, literal, residual_expr)` for an equality found
241+
/// anywhere in a right-leaning AND conjunction tree, or `None` if no
242+
/// bare column-equals-literal predicate exists. The residual preserves
243+
/// every sibling conjunct in their original order; `None` means the
244+
/// expression was a bare equality with nothing left behind.
245+
fn split_equality_from_expr(expr: &SqlExpr) -> Option<(String, SqlValue, Option<SqlExpr>)> {
246+
// Gather the conjuncts of a top-level AND chain left-to-right.
247+
let mut conjuncts: Vec<SqlExpr> = Vec::new();
248+
flatten_and(expr, &mut conjuncts);
249+
250+
// Find the first conjunct that is a bare column-equals-literal.
251+
let eq_idx = conjuncts.iter().position(is_column_eq_literal)?;
252+
let eq = conjuncts.remove(eq_idx);
253+
let (col, lit) = match eq {
254+
SqlExpr::BinaryOp { left, op, right } => match (*left, op, *right) {
255+
(SqlExpr::Column { name, .. }, BinaryOp::Eq, SqlExpr::Literal(v)) => (name, v),
256+
(SqlExpr::Literal(v), BinaryOp::Eq, SqlExpr::Column { name, .. }) => (name, v),
257+
_ => return None,
258+
},
259+
_ => return None,
260+
};
261+
262+
let residual = rebuild_and(conjuncts);
263+
Some((col, lit, residual))
264+
}
265+
266+
/// Append every leaf of a right-leaning `AND` tree to `out`. Non-AND
267+
/// expressions are a single leaf.
268+
fn flatten_and(expr: &SqlExpr, out: &mut Vec<SqlExpr>) {
269+
match expr {
270+
SqlExpr::BinaryOp {
271+
left,
272+
op: BinaryOp::And,
273+
right,
274+
} => {
275+
flatten_and(left, out);
276+
flatten_and(right, out);
277+
}
278+
other => out.push(other.clone()),
279+
}
280+
}
281+
282+
/// Reassemble a list of conjuncts into a right-leaning AND tree.
283+
/// Empty input returns `None`; single input returns itself.
284+
fn rebuild_and(mut conjuncts: Vec<SqlExpr>) -> Option<SqlExpr> {
285+
let last = conjuncts.pop()?;
286+
Some(
287+
conjuncts
288+
.into_iter()
289+
.rfold(last, |acc, next| SqlExpr::BinaryOp {
290+
left: Box::new(next),
291+
op: BinaryOp::And,
292+
right: Box::new(acc),
293+
}),
294+
)
295+
}
296+
297+
/// Decide whether the query's WHERE conjuncts entail the partial-index
298+
/// predicate. `None` predicate means a full index — trivially entailed.
299+
/// Initial version uses conjunct-level structural equality: every
300+
/// conjunct of the index predicate must appear (by `PartialEq`) as a
301+
/// conjunct of the query. This is conservative and deliberately so —
302+
/// a false positive here would silently omit rows from query results,
303+
/// which is the exact bug #73 reports.
304+
fn partial_index_entailed(predicate: Option<&str>, query_conjuncts: &[SqlExpr]) -> bool {
305+
let Some(text) = predicate else {
306+
return true;
307+
};
308+
let Ok(parsed) = crate::parse_expr_string(text) else {
309+
// A catalog predicate we can't parse is not entailed — refuse
310+
// to use the index rather than assume anything about its
311+
// contents. The DDL path validates at CREATE INDEX time, so
312+
// reaching this branch indicates drift.
313+
return false;
314+
};
315+
let mut index_conjuncts: Vec<SqlExpr> = Vec::new();
316+
flatten_and(&parsed, &mut index_conjuncts);
317+
// Structural equality via the stable `Debug` representation:
318+
// `SqlExpr` doesn't derive `PartialEq` (several nested variants
319+
// carry types that can't derive it cheaply), but the Debug form is
320+
// canonical for equivalent trees produced by the same parser. This
321+
// is conservative — it matches only identical AST shapes and not,
322+
// e.g., `a = 1` vs `1 = a`. Callers should write index predicates
323+
// in the same normal form they use in query WHERE clauses, which
324+
// is the convention everywhere else in the codebase.
325+
index_conjuncts.iter().all(|ic| {
326+
let ic_dbg = format!("{ic:?}");
327+
query_conjuncts.iter().any(|qc| format!("{qc:?}") == ic_dbg)
328+
})
329+
}
330+
331+
fn is_column_eq_literal(expr: &SqlExpr) -> bool {
332+
matches!(
333+
expr,
334+
SqlExpr::BinaryOp { left, op: BinaryOp::Eq, right }
335+
if matches!(
336+
(left.as_ref(), right.as_ref()),
337+
(SqlExpr::Column { .. }, SqlExpr::Literal(_))
338+
| (SqlExpr::Literal(_), SqlExpr::Column { .. }),
339+
)
340+
)
341+
}
342+
209343
fn canonical_index_field(field: &str) -> String {
210344
if field.starts_with("$.") || field.starts_with('$') {
211345
field.to_string()

nodedb-sql/src/types.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,12 @@ pub struct IndexSpec {
530530
pub case_insensitive: bool,
531531
/// Build state. Only `Ready` indexes drive query rewrites.
532532
pub state: IndexState,
533+
/// Partial-index predicate as raw SQL text (`WHERE <expr>` body
534+
/// without the keyword), or `None` for full indexes. The planner
535+
/// uses this to reject rewrites whose WHERE clause doesn't entail
536+
/// the predicate — matching against such a partial index would
537+
/// omit rows the index didn't cover.
538+
pub predicate: Option<String>,
533539
}
534540

535541
/// Planner-facing index state. Mirrors the catalog variant but lives here

nodedb/src/bridge/physical_plan/document.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,13 @@ pub struct RegisteredIndex {
256256
pub unique: bool,
257257
pub case_insensitive: bool,
258258
pub state: RegisteredIndexState,
259+
/// Partial-index predicate as raw SQL text (`WHERE <expr>` body,
260+
/// without the `WHERE` keyword). `None` for full indexes.
261+
/// The write path parses and evaluates this per document; rows
262+
/// where the predicate is false are not inserted into the index
263+
/// and do not participate in UNIQUE enforcement.
264+
#[serde(default)]
265+
pub predicate: Option<String>,
259266
}
260267

261268
/// Document engine physical operations (schemaless + strict + DML).
@@ -402,6 +409,11 @@ pub enum DocumentOp {
402409
is_array: bool,
403410
unique: bool,
404411
case_insensitive: bool,
412+
/// Partial-index predicate (raw SQL text of the `WHERE` body)
413+
/// or `None` for full indexes. Rows where the predicate is
414+
/// false are skipped — not indexed, not UNIQUE-checked.
415+
#[serde(default)]
416+
predicate: Option<String>,
405417
},
406418

407419
/// Truncate: delete ALL documents in a collection.

nodedb/src/bridge/physical_plan/wire.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ mod tests {
9999
unique: false,
100100
case_insensitive: false,
101101
state: crate::bridge::physical_plan::RegisteredIndexState::Ready,
102+
predicate: None,
102103
}],
103104
crdt_enabled: false,
104105
storage_mode: crate::bridge::physical_plan::StorageMode::Schemaless,

nodedb/src/control/metrics/system.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,12 @@ pub struct SystemMetrics {
6767
pub document_inserts: AtomicU64,
6868
pub document_reads: AtomicU64,
6969
pub document_collections: AtomicU64,
70+
/// Count of `DocumentOp::BackfillIndex` handler invocations on this
71+
/// node's Data Plane. Per-node: every node increments its own
72+
/// counter exactly when its local core runs the backfill primitive.
73+
/// Tests assert per-node fan-out for distributed CREATE INDEX by
74+
/// reading this counter on every node after DDL completion.
75+
pub document_index_backfills: AtomicU64,
7076

7177
pub columnar_segments: AtomicU64,
7278
pub columnar_compaction_queue: AtomicU64,
@@ -329,6 +335,11 @@ impl SystemMetrics {
329335
self.document_reads.fetch_add(1, Ordering::Relaxed);
330336
}
331337

338+
pub fn record_document_index_backfill(&self) {
339+
self.document_index_backfills
340+
.fetch_add(1, Ordering::Relaxed);
341+
}
342+
332343
pub fn update_document_collections(&self, count: u64) {
333344
self.document_collections.store(count, Ordering::Relaxed);
334345
}

nodedb/src/control/planner/catalog_adapter.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ impl SqlCatalog for OriginCatalog {
240240
nodedb_sql::types::IndexState::Ready
241241
}
242242
},
243+
predicate: i.predicate.clone(),
243244
})
244245
.collect();
245246

nodedb/src/control/server/native/dispatch/plan_builder/document.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ pub(crate) fn build_register(fields: &TextFields, collection: &str) -> crate::Re
327327
unique: false,
328328
case_insensitive: false,
329329
state: crate::bridge::physical_plan::RegisteredIndexState::Ready,
330+
predicate: None,
330331
})
331332
.collect();
332333

nodedb/src/control/server/pgwire/ddl/collection/create/register.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ fn derive_auto_indexes<'a>(
7575
unique: false,
7676
case_insensitive: false,
7777
state: crate::bridge::physical_plan::RegisteredIndexState::Ready,
78+
predicate: None,
7879
})
7980
.collect()
8081
}
@@ -102,6 +103,7 @@ fn extend_with_catalog_indexes(
102103
unique: idx.unique,
103104
case_insensitive: idx.case_insensitive,
104105
state,
106+
predicate: idx.predicate.clone(),
105107
};
106108
if let Some(existing) = out.iter_mut().find(|e| e.path == spec.path) {
107109
*existing = spec;

nodedb/src/control/server/pgwire/ddl/collection/index.rs

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,10 +227,11 @@ pub async fn create_index(
227227
let backfill_plan = crate::bridge::envelope::PhysicalPlan::Document(
228228
crate::bridge::physical_plan::DocumentOp::BackfillIndex {
229229
collection: collection.clone(),
230-
path: extraction_path,
230+
path: extraction_path.clone(),
231231
is_array,
232232
unique: is_unique,
233233
case_insensitive,
234+
predicate: where_condition.clone(),
234235
},
235236
);
236237
let backfill_resp = crate::control::server::dispatch_utils::dispatch_to_data_plane(
@@ -257,6 +258,25 @@ pub async fn create_index(
257258
return Err(sqlstate_error(code, &detail));
258259
}
259260

261+
// Phase 2b: fan the same backfill op to every other cluster node.
262+
// `execute_backfill_index` is vShard-local per core, so without
263+
// this step non-coordinator nodes never populate the index for
264+
// the rows they host — the #71 silent-miss bug. Single-node and
265+
// peerless clusters short-circuit inside the helper.
266+
super::index_fanout::backfill_on_peers(
267+
state,
268+
super::index_fanout::PeerBackfill {
269+
tenant_id,
270+
collection: &collection,
271+
path: &extraction_path,
272+
is_array,
273+
unique: is_unique,
274+
case_insensitive,
275+
predicate: where_condition.as_deref(),
276+
},
277+
)
278+
.await?;
279+
260280
// Phase 3: flip to Ready. Re-read the collection so any concurrent
261281
// mutation (e.g. another DDL on the same collection — blocked by
262282
// descriptor drain in cluster mode, serialized by pgwire session in

0 commit comments

Comments
 (0)