Skip to content

Commit 94214ab

Browse files
committed
fix(sql): read fusion options through a claim-tracking cursor
Fusion parameters were read from a raw token slice and the variant then claimed every remaining token, so a mistyped option keyword ran the statement with a default and no error. The shared extractor now reads through the cursor: each keyword and value is claimed, the ARRAY[...] payload is claimed element by element, and a token no clause owns is refused by name. A present-but-unreadable value is an error, never a default. - drop the raw-text readers; the ARRAY payload and the RRF_K count come from the same token stream - the BM25 field's ON is read after the BM25 anchor: ON also introduces the collection, and the first match is not the field's - the wrapped SEARCH ... USING FUSION(...) surface validates with the same extractor and refuses unclaimed tokens
1 parent 62cf9dd commit 94214ab

7 files changed

Lines changed: 269 additions & 186 deletions

File tree

‎nodedb-sql/src/ddl_ast/graph_parse/cursor.rs‎

Lines changed: 91 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,22 @@ impl<'a> Cursor<'a> {
6969
self.claim_text(pos + 1)
7070
}
7171

72+
/// The word or quoted literal after `keyword`, searching only at or after
73+
/// the first `anchor` token. Used where one keyword introduces two clauses,
74+
/// as `ON` does for `ON <collection>` and `BM25 <text> ON <field>`.
75+
pub(super) fn quoted_after_from(&mut self, anchor: &str, keyword: &str) -> Option<String> {
76+
let anchor_pos = self
77+
.toks
78+
.iter()
79+
.position(|tok| Self::is_keyword(tok, anchor))?;
80+
let offset = self.toks[anchor_pos..]
81+
.iter()
82+
.position(|tok| Self::is_keyword(tok, keyword))?;
83+
let pos = anchor_pos + offset;
84+
self.used[pos] = true;
85+
self.claim_text(pos + 1)
86+
}
87+
7288
/// Every consecutive word or quoted literal after `keyword`, up to the
7389
/// first token that is neither. Used by `AS <label> [, <label>…]`.
7490
pub(super) fn quoted_list_after(&mut self, keyword: &str) -> Vec<String> {
@@ -119,18 +135,68 @@ impl<'a> Cursor<'a> {
119135
}
120136

121137
/// The next `count` float words after `keyword`.
122-
pub(super) fn floats_after<const N: usize>(&mut self, keyword: &str) -> Option<[f64; N]> {
123-
let pos = self.find(keyword)?;
124-
let mut out = [0.0f64; N];
125-
for (offset, slot) in out.iter_mut().enumerate() {
126-
let value = match self.toks.get(pos + 1 + offset)? {
127-
Tok::Word(w) => w.parse::<f64>().ok()?,
128-
_ => return None,
138+
pub(super) fn floats_after_max(
139+
&mut self,
140+
keyword: &str,
141+
max: usize,
142+
) -> Result<Option<Vec<f64>>, SqlError> {
143+
let Some(pos) = self.find(keyword) else {
144+
return Ok(None);
145+
};
146+
let mut out = Vec::new();
147+
let mut at = pos + 1;
148+
while out.len() < max {
149+
match self.toks.get(at) {
150+
Some(Tok::Word(w)) => match w.parse::<f64>() {
151+
Ok(value) => {
152+
self.used[at] = true;
153+
out.push(value);
154+
at += 1;
155+
}
156+
Err(_) => break,
157+
},
158+
_ => break,
159+
}
160+
}
161+
if out.is_empty() {
162+
let found = match self.toks.get(pos + 1) {
163+
Some(Tok::Word(w)) => (*w).to_string(),
164+
Some(Tok::Quoted(s)) => format!("'{s}'"),
165+
Some(Tok::Object(_)) => "{…}".to_string(),
166+
None => "nothing".to_string(),
129167
};
130-
self.used[pos + 1 + offset] = true;
131-
*slot = value;
168+
return Err(SqlError::Parse {
169+
detail: format!("{keyword} expects numbers — found {found}"),
170+
});
171+
}
172+
Ok(Some(out))
173+
}
174+
175+
/// The `ARRAY[f1, f2, …]` payload after `anchor`, claiming the anchor, the
176+
/// `ARRAY` word, and every numeric element. The tokenizer drops the
177+
/// brackets, so the element run ends at the first non-numeric token (the
178+
/// next clause keyword).
179+
pub(super) fn floats_array_after(&mut self, anchor: &str) -> Option<Vec<f64>> {
180+
let pos = self.find(anchor)?;
181+
let mut at = pos + 1;
182+
if let Some(Tok::Word(w)) = self.toks.get(at)
183+
&& w.eq_ignore_ascii_case("ARRAY")
184+
{
185+
self.used[at] = true;
186+
at += 1;
132187
}
133-
Some(out)
188+
let mut out = Vec::new();
189+
while let Some(Tok::Word(w)) = self.toks.get(at) {
190+
match w.parse::<f64>() {
191+
Ok(value) => {
192+
self.used[at] = true;
193+
out.push(value);
194+
at += 1;
195+
}
196+
Err(_) => break,
197+
}
198+
}
199+
if out.is_empty() { None } else { Some(out) }
134200
}
135201

136202
/// Read a `DIRECTION` clause.
@@ -188,23 +254,6 @@ impl<'a> Cursor<'a> {
188254
}
189255
}
190256

191-
/// The token slice, for readers that validate their own span (fusion
192-
/// parameters read the bracket payload from the raw statement text).
193-
pub(super) fn tokens(&self) -> &[Tok<'a>] {
194-
&self.toks
195-
}
196-
197-
/// Claim every remaining token.
198-
///
199-
/// For a statement whose parameters a dedicated extractor validated
200-
/// (`GRAPH RAG FUSION`), where this cursor's clause readers do not see the
201-
/// bracket payload the extractor reads from the raw text.
202-
pub(super) fn consume_rest(&mut self) {
203-
for slot in self.used.iter_mut() {
204-
*slot = true;
205-
}
206-
}
207-
208257
/// Refuse the first token no clause claimed.
209258
pub(super) fn finish(self, statement: &str) -> Result<(), SqlError> {
210259
for (tok, used) in self.toks.iter().zip(self.used.iter()) {
@@ -271,9 +320,23 @@ mod tests {
271320
let toks = tokenize("GRAPH RAG FUSION ON g RRF_K (60.0, 35.0)");
272321
let mut cursor = Cursor::new(toks, 3);
273322
assert_eq!(cursor.word_after("ON").as_deref(), Some("g"));
274-
assert_eq!(cursor.floats_after::<2>("RRF_K"), Some([60.0, 35.0]));
323+
assert_eq!(
324+
cursor.floats_after_max("RRF_K", 3).unwrap(),
325+
Some(vec![60.0, 35.0])
326+
);
275327
cursor
276328
.finish("GRAPH RAG FUSION")
277329
.expect("every token claimed");
278330
}
331+
332+
#[test]
333+
fn a_mistyped_float_value_is_refused() {
334+
let toks = tokenize("GRAPH RAG FUSION ON g RRF_K (fast)");
335+
let mut cursor = Cursor::new(toks, 3);
336+
let err = cursor.floats_after_max("RRF_K", 3).unwrap_err();
337+
assert!(
338+
err.to_string().contains("RRF_K"),
339+
"the error must name the clause: {err}"
340+
);
341+
}
279342
}

‎nodedb-sql/src/ddl_ast/graph_parse/entry.rs‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,7 @@ pub fn try_parse(sql: &str) -> Option<Result<NodedbStatement, SqlError>> {
5555
} else if upper.starts_with("GRAPH ALGO ") {
5656
run(toks, 2, "GRAPH ALGO", variants::parse_algo)
5757
} else if upper.starts_with("GRAPH RAG FUSION ") {
58-
run(toks, 3, "GRAPH RAG FUSION", |cursor| {
59-
variants::parse_rag_fusion(cursor, trimmed)
60-
})
58+
run(toks, 3, "GRAPH RAG FUSION", variants::parse_rag_fusion)
6159
} else {
6260
// Starts with `GRAPH ` but names no known command. Still graph DSL,
6361
// so report it here rather than letting the SQL parser guess.
@@ -382,6 +380,28 @@ mod tests {
382380
}
383381
}
384382

383+
/// `ON` introduces both the fusion collection and the BM25 field. The field
384+
/// read must anchor on `BM25`, or the collection name is captured as the
385+
/// field and the real `ON` is left unclaimed.
386+
#[test]
387+
fn parse_rag_fusion_three_source_reads_the_bm25_field() {
388+
let stmt = parsed(
389+
"GRAPH RAG FUSION ON ents \
390+
QUERY ARRAY[0.1] \
391+
VECTOR_TOP_K 5 \
392+
BM25 'attention' ON 'body' \
393+
RRF_K (60.0, 35.0, 50.0)",
394+
);
395+
match stmt {
396+
NodedbStatement::Graph(GraphStmt::GraphRagFusion { params, .. }) => {
397+
assert_eq!(params.bm25_query.as_deref(), Some("attention"));
398+
assert_eq!(params.bm25_field.as_deref(), Some("body"));
399+
assert_eq!(params.rrf_k_triple, Some((60.0, 35.0, 50.0)));
400+
}
401+
other => panic!("expected GraphRagFusion, got {other:?}"),
402+
}
403+
}
404+
385405
#[test]
386406
fn parse_rag_fusion_missing_collection_names_the_clause() {
387407
let error = try_parse("GRAPH RAG FUSION QUERY ARRAY[0.1] VECTOR_TOP_K 5")

0 commit comments

Comments
 (0)