feat(sparql): burn-down Wave 1 — validation passes, grammar tightening, authoritative diagnostics, reifier forms (−123 registers)#1453
Open
aaj3f wants to merge 26 commits into
Open
feat(sparql): burn-down Wave 1 — validation passes, grammar tightening, authoritative diagnostics, reifier forms (−123 registers)#1453aaj3f wants to merge 26 commits into
aaj3f wants to merge 26 commits into
Conversation
Per GroupGraphPatternSub, a '.' at group level is legal only as the
single optional separator immediately after a GraphPatternNotTriples;
within a TriplesBlock the '.' between two same-subject blocks is
mandatory. The group loop previously skipped any dot anywhere, so
leading, doubled, and standalone dots — and missing separators between
triple patterns — were silently accepted.
- parse_group_graph_pattern: replace the skip-any-dot branch with a
dot_allowed flag tracked per grammar position; stray dots now emit an
error-severity diagnostic (same single-token flow, no backtracking).
- parse_triples_block: when the optional trailing dot is absent and the
next token starts a term, emit "expected '.' between triple
patterns".
Greens W3C syn-bad-02/03 (missing dot) and syn-bad-05..14 (stray dots);
removes the 12 register entries (SPARQL10_SYNTAX).
BEHAVIOR CHANGE (D-4, hard error + changelog): queries relying on the
previous dot leniency (e.g. '{ ?s ?p ?o .. }' or '{ . }') are now
rejected. This is the highest-regression-surface change in the wave and
is isolated in this PR for easy bisect/revert.
Filter ::= 'FILTER' Constraint, where Constraint is a bracketted expression, a built-in call, or a function call. parse_filter_pattern previously accepted anything parse_expression accepted, so bare terms (FILTER ?x, FILTER true) and unparenthesized operator expressions (FILTER ?x > 5) parsed silently. The check is a post-parse AST-shape predicate (is_constraint_expression) rather than a token-level keyword enumeration: it stays correct as builtins are added and preserves the existing token flow. The Filter node is still produced for tooling; the error-severity diagnostic makes the parse fail authoritatively at the API seam. Aggregates remain grammatically acceptable here (rejecting aggregates in FILTER is semantic validation, PR-2's territory). PR-1's P5b ORDER-BY-constraint fix admits the same alternatives; the helper documents that coupling. Greens W3C filter-missing-parens; removes its SPARQL10_SYNTAX register entry. BEHAVIOR CHANGE (D-4, hard error + changelog): FILTER ?x and FILTER ?x > 5 are now rejected — wrap the expression: FILTER(...).
…ery seam parse_and_validate_sparql accepted any parse that produced an AST, even when error recovery had emitted error-severity diagnostics — so a query the parser flagged as malformed still executed against the recovered (silently rewritten) AST. Every public query entry point funnels through this helper (fluree.query / query_sparql / dataset + stream + credential + connection + explain paths), so the diagnostic-swallowing hole made any reject-more parser work cosmetic (ROADMAP §1 addendum). Decision (D-4 shape): reject at the API seam rather than making parse_sparql withhold the AST on error. This is the minimal change that makes error diagnostics authoritative while preserving ParseOutput's recovery contract (AST + diagnostics) for tooling and the W3C harness; warning-severity diagnostics still never reject. The SPARQL UPDATE path (tx_builder::parse_and_lower_sparql_update) already enforced this rule; the two seams now match. Adds API-level regression tests proving V1/V2-flagged queries return an error through fluree.query instead of executing (each fails without this fix).
…alidation Two AST walkers needed by the V3-V6 validation passes (roadmap §1.1 item 2 — no Expression::variables()/contains_aggregate() helper existed): - Expression::walk/contains_aggregate/variables/unaggregated_variables in ast/expr.rs — pre-order expression traversal that stops at EXISTS/NOT EXISTS pattern boundaries, with an aggregate-argument- skipping variant for GROUP BY projection-scope checks. - GraphPattern::add_in_scope_variables in ast/pattern.rs — the SPARQL 1.1 §18.2.1 in-scope variable definition (MINUS right side excluded, FILTER contributes nothing, sub-SELECT contributes its projection, annotation-tail variables included). Shared by the GROUP BY projection pass, the BIND scope pass, the duplicate-alias pass, and the SPARQL 1.2 nested-aggregate check.
…atterns
New validate-time pass (validate/bnode_scope.rs, DiagCode V001) enforcing
SPARQL 1.1 §19.6: a blank-node label may not appear in two different
basic graph patterns. Boundary set per the cluster-doc decision: GRAPH /
OPTIONAL / UNION / MINUS / SERVICE / nested { } group / sub-SELECT end a
BGP; FILTER (per §18.2.2.5) — and, conservatively, BIND / VALUES /
property paths — do not. EXISTS patterns are scanned as their own groups.
Consecutive Bgp siblings inside a Group can only arise from an explicitly
braced block (the parser merges plain adjacent triples and simplifies
single-pattern groups), so they are assigned distinct scopes — this is
what distinguishes '{ _:a ?p ?v . { _:a ?q 1 } }' (illegal) from a
FILTER-split single BGP (legal).
Applies to query WHERE patterns only; update operations keep their own
blank-node rules (PR-U territory).
Register: −11 SPARQL10_SYNTAX entries (blabel-cross-{graph,optional,
union}-bad, syn-bad-34..38, syn-bad-{GRAPH,OPT,UNION}-breaks-BGP); the
13 remaining accepts-invalid entries are V1/V2 dot-and-FILTER grammar
tightening, reclassified for PR-3.
New validate-time pass (validate/projection.rs, DiagCodes V002/V003) enforcing SPARQL 1.1 §11 / §18.2.4: in a grouped query (explicit GROUP BY, or an aggregate in the projection = implicit single group) every projected variable must be a group key or appear only inside an aggregate, and SELECT * is not permitted with GROUP BY. Runs on the top-level SELECT clause and on every sub-SELECT. Deliberate leniencies to avoid over-rejection: GROUP BY (?v) counts as the key ?v; aliases assigned by earlier items in the same SELECT clause are usable in later projection expressions; HAVING / ORDER BY are not checked (no conformance test requires it — grammar-note scope only). Register: −9 (SPARQL11_AGGREGATES agg08-agg12, SPARQL11_GROUPING group06/group07 → suite fully green, SPARQL11_SYNTAX_QUERY test_43/test_44). test_45/test_60/61a/62a/65 remain for the V5/V6 passes.
Parse-time check (DiagCode V004) enforcing SPARQL 1.1 §10.1 / grammar
note 12: the variable assigned by BIND(expr AS ?v) must not already be
in scope in the containing group graph pattern up to that point. The
in-scope set accumulates over the preceding siblings via the shared
GraphPattern::add_in_scope_variables walker (§18.2.1 rules: nested
groups and UNION branches propagate out, MINUS right side and FILTER
variables do not; only elements BEFORE the BIND count).
Placement note: this pass lives in parse_group_graph_pattern, not
validate(), by necessity — the parser's single-pattern group
simplification makes '{ ... { BIND(e AS ?v) } }' (legal, fresh scope,
W3C syntax-BINDscope2) and '{ ... BIND(e AS ?v) }' (illegal, W3C
syntax-BINDscope6) produce byte-identical ASTs, so only the parser can
tell them apart. Changing the AST shape would alter the lowering
invariant that decides scope-boundary subqueries (out of scope here).
Per roadmap D-4 the new error prevents AST production in parse_sparql —
recovered-error ASTs would otherwise execute through the API's
parse-then-validate path, which only inspects diagnostics when the AST
is absent.
Register: −3 SPARQL11_SYNTAX_QUERY (test_60, test_61a, test_62a).
Validate-time check (DiagCode V005) enforcing SPARQL 1.1 §19.8 grammar note 13: the variable assigned in (expr AS ?v) must not be assigned by an earlier item in the same SELECT clause, nor already be in scope in the WHERE pattern (§18.2.1 — a sub-SELECT projects its variables into the outer pattern's scope). Using an earlier alias in a later expression remains legal; only re-assignment is rejected. Applies to the top-level SELECT clause and every sub-SELECT (each against its own pattern, keeping the positive syntax-SELECTscope1/3 shapes green). Empirically it is the in-scope half of this pass — not V4/V5 — that greens test_65 (syntax-SELECTscope2): the query has no GROUP BY, no aggregate, and no BIND; the violation is the outer (1 AS ?X) colliding with the sub-SELECT's projected ?X. Register: −2 SPARQL11_SYNTAX_QUERY (test_45 duplicate-alias, test_65 alias-in-scope); the suite's accepts-invalid class is now empty (the 4 remaining entries are rejects-valid parser gaps).
…LUES variables Two validate-time checks for the SPARQL12_SYNTAX negative tests (DiagCodes V006/V007), sharing the PR's walkers: - Nested aggregates: an aggregate call may not appear inside another aggregate's argument (COUNT(COUNT(*))). Checked over SELECT projections and GROUP BY / HAVING / ORDER BY expressions, top-level and sub-SELECT. - Duplicated VALUES variable: the VALUES variable list must be distinct. Implemented in the Values arm of the graph-pattern walk, so it covers inline VALUES everywhere (queries, sub-SELECTs, update WHERE clauses) plus the post-query VALUES clause, which is now routed through the same walk. Register: −2 SPARQL12_SYNTAX (nested-aggregate-functions, duplicated-values-variable) → suite fully green.
…masking
Making parse errors authoritative at the API seam exposed four W3C
queries that only worked because the API executed error-recovered ASTs
(bindings#values7, bindings#inline2, sort#dawg-sort-builtin,
sort#dawg-sort-function) plus two whose dropped tails the new
trailing-token assertion now surfaces (syntax-order-06/07). All are
valid SPARQL the parser rejected:
- ORDER BY bare Constraint (OrderCondition ::= ... | ( Constraint | Var )):
'ORDER BY str(?o)' / 'ORDER BY xsd:integer(?o)' now parse via a
try-parse-with-restore branch in parse_order_condition. Hunk is
byte-identical to wave-0 PR-1's P5b fix so the branches merge cleanly.
- VALUES row shape follows the var-LIST shape, not the variable count:
a parenthesized single-var list ('VALUES (?x) { (:b) }') takes
parenthesized rows (InlineDataFull), only a bare var takes bare values
(InlineDataOneVar).
- SubSelect trailing VALUES clause (SubSelect ::= SelectClause
WhereClause SolutionModifier ValuesClause): parsed into a new
SubSelect::values field and lowered by joining the VALUES table into
the subquery's pattern list before projection (spec 18.2.4.3 insertion
point for modifier-free subqueries; approximate under GROUP BY).
Greens W3C syntax-order-07 (register entry removed); keeps values7,
inline2, dawg-sort-builtin, dawg-sort-function, and syntax-order-06
green once parse errors reject at the API.
…validate time
Validate-time check (DiagCode V008) for the one wave-2 negative:
an annotation tail with no explicit reifier id (a bare '{| ... |}'
block or bare '~') mints an anonymous reifier, which has no
addressable identity to delete — invalid in DELETE DATA ground data
(W3C sparql12 syntax-update-anonreifier-02, which was verified
parseable-but-unvalidated at this branch's base: the request parses
via the trailing-token hole and validate() raised no error).
This mirrors the existing lowering-time rejection in
fluree-db-transact (AnnotationExpansionMode::DeleteData) with the same
message text, so it now also fires on the parse+validate-only paths
(the W3C syntax harness, standalone validation) — surfacing earlier,
before lowering, on the API path.
INSERT DATA is deliberately NOT tightened: anonymous annotation blocks
there are Fluree's committed SPARQL 1.2 edge-annotation transact
surface (pinned by it_query_sparql_annotations.rs) — recorded as a
reviewed divergence in the PR description. The W3C test greens on its
DELETE DATA half.
Register: −1 SPARQL12_SYNTAX_TRIPLE_TERMS_NEGATIVE → suite fully green.
DELETE WHERE { GRAPH <g> { ... } } previously failed twice: the validator
rejected the GRAPH block outright (making W3C syntax-update-1 test_36 fail
as a positive syntax test) and, for callers bypassing validation, lowering
hard-errored with UnsupportedFeature (failing dawg-delete-where-02/04/06).
DELETE WHERE is shorthand for DELETE { P } WHERE { P }, and the Modify path
already supports concrete-IRI GRAPH blocks end to end. Route GRAPH-bearing
DELETE WHERE patterns through that same machinery: the quad pattern becomes
a stored SparqlWhereClause (staging-time lowering via the shared query
engine, which evaluates GRAPH blocks) plus graph-scoped delete templates
via lower_quad_pattern_to_templates. The triple-only fast path is
unchanged, including its blank-node existential-variable rewriting; on the
new GRAPH path non-stable blank nodes are rewritten to reserved
_fluree_bn_* variables shared by WHERE and templates, and stable _:fdb-
ids keep addressing the stored node as constants.
The validator now applies the same template rule as Modify (GRAPH <iri>
allowed, GRAPH ?var still rejected as Phase 1) instead of rejecting every
GRAPH block, and the parity-test table in the compliance doc gains the
UPDATE/transact row (it_transact_update.rs, it_named_graphs.rs).
Register shrink (-5 across both registers): syntax-update-1 test_36 (both
copies), dawg-delete-where-02/04/06.
Surface parity: IR/engine-level fix. New JSON-LD regression test
(graph-scoped delete-where via top-level "graph") plus a SPARQL
DELETE WHERE { GRAPH } integration test in it_named_graphs.rs.
SPARQL 1.1 Update (§19.8 grammar note 8) forbids blank nodes in DELETE DATA, DELETE WHERE, and the DELETE template of a Modify operation: a blank node denotes a fresh node and can never match existing data, so the retraction skolemized a brand-new SID and silently matched nothing. No validation existed, failing eleven W3C negative syntax tests. Enforce the rule at three points, each with a clear error naming the rule and the alternatives (variable bound by WHERE, concrete IRI, stable id): - validator: new DiagCode::BlankNodeInDelete (F010) for blank nodes in DELETE DATA ground data, DELETE WHERE patterns, and Modify DELETE templates, including inside GRAPH <iri> blocks. INSERT DATA and INSERT templates keep CONSTRUCT-style fresh-mint blank nodes. - SPARQL lowering: new LowerError::BlankNodeInDelete for DELETE DATA and Modify DELETE templates, so the transact builders (which lower without running validate()) reject too — mirroring exactly where the annotation machinery already rejects blank reifiers (rejects_blank_reifier: DeleteData | DeleteTemplate). - JSON-LD surface: delete templates containing a blank node (explicit "_:..." @id or a nested object without @id, which mints one) are rejected at parse with the same rationale. Two deliberate carve-outs preserve documented Fluree extensions: stable _:fdb- ids stay legal in every DELETE form (constants addressing the stored node; the validator duplicates the feature-gated prefix constant with a lowering-feature sync test), and DELETE WHERE blank nodes keep their existential-variable semantics at the lowering layer while the strict SPARQL surface (validate()) rejects them per spec. Register shrink (-14 across both registers): syntax-update-1 test_50/51/52 (both copies), dawg-delete-insert-03/03b/05/06/07/07b/08/09. test_54 stays (cross-operation bnode scoping, PR-U2). Surface parity: surface-validation fix; the JSON-LD transact surface CAN express a bnode-in-delete, so it gains the same rejection plus the negative test update_delete_blank_node_rejected in it_transact_update.rs.
parse_sparql returned after one parsed operation with no EOF check, so
anything after it was silently discarded. The worst case is a standard
multi-operation SPARQL UPDATE ('INSERT ...; DELETE ...'): the INSERT
committed and every following operation vanished — silent data loss
through graph().transact().sparql_update().commit() (#1438).
The parse entry now asserts EOF after a complete Query/Update:
- any trailing token after a query form is an error-severity diagnostic;
- for updates, one trailing ';' stays legal (Update ::= Prologue
( Update1 ( ';' Update )? )? with an empty recursive Update), but
content after it is rejected with an explicit multi-op error naming
the limitation (D-10a interim guard; PR-U2 replaces the rejection
with request-level multi-op support and reuses this single
entry-point assertion).
With parse errors authoritative at both API seams, the two-op case is
verified end-to-end in it_transact_update: the request errors loudly
and stages nothing; a single op with a trailing ';' still commits.
Register deltas: syntax-update-1#test_54 (both copies) and
triple-terms-negative#syntax-update-anonreifier-02 now pass (loud
rejection is what those negatives demand) — entries removed;
delete-insert#dawg-delete-insert-01b and
triple-terms-positive#update-reifier-08 are genuinely multi-op W3C
tests now failing loudly instead of passing by silent truncation —
registered with comments pointing at PR-U2/#1438.
Five it_query_sparql.rs tests pinned SPARQL shapes that the V4 projection-scope pass now rejects (they are literally the W3C agg08/group06 negative-syntax shapes): - issue-#1362 and field-P0 GROUP-BY-expression tests re-projected an UNALIASED key expression (SELECT (LCASE(?cur) AS ?k) ... GROUP BY (LCASE(?cur)) — agg08's shape). Migrated to the spec-valid aliased key (GROUP BY (expr AS ?k) ... SELECT ?k); the unaliased-collapse engine path stays covered by sparql_group_by_bare_builtin_not_projected (aggregate-only projection), and a new sparql_group_by_reprojected_key_expression_rejected pins the rejection and documents the migration. - two tests pinned Fluree's grouped-list extension on the SPARQL surface (SELECT ?person ?favNums ... GROUP BY ?person — group06's shape). Rewritten to assert the rejection plus a spec-valid aggregate equivalent preserving each test's intent (OPTIONAL-null-through- grouping via COUNT; multi-key grouping with AVG/MAX). Grouped-list projection remains a JSON-LD-surface feature (see it_query_grouping.rs). Behavior change is deliberate under roadmap decision D-4 (hard error + changelog); the PR description carries the migration note.
…sses New fluree-db-api/tests/it_query_grouping.rs (grp_query), per docs/contributing/sparql-compliance.md § Query Surface Parity: - SPARQL surface: hard-error assertions through the public API for V3 (bnode cross-scope), V4 (ungrouped projection, SELECT *+GROUP BY), V5 (BIND rescope), V6 (duplicate / in-scope alias), and the SPARQL 1.2 nested-aggregate and duplicated-VALUES checks — plus still-works controls for valid grouped and BIND queries. - JSON-LD surface divergence pins (deliberately NOT changed by this PR; tightening JSON-LD needs its own decision): ungrouped select under groupBy stays accepted and projects a per-group LIST (the long-standing analytical feature), and bind on an already-bound variable stays accepted, empirically behaving as a join/constraint on the existing binding (conflicting expression yields zero rows, consistent expression keeps them). V3/V6/nested-aggregate/dup-VALUES are SPARQL-surface-only rules (no JSON-LD analogue); recorded as such in the test docs.
…notation IR (burn-down PR-W2A) Parser (bucket A/D of the wave-2 triple-terms cluster): - reified triples << s p o ~ r? >> in object position, nested, and as standalone statements (SPARQL 1.2 ReifiedTriple with an empty property list), including in CONSTRUCT WHERE shorthand - in-triple reifiers (~ id?) with IRI / blank-node / variable ids; the reifier-less subject form stays eligible for the legacy f:t / f:op history reading (selected at lowering) - annotation tails as full (reifier | annotationBlock)* sequences, grouped into AnnotationUnit per the RDF 1.2 attachment rule (multi-reifier / multi-block, interleaved) - annotation-block bodies as PropertyListPathNotEmpty (path verbs, reified-triple objects) - VERSION specifier narrowed to short quoted strings (long strings were only rejected by accident before the standalone-<<>> fix; W3C negative version-bad-01/02) Lowering routes everything the reifier model can evaluate onto the existing IR: reified-triple terms desugar per spec to their reifier ref plus a sibling Pattern::AnnotationTarget (r rdf:reifies <<(s p o)>>), memoized per occurrence so ;-continued patterns share one reifier; annotation units lower to one Pattern::EdgeAnnotation each over the shared base edge; block path verbs reuse the property-path lowering with the reifier as subject. Non-evaluable positions (UPDATE templates, path subjects, VALUES) defer with clean not_implemented / UnsupportedFeature errors per burn-down decision D-1 (accept-then-defer) instead of parse errors. Validation keeps the W3C negative suite intact: anonymous annotation units in DELETE templates / DELETE WHERE are rejected (F010, syntax-update-anonreifier-01 — previously rejected only by the accidental path-in-block parse failure), and INSERT/DELETE DATA ground checks recurse into reified-triple terms. The INSERT/DELETE DATA anonymous-annotation validation stays with sibling burn-down PR-2. fluree-db-transact: mechanical AnnotationUnit adoption — the UPDATE expansion emits one f:reifies* bundle per unit, the user-authored f:reifies firewall walks path verbs' IRI leaves, and reified-triple template terms reject cleanly instead of panicking.
…ion tests Per compliance § Query Surface Parity (and the ROADMAP PR-W2A row's explicit deliverable): insert through the JSON-LD @annotation surface, query through the NEW SPARQL 1.2 reifier syntax, and assert the results are identical to the established ?ann rdf:reifies <<( ... )>> form — proving the new forms reach the same Pattern::EdgeAnnotation / Pattern::AnnotationTarget machinery and annotation storage: - standalone reified triple << s p o ~ ?r >> . vs rdf:reifies - object-position reified triple joining a JSON-LD-written NAMED reifier (@annotation @id) cited by another JSON-LD triple - two annotation units (~ ?a {| .. |} ~ ?b {| .. |}) both binding the single reifier one @annotation wrote (row count stays 1, ?a = ?b)
…rms (-61) - SPARQL12_SYNTAX_TRIPLE_TERMS_POSITIVE: 85 -> 27. All 58 bucket-A/D reifier-form entries green; the remaining 27 are buckets B (3 triple-term builtins) and C (24 bare triple-term values), owned by sibling PR-W2BC under decision D-1 — comments re-pointed. - SPARQL12_VERSION: 3 -> 0. version-01/02/05 failed only on the bare << s p o >> patterns in their bodies (ROADMAP §6.1 comment correction), which the reifier-form work greened; version-bad-01/02 stay rejected via the VERSION short-string check. - SPARQL12_SYNTAX_TRIPLE_TERMS_NEGATIVE: unchanged (1) — the INSERT/DELETE DATA annotation validation is sibling PR-2's; ownership note added. syntax-update-anonreifier-01 stays rejected through the new DELETE-template validator. Full suite: 36/36 categories green, 0 unexpected passes / stale entries.
# Conflicts: # testsuite-sparql/tests/registers/mod.rs
# Conflicts: # fluree-db-sparql/src/parse/query/mod.rs # fluree-db-sparql/src/parse/query/term.rs # fluree-db-sparql/src/parse/query/tests.rs # testsuite-sparql/tests/registers/mod.rs
# Conflicts: # fluree-db-sparql/src/diag/mod.rs # fluree-db-sparql/src/validate/mod.rs # testsuite-sparql/tests/registers/mod.rs
# Conflicts: # fluree-db-sparql/src/diag/mod.rs # fluree-db-sparql/src/validate/mod.rs # fluree-db-transact/src/lower_sparql_update.rs # testsuite-sparql/tests/registers/mod.rs
Union-resolved register merges leave supersets; the both-way CI gate then reports every entry the integrated branch actually greens (56 tests, 61 lines incl. double-registered copies) — cross-PR interactions like the update-syntax tests greening under PR-2+PR-3+PR-U1's combined defenses resolve mechanically instead of by hand-tracked set math.
tests.rs rebuilt deterministically from the clean parents (pr-3's single inserted block appended; pr-w2a's units-API test migrations applied via their base anchors) after interleaved conflict hunks with shared boilerplate context defeated naive both-sides resolution; SubSelect test literals gain the new values field; unused Annotation import dropped.
bplatz
reviewed
Jul 8, 2026
| bnode_scope::check_blank_node_scopes(pattern, &mut self.diagnostics); | ||
| } | ||
|
|
||
| fn validate_update(&mut self, op: &UpdateOperation) { |
Contributor
There was a problem hiding this comment.
validate_update — and the UPDATE rules under it (BlankNodeInDelete, AnonymousAnnotationInDelete, UnsupportedGraphInUpdate) — is never reached on the production UPDATE path. fluree-db-api's parse_and_lower_sparql_update only checks parse-level has_errors() and then lowers; it never calls validate(). Enforcement today relies entirely on the lowering-side mirrors (reject_blank_nodes_in_delete_quad_pattern, the GraphName::Var rejection, resolve_reifier's anon/blank-reifier guards), so behavior is currently correct — but this is fragile in two ways: a W3C negative-update test can go green via the harness's own validate() call while production takes a different code path, and any future validator-only UPDATE rule would silently not apply to real updates. Suggest either wiring validate() into the update seam or documenting explicitly that lowering is the authoritative enforcement point for UPDATE.
| let span = stream.current_span(); | ||
| stream.add_diagnostic(Diagnostic::error( | ||
| DiagCode::ExpectedToken, | ||
| "multi-operation SPARQL UPDATE requests (';'-separated) are not \ |
Contributor
There was a problem hiding this comment.
This multi-op guard (the #1438 silent-data-loss fix) pushes an error diagnostic but does not null the AST — unlike V5's BindTargetAlreadyInScope, which suppresses AST production. The in-tree seams (parse_and_lower_sparql_update, parse_and_validate_sparql) all check has_errors(), so it's loud where it matters. But the public lower_sparql_update_ast export has no internal guard: a caller that reads .ast without first checking has_errors() still gets a valid single-op AST and executes only the first operation — re-opening exactly the #1438 window this guard exists to close. Consider nulling the AST here (or guarding inside the lowering entry point) as belt-and-suspenders, given the failure mode is silent data loss.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reviewer quick reference
End-user API/contract changes — this is the reject-more wave; read this list carefully. SPARQL inputs that previously "worked" and now error (each spec-mandated, each with a changelog/migration note in its section): (a)
GROUP BY (expr)with the raw expression reprojected — the field/#1362 shape — must alias the key (GROUP BY (expr AS ?k)); (b) Fluree's grouped-list projection extension is removed from SPARQL only (JSON-LD keeps it, parity-pinned); (c) BIND-scope, duplicate-alias, bnode-cross-scope, and GROUP-projection violations now reject; (d) stray/doubled dots and bare FILTER terms now reject (V1 is the highest-regression-risk item — users may rely on dot leniency; isolated commits for bisect/revert); (e) parse errors are now authoritative at the API: a query whose parse produced error diagnostics no longer executes a recovered AST; (f) multi-operation UPDATE requests error loudly instead of silently executing only the first operation (interim guard; real support lands in Wave 2); (g) blank nodes in DELETE forms are rejected on both the SPARQL and JSON-LD transact surfaces (_:fdb-stable ids exempt).Performance verdict: entirely parse/validate-time and update-staging work — zero per-row engine code touched. BSBM measured anyway on the shared parse entry (numbers below): HEAD ≤ base on all three queries.
Production bugs found & fixed along the way: the API diagnostics-swallowing hole —
fluree.queryexecuted error-recovered ASTs, silently answering a different question than asked (this is the wave's most important product fix); five "green-by-recovery" W3C tests that only passed because of that hole (their real parser gaps are fixed here, not registered); the #1438 silent-data-loss window closed (loud guard, verified end-to-end).Decisions actioned in this PR
D-4 (reject-more behavior changes ship as hard errors) — actioned by pr-2 and pr-3.
D-10a (fast-track the multi-op guard) — actioned by pr-3.
;-loop support in Wave 2 — chosen; (2) wait for full multi-op support — rejected: every day in between, standard multi-op SPARQL UPDATE requests silently commit only their first operation (SPARQL UPDATE: multi-operation (;) requests silently execute only the first operation #1438 — silent data loss in production).D-1 (SPARQL 1.2 triple-term strategy: accept-then-defer) — actioned by pr-w2a for the reifier forms.
not_implementedfor the rest — chosen; (2) reject-on-principle (documented-divergence register) — rejected: keeps 60+ syntax tests red for forms the engine can actually evaluate; (3) desugar triple terms to reifiers — rejected as semantically wrong: the 1.2 grammar itself distinguishesBIND(<<( )>>)(valid, a value) fromBIND(<< >>)(invalid, a reifier) — desugaring would green syntax while corrupting eval semantics.not_implementedinstead of a parse error.Wave 1 of the W3C SPARQL burn-down (4 components, ≈−123 register entries)
Stacks on the Wave-0 PR. This wave is the reject-more + validation wave: it makes the parser and API say "no" where the spec requires it. Every component carries the decision-transparency and changelog obligations for the behavior changes it ships (D-4 was adopted as hard-errors-with-changelog; the options considered are in each section and ROADMAP §4).
Components
1. Semantic validation passes (
pr-2, −28). V3 blank-node label scope across BGP boundaries, V4 GROUP BY/aggregate projection scope, V5 BIND scope, V6 duplicate/in-scope SELECT aliases — four validation passes that previously did not exist anywhere — plus the SPARQL 1.2 nested-aggregate and duplicated-VALUES-variable checks. The two AST walkers (free-variable collector,contains_aggregate) were written from scratch (the audit's claim that one existed was refuted). V5 lives in the parser by necessity: the single-pattern group simplification makes the legal and illegal BIND-scope shapes byte-identical as ASTs. Behavior changes your review should consciously bless (both pinned by tests + in the migration note): (a)GROUP BY (expr)+ reprojection — the field/#1362 shape, literally W3Cagg08— is now rejected; alias the key (GROUP BY (expr AS ?k)). (b) Fluree's SPARQL grouped-list projection extension is literally W3Cgroup06— removed from the SPARQL surface; JSON-LD keeps it, with parity pins documenting the deliberate divergence.2. Grammar tightening + the diagnostics seam (
pr-3, −15). V1 dot-structure enforcement (same token flow, no backtracking), V2 FILTER-requires-Constraint, and the product-critical seam fix: parse-error diagnostics are now authoritative at the API even when error recovery produced an AST — previouslyfluree.querysilently executed recovered ASTs that answered a different question than the user asked. That fix immediately unmasked five "green-by-recovery" tests, whose real gaps (bare ORDER BY constraints, VALUES row shape, sub-SELECT VALUES lowering) are fixed here rather than registered. Also owns the single trailing-token/EOF assertion: multi-op UPDATE requests stopped silently executing only their first operation and errored loudly instead (D-10a interim guard for #1438; verified end-to-end). Decision transparency: actions D-4 and D-10a; V1 is flagged as the highest-regression-surface change in the wave (users may rely on dot leniency) — isolated commits for easy bisect.3. UPDATE validation (
pr-u1, −19). Class D:GRAPHblocks insideDELETE WHEREnow route through the existing Modify graph-scoped-template lowering (both prior rejections removed; triple-only fast path untouched). Class E: blank nodes in DELETE forms rejected across validator + lowering + the JSON-LD transact surface (which can express a bnode-in-delete — behavior change, in the migration note), with the stable_:fdb-ids exempted. Compliance-doc parity table gains the UPDATE/transact row.4. RDF 1.2 reifier-form parser (
pr-w2a, −61). Object-position/nested/standalone<< s p o ~ r >>,~variants, multi-reifier/multi-block annotation tails grouped intoAnnotationUnits per the RDF 1.2 attachment rule, property-path verbs in annotation bodies — all lowered to the existingPattern::EdgeAnnotationmachinery where evaluable, cleannot_implementedwhere deferred. Kept the negative suite green by design (new VERSION short-string check; anon-annotation-in-DELETE validator). Includes the 3SPARQL12_VERSIONentries (VERSIONalready parsed; the failures were reified-triple bodies). Decision transparency (D-1): accept-then-defer was chosen over reject-on-principle and over desugar-to-reifier (the latter is semantically wrong: the 1.2 grammar distinguishes triple terms from reifiers); trade-off — deferred forms now fail at runtime withnot_implementedinstead of at parse. JSON-LD parity: byte-equal results between@annotationinserts queried viardf:reifiesand via the new syntax.Integration notes
syntax-order-07and pr-2∩pr-3anonreifier-02double-claims resolved mechanically by the gate).tests.rswas rebuilt deterministically from the clean parents after interleaved test-section hunks defeated naive merging;SubSelectgained pr-3'svaluesfield across all initializers; pr-2's walkers were migrated to pr-w2a'sAnnotationUnitAST.Verification
Full W3C suite 36/36, 0 failed, 0 stale;
fluree-db-sparql601 unit tests;grp_query_sparql274,grp_transact171; fmt + clippy clean.Issues closed when merged: #1438's interim guard (full fix in Wave 2).
Performance notes (per component)
Validation passes (pr-2): run once per parse over the AST; the two new walkers are simple recursive visitors. No runtime/engine code touched; nothing per-row.
Grammar tightening + API seam (pr-3): V1/V2 preserve the existing token flow (a
dot_allowedflag and a post-parse AST-shape predicate — no backtracking added to the happy path); the EOF assertion is a single end-of-parse check; the API seam fix is one branch at query submission.query_hot_bsbmbefore/after: 538/682/950 µs (HEAD) vs 579/735/1005 µs (base) for q3/q5/q9 — HEAD ≤ base across the board; later same-code reruns showed the box noisy under concurrent sessions, so read this as "no regression," not as an improvement claim. No per-row code touched.UPDATE validation (pr-u1): update staging only — the query hot path is untouched by construction. GRAPH-in-DELETE-WHERE routes through the existing Modify graph-scoped-template lowering rather than new machinery; the triple-only DELETE WHERE fast path is preserved.
Reifier-form parser (pr-w2a): parse-time + lowering onto the existing
Pattern::EdgeAnnotationoperators — D-1's Option 1 was chosen precisely because it carries zero storage/index/perf risk. The grownAnnotationTargetIR variant is boxed to keep the pattern-enum width unchanged (enum width is the real hot-path exposure for IR types). Deferred positions cost nothing at runtime (they error at lowering).Issues closed on merge
None auto-closed by this PR — deliberately. #1438's window is closed here (silent data loss → loud error, verified end-to-end with a two-op request that previously committed only its first operation), but the fix — real multi-operation support — lands in the Wave 2 PR, which carries the
Closes #1438keyword.